From: Nathan Bossart <nathandbossart@gmail.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Chao Li <li.evan.chao@gmail.com>
Cc: Rahila Syed <rahilasyed90@gmail.com>
Cc: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgindent versus struct members and typedefs
Date: Tue, 5 May 2026 16:47:07 -0500
Message-ID: <afplW8DvRONDc1yl@nathan> (raw)
In-Reply-To: <509793.1764715875@sss.pgh.pa.us>
References: <aS9hkwnkWf3dZIA_@nathan>
<347EA055-D626-414C-B10D-7E70041B9B55@gmail.com>
<509793.1764715875@sss.pgh.pa.us>
On Tue, Dec 02, 2025 at 05:51:15PM -0500, Tom Lane wrote:
> Chao Li <li.evan.chao@gmail.com> writes:
>> On Dec 3, 2025, at 06:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
>>> I tried to fix pgindent for a few, but the code is basically impenetrable.
>>> I didn't find any fixes upstream [0], either. As noted above, we could
>>> also fix it by avoiding the naming conflicts. However, I can't imagine
>>> that's worth the churn, and I've already spent way too much time on this,
>>> so IMHO the best thing to do here is nothing.
>
>> I think that’s fine.
>
> Agreed, not worth the trouble to fool with.
For fun, I spent some time with an AI tool to develop the attached fix for
this problem. The explanation seems reasonable to me, although I am by no
means a pgindent expert. When I looked at this in December, I did find
this similar commit from upstream [0], but I failed to make the connection
with last_u_d. 0002 is the result of a pgindent run after applying 0001.
You'll notice that it fixes the exact set of cases I found with grep
upthread.
[0] https://github.com/pstef/freebsd_indent/commit/afa2239
--
nathan
From 804bb1097a44cc35178f3e1de46d17422ce7b67f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 5 May 2026 16:03:13 -0500
Subject: [PATCH v1 1/2] pgindent: Fix spacing after != when member name
matches typedef.
When a struct member name matches a registered typedef, pgindent
removes the space after "!=" (and some other operators):
entry->dsh.dsa_handle !=DSA_HANDLE_INVALID
In lexi.c, when an identifier matches the typedef list, the code
sets last_u_d = true before jumping to found_typename. This flag
tells the lexer that the next operator should be treated as unary,
which is correct for type names (e.g., "Datum *x" needs * to be
unary, not multiplication).
However, found_typename has a guard that recognizes when a typedef
name appears after "." or "->": in that case, the name is a struct
member, not a type, so it's returned as a plain identifier. The
problem is that last_u_d has already been set to true and is never
corrected. This causes the next operator (e.g., "!=") to be
misclassified as unary, which suppresses the space after it.
To fix, only set last_u_d when the token won't be caught by the
guard, i.e., when it will actually be treated as a type name.
This also explains why "==" was not affected: the "==" case always
returns binary_op regardless of last_u_d, while "!=" checks
last_u_d to decide.
---
src/tools/pg_bsd_indent/lexi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/tools/pg_bsd_indent/lexi.c b/src/tools/pg_bsd_indent/lexi.cindex 943bf7ce6b0..e846188d6f4 100644--- a/src/tools/pg_bsd_indent/lexi.c+++ b/src/tools/pg_bsd_indent/lexi.c@@ -363,7 +363,8 @@ lexi(struct parser_state *state)
bsearch(s_token, typenames, typename_top + 1,
sizeof(typenames[0]), strcmp_type))) {
state->keyword = 4; /* a type name */
- state->last_u_d = true;+ if (state->last_token != period && state->last_token != unary_op)+ state->last_u_d = true;
goto found_typename;
}
} else { /* we have a keyword */
--
2.50.1 (Apple Git-155)
From ccdde305aad8277bf91cf02577c723a008919288 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 5 May 2026 16:04:14 -0500
Subject: [PATCH v1 2/2] run pgindent
---
src/backend/replication/logical/logicalfuncs.c | 2 +-
src/backend/storage/ipc/dsm_registry.c | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.cindex 512013b0ef0..71fbaf72269 100644--- a/src/backend/replication/logical/logicalfuncs.c+++ b/src/backend/replication/logical/logicalfuncs.c@@ -218,7 +218,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
* what we need.
*/
if (!binary &&
- ctx->options.output_type !=OUTPUT_PLUGIN_TEXTUAL_OUTPUT)+ ctx->options.output_type != OUTPUT_PLUGIN_TEXTUAL_OUTPUT)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("logical decoding output plugin \"%s\" produces binary output, but function \"%s\" expects textual data",
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.cindex 2b56977659b..b9961c26019 100644--- a/src/backend/storage/ipc/dsm_registry.c+++ b/src/backend/storage/ipc/dsm_registry.c@@ -479,7 +479,7 @@ pg_get_dsm_registry_allocations(PG_FUNCTION_ARGS)
entry->dsa.handle != DSA_HANDLE_INVALID)
vals[2] = Int64GetDatum(dsa_get_total_size_from_handle(entry->dsa.handle));
else if (entry->type == DSMR_ENTRY_TYPE_DSH &&
- entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)+ entry->dsh.dsa_handle != DSA_HANDLE_INVALID)
vals[2] = Int64GetDatum(dsa_get_total_size_from_handle(entry->dsh.dsa_handle));
else
nulls[2] = true;
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.cindex c1a4672aa6f..80dc3bbc8da 100644--- a/src/bin/pg_basebackup/pg_basebackup.c+++ b/src/bin/pg_basebackup/pg_basebackup.c@@ -1282,7 +1282,7 @@ ReceiveArchiveStream(PGconn *conn, pg_compress_specification *compress)
ReceiveCopyData(conn, ReceiveArchiveStreamChunk, &state);
/* If we wrote the backup manifest to a file, close the file. */
- if (state.manifest_file !=NULL)+ if (state.manifest_file != NULL)
{
fclose(state.manifest_file);
state.manifest_file = NULL;
@@ -1341,7 +1341,7 @@ ReceiveArchiveStreamChunk(size_t r, char *copybuf, void *callback_data)
/* Sanity check. */
if (state->manifest_buffer != NULL ||
- state->manifest_file !=NULL)+ state->manifest_file != NULL)
pg_fatal("archives must precede manifest");
/* Parse the rest of the CopyData message. */
@@ -1406,7 +1406,7 @@ ReceiveArchiveStreamChunk(size_t r, char *copybuf, void *callback_data)
appendPQExpBuffer(state->manifest_buffer, copybuf + 1,
r - 1);
}
- else if (state->manifest_file !=NULL)+ else if (state->manifest_file != NULL)
{
/* Manifest data, write to disk. */
if (fwrite(copybuf + 1, r - 1, 1,
--
2.50.1 (Apple Git-155)
Attachments:
[text/plain] v1-0001-pgindent-Fix-spacing-after-when-member-name-match.patch (2.0K, ../afplW8DvRONDc1yl@nathan/2-v1-0001-pgindent-Fix-spacing-after-when-member-name-match.patch)
download | inline diff:
From 804bb1097a44cc35178f3e1de46d17422ce7b67f Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 5 May 2026 16:03:13 -0500
Subject: [PATCH v1 1/2] pgindent: Fix spacing after != when member name
matches typedef.
When a struct member name matches a registered typedef, pgindent
removes the space after "!=" (and some other operators):
entry->dsh.dsa_handle !=DSA_HANDLE_INVALID
In lexi.c, when an identifier matches the typedef list, the code
sets last_u_d = true before jumping to found_typename. This flag
tells the lexer that the next operator should be treated as unary,
which is correct for type names (e.g., "Datum *x" needs * to be
unary, not multiplication).
However, found_typename has a guard that recognizes when a typedef
name appears after "." or "->": in that case, the name is a struct
member, not a type, so it's returned as a plain identifier. The
problem is that last_u_d has already been set to true and is never
corrected. This causes the next operator (e.g., "!=") to be
misclassified as unary, which suppresses the space after it.
To fix, only set last_u_d when the token won't be caught by the
guard, i.e., when it will actually be treated as a type name.
This also explains why "==" was not affected: the "==" case always
returns binary_op regardless of last_u_d, while "!=" checks
last_u_d to decide.
---
src/tools/pg_bsd_indent/lexi.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/tools/pg_bsd_indent/lexi.c b/src/tools/pg_bsd_indent/lexi.cindex 943bf7ce6b0..e846188d6f4 100644--- a/src/tools/pg_bsd_indent/lexi.c+++ b/src/tools/pg_bsd_indent/lexi.c@@ -363,7 +363,8 @@ lexi(struct parser_state *state)
bsearch(s_token, typenames, typename_top + 1,
sizeof(typenames[0]), strcmp_type))) {
state->keyword = 4; /* a type name */
- state->last_u_d = true;+ if (state->last_token != period && state->last_token != unary_op)+ state->last_u_d = true;
goto found_typename;
}
} else { /* we have a keyword */
--
2.50.1 (Apple Git-155)
[text/plain] v1-0002-run-pgindent.patch (3.0K, ../afplW8DvRONDc1yl@nathan/3-v1-0002-run-pgindent.patch)
download | inline diff:
From ccdde305aad8277bf91cf02577c723a008919288 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Tue, 5 May 2026 16:04:14 -0500
Subject: [PATCH v1 2/2] run pgindent
---
src/backend/replication/logical/logicalfuncs.c | 2 +-
src/backend/storage/ipc/dsm_registry.c | 2 +-
src/bin/pg_basebackup/pg_basebackup.c | 6 +++---
3 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/replication/logical/logicalfuncs.c b/src/backend/replication/logical/logicalfuncs.cindex 512013b0ef0..71fbaf72269 100644--- a/src/backend/replication/logical/logicalfuncs.c+++ b/src/backend/replication/logical/logicalfuncs.c@@ -218,7 +218,7 @@ pg_logical_slot_get_changes_guts(FunctionCallInfo fcinfo, bool confirm, bool bin
* what we need.
*/
if (!binary &&
- ctx->options.output_type !=OUTPUT_PLUGIN_TEXTUAL_OUTPUT)+ ctx->options.output_type != OUTPUT_PLUGIN_TEXTUAL_OUTPUT)
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
errmsg("logical decoding output plugin \"%s\" produces binary output, but function \"%s\" expects textual data",
diff --git a/src/backend/storage/ipc/dsm_registry.c b/src/backend/storage/ipc/dsm_registry.cindex 2b56977659b..b9961c26019 100644--- a/src/backend/storage/ipc/dsm_registry.c+++ b/src/backend/storage/ipc/dsm_registry.c@@ -479,7 +479,7 @@ pg_get_dsm_registry_allocations(PG_FUNCTION_ARGS)
entry->dsa.handle != DSA_HANDLE_INVALID)
vals[2] = Int64GetDatum(dsa_get_total_size_from_handle(entry->dsa.handle));
else if (entry->type == DSMR_ENTRY_TYPE_DSH &&
- entry->dsh.dsa_handle !=DSA_HANDLE_INVALID)+ entry->dsh.dsa_handle != DSA_HANDLE_INVALID)
vals[2] = Int64GetDatum(dsa_get_total_size_from_handle(entry->dsh.dsa_handle));
else
nulls[2] = true;
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.cindex c1a4672aa6f..80dc3bbc8da 100644--- a/src/bin/pg_basebackup/pg_basebackup.c+++ b/src/bin/pg_basebackup/pg_basebackup.c@@ -1282,7 +1282,7 @@ ReceiveArchiveStream(PGconn *conn, pg_compress_specification *compress)
ReceiveCopyData(conn, ReceiveArchiveStreamChunk, &state);
/* If we wrote the backup manifest to a file, close the file. */
- if (state.manifest_file !=NULL)+ if (state.manifest_file != NULL)
{
fclose(state.manifest_file);
state.manifest_file = NULL;
@@ -1341,7 +1341,7 @@ ReceiveArchiveStreamChunk(size_t r, char *copybuf, void *callback_data)
/* Sanity check. */
if (state->manifest_buffer != NULL ||
- state->manifest_file !=NULL)+ state->manifest_file != NULL)
pg_fatal("archives must precede manifest");
/* Parse the rest of the CopyData message. */
@@ -1406,7 +1406,7 @@ ReceiveArchiveStreamChunk(size_t r, char *copybuf, void *callback_data)
appendPQExpBuffer(state->manifest_buffer, copybuf + 1,
r - 1);
}
- else if (state->manifest_file !=NULL)+ else if (state->manifest_file != NULL)
{
/* Manifest data, write to disk. */
if (fwrite(copybuf + 1, r - 1, 1,
--
2.50.1 (Apple Git-155)
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: nathandbossart@gmail.com, tgl@sss.pgh.pa.us, li.evan.chao@gmail.com, rahilasyed90@gmail.com, robertmhaas@gmail.com, pgsql-hackers@lists.postgresql.org
Subject: Re: pgindent versus struct members and typedefs
In-Reply-To: <afplW8DvRONDc1yl@nathan>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox