Обсуждение: Annoying warning in SerializeClientConnectionInfo

Hi,

When building without assertions with a rather recent gcc (trunk built in the
last weeks), I get this warning:

../../../../../home/andres/src/postgresql/src/backend/utils/init/miscinit.c: In function
'SerializeClientConnectionInfo':
../../../../../home/andres/src/postgresql/src/backend/utils/init/miscinit.c:1102:36: warning: parameter 'maxsize' set
butnot used [-Wunused-but-set-parameter=]
 
 1102 | SerializeClientConnectionInfo(Size maxsize, char *start_address)
      |                               ~~~~~^~~~~~~

And the warning is right. Not sure why a new compiler is needed, IIRC this
warning is present in other cases with older compilers too.

The most obvious fix is to slap on a PG_USED_FOR_ASSERTS_ONLY. However, we so
far don't seem to have used it for function parameters... But I don't see a
problem with starting to do so.

Greetings,

Andres Freund

On Mon, Aug 11, 2025 at 3:52 PM Andres Freund <andres@anarazel.de> wrote:
> And the warning is right. Not sure why a new compiler is needed, IIRC this
> warning is present in other cases with older compilers too.

Probably
    https://github.com/gcc-mirror/gcc/commit/0eac9cfee
which was committed last month.

Andy Fan reported this as well [1] but I did not see it at the time.
:( Sorry, Andy, my email had changed.

> The most obvious fix is to slap on a PG_USED_FOR_ASSERTS_ONLY. However, we so
> far don't seem to have used it for function parameters... But I don't see a
> problem with starting to do so.

WFM. Do you have any opinions on our use of maxsize in general? I
think there are other serialization functions that just assert, but it
looks like some are more actively throwing errors if there's not
enough space. Given the subject matter here I'm wondering if we should
take the stricter approach.

Thanks,
--Jacob

[1] https://postgr.es/m/875xyqhzs6.fsf%40163.com

On Mon, Aug 11, 2025 at 04:30:30PM -0700, Jacob Champion wrote:
> Probably
>     https://github.com/gcc-mirror/gcc/commit/0eac9cfee
> which was committed last month.
>
> Andy Fan reported this as well [1] but I did not see it at the time.
> :( Sorry, Andy, my email had changed.

Warning remains undetected here.

>> The most obvious fix is to slap on a PG_USED_FOR_ASSERTS_ONLY. However, we so
>> far don't seem to have used it for function parameters... But I don't see a
>> problem with starting to do so.

I'm guessing that it should be OK, pg_attribute_unused is only
available for __GNUC__, still wondered if MinGW+gcc would like that.

[ .. 30 minutes later, after a test .. ]

I have kicked one job, to note that the libpq build is broken on
Windows, bit due to a different thing because a bunch of other CF bot
jobs are failing the same way:
https://github.com/michaelpq/postgres/runs/47868772065

And the rest was looking OK, so appending a
PG_USED_FOR_ASSERTS_ONLY in the declaration seems OK from here.

> WFM. Do you have any opinions on our use of maxsize in general? I
> think there are other serialization functions that just assert, but it
> looks like some are more actively throwing errors if there's not
> enough space. Given the subject matter here I'm wondering if we should
> take the stricter approach.

I'd rather keep the sanity check on maxsize, even if it means to have
a tweak based on the size of SerializedClientConnectionInfo.  If you
feel differently about that as the original author of this code, I'd
be OK with what you want to prioritize here.

Another thing that we can do is use an USE_ASSERT_CHECKING around the
variable getting set, but as far as I can see the
PG_USED_FOR_ASSERTS_ONLY in the function declaration should work fine.
If the buildfarm blurps on the first approach, we could always use the
second approach as fallback.
--
Michael

Hi,

On 2025-08-11 16:30:30 -0700, Jacob Champion wrote:
> On Mon, Aug 11, 2025 at 3:52 PM Andres Freund <andres@anarazel.de> wrote:
> > And the warning is right. Not sure why a new compiler is needed, IIRC this
> > warning is present in other cases with older compilers too.
> 
> Probably
>     https://github.com/gcc-mirror/gcc/commit/0eac9cfee
> which was committed last month.
> 
> Andy Fan reported this as well [1] but I did not see it at the time.
> :( Sorry, Andy, my email had changed.
> 
> > The most obvious fix is to slap on a PG_USED_FOR_ASSERTS_ONLY. However, we so
> > far don't seem to have used it for function parameters... But I don't see a
> > problem with starting to do so.
> 
> WFM. Do you have any opinions on our use of maxsize in general?

It's somewhat odd.


> I think there are other serialization functions that just assert, but it
> looks like some are more actively throwing errors if there's not enough
> space. Given the subject matter here I'm wondering if we should take the
> stricter approach.

I think it'd be fine to error out here. For many asserts we don't want them in
real builds because they'd (in very small increments) make the code slower and
bigger. But this is very very far from a hot path...

Greetings,

Andres Freund

On Mon, Aug 11, 2025 at 7:34 PM Michael Paquier <michael@paquier.xyz> wrote:
> And the rest was looking OK, so appending a
> PG_USED_FOR_ASSERTS_ONLY in the declaration seems OK from here.

If we're the first to use the attribute this way, I think I'd prefer
to put it on the definition only.

> I'd rather keep the sanity check on maxsize, even if it means to have
> a tweak based on the size of SerializedClientConnectionInfo.

I don't think I understand what you mean by this? I don't want to get
rid of the check, but I was wondering if we could strengthen the
behavior on HEAD to raise an ERROR regardless of whether assertions
are enabled or not. Similar to the approach taken by
SerializeComboCIDState().

I think the PG_USED_FOR_ASSERTS_ONLY fix is preferable for backport,
so I don't want to get in the way of that approach.

> Another thing that we can do is use an USE_ASSERT_CHECKING around the
> variable getting set, but as far as I can see the
> PG_USED_FOR_ASSERTS_ONLY in the function declaration should work fine.
> If the buildfarm blurps on the first approach, we could always use the
> second approach as fallback.

Agreed.

Thanks,
--Jacob

On Tue, Aug 12, 2025 at 7:31 AM Andres Freund <andres@anarazel.de> wrote:
> On 2025-08-11 16:30:30 -0700, Jacob Champion wrote:
> > WFM. Do you have any opinions on our use of maxsize in general?
>
> It's somewhat odd.

Okay, glad it's not just me :D

--Jacob

On Tue, Aug 12, 2025 at 01:44:32PM -0700, Jacob Champion wrote:
> I don't think I understand what you mean by this? I don't want to get
> rid of the check, but I was wondering if we could strengthen the
> behavior on HEAD to raise an ERROR regardless of whether assertions
> are enabled or not. Similar to the approach taken by
> SerializeComboCIDState().

Yeah, we could do that as well.  I was looking at all routine calls,
but did not notice the elog(ERROR) thrown in this case for the
combocid case.

> I think the PG_USED_FOR_ASSERTS_ONLY fix is preferable for backport,
> so I don't want to get in the way of that approach.

The attached has been working for me.  Thoughts?
--
Michael

Hi,

On 2025-08-13 12:54:27 +0900, Michael Paquier wrote:
> > I think the PG_USED_FOR_ASSERTS_ONLY fix is preferable for backport,
> > so I don't want to get in the way of that approach.
> 
> The attached has been working for me.  Thoughts?

I think we shouldn't add the attribute to the declaration, just to the
function. The caller doesn't need to know that it's unused, it's purely a
question of the specific implementation that the attribute is unused.

Greetings,

Andres Freund

On Wed, Aug 13, 2025 at 8:28 AM Andres Freund <andres@anarazel.de> wrote:
> I think we shouldn't add the attribute to the declaration, just to the
> function. The caller doesn't need to know that it's unused, it's purely a
> question of the specific implementation that the attribute is unused.

+1

--Jacob

On Wed, Aug 13, 2025 at 08:30:19AM -0700, Jacob Champion wrote:
> On Wed, Aug 13, 2025 at 8:28 AM Andres Freund <andres@anarazel.de> wrote:
>> I think we shouldn't add the attribute to the declaration, just to the
>> function. The caller doesn't need to know that it's unused, it's purely a
>> question of the specific implementation that the attribute is unused.
>
> +1

Okay, done so only in the function, then.
--
Michael

On Thu, Aug 14, 2025 at 12:23 AM Michael Paquier <michael@paquier.xyz> wrote:
> Okay, done so only in the function, then.

Thanks! I'll keep thinking about the assert-vs-error, but there are a
pile of TODOs that are in line first.

--Jacob