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 but not 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]/messages/by-id/875xyqhzs6.fsf@163.com 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
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
Attachments:
0001-Append-PG_USED_FOR_ASSERTS_ONLY-on-a-function-variab.patchtext/x-diff; charset=us-asciiDownload
From 35cabb3ead7cb4b128ae093c0f39f2a65a94c2e2 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Tue, 12 Aug 2025 11:10:07 +0900
Subject: [PATCH] Append PG_USED_FOR_ASSERTS_ONLY on a function variable
---
src/include/miscadmin.h | 3 ++-
src/backend/utils/init/miscinit.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/include/miscadmin.h b/src/include/miscadmin.h
index 1bef98471c36..4d8e827a269a 100644
--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -533,7 +533,8 @@ typedef void (*shmem_request_hook_type) (void);
extern PGDLLIMPORT shmem_request_hook_type shmem_request_hook;
extern Size EstimateClientConnectionInfoSpace(void);
-extern void SerializeClientConnectionInfo(Size maxsize, char *start_address);
+extern void SerializeClientConnectionInfo(Size maxsize PG_USED_FOR_ASSERTS_ONLY,
+ char *start_address);
extern void RestoreClientConnectionInfo(char *conninfo);
/* in executor/nodeHash.c */
diff --git a/src/backend/utils/init/miscinit.c b/src/backend/utils/init/miscinit.c
index 65d8cbfaed58..545d1e90fbd4 100644
--- a/src/backend/utils/init/miscinit.c
+++ b/src/backend/utils/init/miscinit.c
@@ -1099,7 +1099,8 @@ EstimateClientConnectionInfoSpace(void)
* Serialize MyClientConnectionInfo for use by parallel workers.
*/
void
-SerializeClientConnectionInfo(Size maxsize, char *start_address)
+SerializeClientConnectionInfo(Size maxsize PG_USED_FOR_ASSERTS_ONLY,
+ char *start_address)
{
SerializedClientConnectionInfo serialized = {0};
--
2.50.0
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