Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Peter Eisentraut  writes:
> On 16.05.24 16:45, Tom Lane wrote:
>> Yeah, that was bothering me too, but I went for the minimum delta.
>> I did think that a couple of integer macros would be a better idea,
>> so +1 for what you did here.

> I committed this, and Michael took care of WaitEventExtension, so we 
> should be all clear here.

Thanks.  I just made the committed typedefs.list exactly match the
current buildfarm output, so we're clean for now.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Peter Eisentraut]

On 16.05.24 16:45, Tom Lane wrote:

Peter Eisentraut  writes:

In these cases, I think for
NotificationHash
ResourceOwnerData
WalSyncMethod
we can just get rid of the typedef.


I have no objection to dealing with NotificationHash as you have here.


ReadBuffersFlags shouldn't be an enum at all, because its values are
used as flag bits.


Yeah, that was bothering me too, but I went for the minimum delta.
I did think that a couple of integer macros would be a better idea,
so +1 for what you did here.


I committed this, and Michael took care of WaitEventExtension, so we 
should be all clear here.

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Michael Paquier]

On Fri, May 17, 2024 at 10:24:57AM +0900, Michael Paquier wrote:
> Yep.  I can handle that in 2~3 hours.

And done with 110eb4aefbad.  If there's anything else, feel free to
let me know.
--
Michael


signature.asc
Description: PGP signature

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Michael Paquier]

On Thu, May 16, 2024 at 09:09:36PM -0400, Tom Lane wrote:
> WFM, and this is probably a place where we don't want to change the
> API in v17 and again in v18, so I agree with pushing now.
>
> Reminder though: beta1 release freeze begins Saturday.
> Not many hours left.

Yep.  I can handle that in 2~3 hours.
--
Michael


signature.asc
Description: PGP signature

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Michael Paquier  writes:
> On Thu, May 16, 2024 at 10:45:18AM -0400, Tom Lane wrote:
>> ... I think the enum should be nuked altogether, but
>> it's a bit late to be redesigning that for v17 perhaps.

> You're right, WaitEventExtension is better gone.  The only thing that
> matters is that we want to start computing the IDs assigned to the
> custom wait events for extensions with a number higher than the
> existing WAIT_EXTENSION to avoid interferences in pg_stat_activity, so
> this could be cleaned up as the attached.

WFM, and this is probably a place where we don't want to change the
API in v17 and again in v18, so I agree with pushing now.

Reminder though: beta1 release freeze begins Saturday.
Not many hours left.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Michael Paquier]

On Thu, May 16, 2024 at 10:45:18AM -0400, Tom Lane wrote:
> I am also quite confused by that.  It seems to be kind of an enum
> that is supposed to be extended at runtime, meaning that neither
> of the existing enum member values ought to be used as such, although
> either autoprewarm.c didn't get the memo or I misunderstand the
> intended usage.  NUM_BUILTIN_WAIT_EVENT_EXTENSION is possibly the
> most bizarre idea I've ever seen: what would a "built-in extension"
> event be exactly?  I think the enum should be nuked altogether, but
> it's a bit late to be redesigning that for v17 perhaps.

You're right, WaitEventExtension is better gone.  The only thing that
matters is that we want to start computing the IDs assigned to the
custom wait events for extensions with a number higher than the
existing WAIT_EXTENSION to avoid interferences in pg_stat_activity, so
this could be cleaned up as the attached.

The reason why autoprewarm.c does not have a custom wait event
assigned is that it does not make sense there: this would not show up
in pg_stat_activity.  I think that we should just switch back to
PG_WAIT_EXTENSION there and call it a day.

I can still clean up that in time for beta1, as in today's time.  But
that can wait, as well.  Thoughts?
--
Michael
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 080e92d1cf..1b735d4a0e 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -53,12 +53,6 @@ extern PGDLLIMPORT uint32 *my_wait_event_info;
  *
  * The ID retrieved can be used with pgstat_report_wait_start() or equivalent.
  */
-typedef enum
-{
-	WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
-	WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED,
-} WaitEventExtension;
-
 extern void WaitEventExtensionShmemInit(void);
 extern Size WaitEventExtensionShmemSize(void);
 
diff --git a/src/backend/utils/activity/wait_event.c b/src/backend/utils/activity/wait_event.c
index 4ffcb10c8b..084a9dfdc2 100644
--- a/src/backend/utils/activity/wait_event.c
+++ b/src/backend/utils/activity/wait_event.c
@@ -89,8 +89,7 @@ typedef struct WaitEventExtensionCounterData
 static WaitEventExtensionCounterData *WaitEventExtensionCounter;
 
 /* first event ID of custom wait events for extensions */
-#define NUM_BUILTIN_WAIT_EVENT_EXTENSION	\
-	(WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - WAIT_EVENT_EXTENSION)
+#define WAIT_EVENT_EXTENSION_INITIAL_ID	1
 
 /* wait event info for extensions */
 #define WAIT_EVENT_EXTENSION_INFO(eventId)	(PG_WAIT_EXTENSION | eventId)
@@ -129,7 +128,7 @@ WaitEventExtensionShmemInit(void)
 	if (!found)
 	{
 		/* initialize the allocation counter and its spinlock. */
-		WaitEventExtensionCounter->nextId = NUM_BUILTIN_WAIT_EVENT_EXTENSION;
+		WaitEventExtensionCounter->nextId = WAIT_EVENT_EXTENSION_INITIAL_ID;
 		SpinLockInit(>mutex);
 	}
 
@@ -244,7 +243,7 @@ GetWaitEventExtensionIdentifier(uint16 eventId)
 	WaitEventExtensionEntryById *entry;
 
 	/* Built-in event? */
-	if (eventId < NUM_BUILTIN_WAIT_EVENT_EXTENSION)
+	if (eventId < WAIT_EVENT_EXTENSION_INITIAL_ID)
 		return "Extension";
 
 	/* It is a user-defined wait event, so lookup hash table. */
diff --git a/contrib/pg_prewarm/autoprewarm.c b/contrib/pg_prewarm/autoprewarm.c
index 248b9914a3..1c8804dc43 100644
--- a/contrib/pg_prewarm/autoprewarm.c
+++ b/contrib/pg_prewarm/autoprewarm.c
@@ -226,7 +226,7 @@ autoprewarm_main(Datum main_arg)
 			(void) WaitLatch(MyLatch,
 			 WL_LATCH_SET | WL_EXIT_ON_PM_DEATH,
 			 -1L,
-			 WAIT_EVENT_EXTENSION);
+			 PG_WAIT_EXTENSION);
 		}
 		else
 		{
@@ -253,7 +253,7 @@ autoprewarm_main(Datum main_arg)
 			(void) WaitLatch(MyLatch,
 			 WL_LATCH_SET | WL_TIMEOUT | WL_EXIT_ON_PM_DEATH,
 			 delay_in_ms,
-			 WAIT_EVENT_EXTENSION);
+			 PG_WAIT_EXTENSION);
 		}
 
 		/* Reset the latch, loop. */


signature.asc
Description: PGP signature

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Peter Eisentraut  writes:
> In these cases, I think for
> NotificationHash
> ResourceOwnerData
> WalSyncMethod
> we can just get rid of the typedef.

I have no objection to dealing with NotificationHash as you have here.

> ReadBuffersFlags shouldn't be an enum at all, because its values are 
> used as flag bits.

Yeah, that was bothering me too, but I went for the minimum delta.
I did think that a couple of integer macros would be a better idea,
so +1 for what you did here.

> WaitEventExtension, I'm not sure, it's like, an extensible enum?  I 
> guess let's remove the typedef there, too.

I am also quite confused by that.  It seems to be kind of an enum
that is supposed to be extended at runtime, meaning that neither
of the existing enum member values ought to be used as such, although
either autoprewarm.c didn't get the memo or I misunderstand the
intended usage.  NUM_BUILTIN_WAIT_EVENT_EXTENSION is possibly the
most bizarre idea I've ever seen: what would a "built-in extension"
event be exactly?  I think the enum should be nuked altogether, but
it's a bit late to be redesigning that for v17 perhaps.

> Attached is a variant patch.

I'm good with this, with a mental note to look again at
WaitEventExtension later.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Peter Eisentraut]

On 16.05.24 01:32, Tom Lane wrote:

As for the remainder, they aren't showing up because no variable
or field is declared using them, which means no debug symbol
table entry is made for them.  This means we could just drop those
typedefs and be little the worse off notationally.  I experimented
with a patch for that, as attached.  (In the case of NotificationHash,
I thought it better to arrange for there to be a suitable variable;
but it could certainly be done the other way too.)  Is this too anal?


I agree we should get rid of these.

Over the last release cycle, I've been leaning a bit more toward not 
typdef'ing enums and structs that are only in local use, in part because 
of the implied need to keep the typedefs list up to date.


In these cases, I think for

NotificationHash
ResourceOwnerData
WalSyncMethod

we can just get rid of the typedef.

ReadBuffersFlags shouldn't be an enum at all, because its values are 
used as flag bits.


WaitEventExtension, I'm not sure, it's like, an extensible enum?  I 
guess let's remove the typedef there, too.


Attached is a variant patch.
diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index d0891e3f0e0..ab4c72762d8 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -396,10 +396,10 @@ typedef struct NotificationList
 
 #define MIN_HASHABLE_NOTIFIES 16   /* threshold to build hashtab */
 
-typedef struct NotificationHash
+struct NotificationHash
 {
Notification *event;/* => the actual Notification struct */
-} NotificationHash;
+};
 
 static NotificationList *pendingNotifies = NULL;
 
@@ -2299,7 +2299,7 @@ AddEventToPendingNotifies(Notification *n)
 
/* Create the hash table */
hash_ctl.keysize = sizeof(Notification *);
-   hash_ctl.entrysize = sizeof(NotificationHash);
+   hash_ctl.entrysize = sizeof(struct NotificationHash);
hash_ctl.hash = notification_hash;
hash_ctl.match = notification_match;
hash_ctl.hcxt = CurTransactionContext;
diff --git a/src/backend/utils/resowner/resowner.c 
b/src/backend/utils/resowner/resowner.c
index ab9343bc5cf..505534ee8d3 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -107,7 +107,7 @@ 
StaticAssertDecl(RESOWNER_HASH_MAX_ITEMS(RESOWNER_HASH_INIT_SIZE) >= RESOWNER_AR
 /*
  * ResourceOwner objects look like this
  */
-typedef struct ResourceOwnerData
+struct ResourceOwnerData
 {
ResourceOwner parent;   /* NULL if no parent (toplevel owner) */
ResourceOwner firstchild;   /* head of linked list of children */
@@ -155,7 +155,7 @@ typedef struct ResourceOwnerData
 
/* The local locks cache. */
LOCALLOCK  *locks[MAX_RESOWNER_LOCKS];  /* list of owned locks */
-} ResourceOwnerData;
+};
 
 
 /*
@@ -415,7 +415,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name)
ResourceOwner owner;
 
owner = (ResourceOwner) MemoryContextAllocZero(TopMemoryContext,
-   
   sizeof(ResourceOwnerData));
+   
   sizeof(struct ResourceOwnerData));
owner->name = name;
 
if (parent)
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 76787a82673..1a1f11a943f 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -19,14 +19,14 @@
 
 
 /* Sync methods */
-typedef enum WalSyncMethod
+enum WalSyncMethod
 {
WAL_SYNC_METHOD_FSYNC = 0,
WAL_SYNC_METHOD_FDATASYNC,
WAL_SYNC_METHOD_OPEN,   /* for O_SYNC */
WAL_SYNC_METHOD_FSYNC_WRITETHROUGH,
WAL_SYNC_METHOD_OPEN_DSYNC  /* for O_DSYNC */
-} WalSyncMethod;
+};
 extern PGDLLIMPORT int wal_sync_method;
 
 extern PGDLLIMPORT XLogRecPtr ProcLastRecPtr;
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 42211bfec4f..08364447c74 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -107,14 +107,10 @@ typedef struct BufferManagerRelation
 #define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
 #define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = 
p_smgr, .relpersistence = p_relpersistence})
 
-typedef enum ReadBuffersFlags
-{
-   /* Zero out page if reading fails. */
-   READ_BUFFERS_ZERO_ON_ERROR = (1 << 0),
-
-   /* Call smgrprefetch() if I/O necessary. */
-   READ_BUFFERS_ISSUE_ADVICE = (1 << 1),
-} ReadBuffersFlags;
+/* Zero out page if reading fails. */
+#define READ_BUFFERS_ZERO_ON_ERROR (1 << 0)
+/* Call smgrprefetch() if I/O necessary. */
+#define READ_BUFFERS_ISSUE_ADVICE (1 << 1)
 
 struct ReadBuffersOperation
 {

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

I wrote:
> This works for me.  One point that could stand discussion while we're
> here is whether the once-a-cycle run should use the verbatim buildfarm
> results or it's okay to editorialize on that typedefs list.  I did a
> little of the latter in da256a4a7, and I feel like we should either
> bless that practice in this document or decide that it was a bad idea.

> For reference, what I added to the buildfarm's list was

> +InjectionPointCacheEntry
> +InjectionPointCondition
> +InjectionPointConditionType
> +InjectionPointEntry
> +InjectionPointSharedState
> +NotificationHash
> +ReadBuffersFlags
> +ResourceOwnerData
> +WaitEventExtension
> +WalSyncMethod

I realized that the reason the InjectionPoint typedefs were missing
is that none of the buildfarm animals that contribute typedefs are
building with --enable-injection-points.  I rectified that on sifaka,
and now those are in the list available from the buildfarm.

As for the remainder, they aren't showing up because no variable
or field is declared using them, which means no debug symbol
table entry is made for them.  This means we could just drop those
typedefs and be little the worse off notationally.  I experimented
with a patch for that, as attached.  (In the case of NotificationHash,
I thought it better to arrange for there to be a suitable variable;
but it could certainly be done the other way too.)  Is this too anal?

regards, tom lane

diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index d0891e3f0e..6861f028d2 100644
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -2253,10 +2253,13 @@ AsyncExistsPendingNotify(Notification *n)
 	if (pendingNotifies->hashtab != NULL)
 	{
 		/* Use the hash table to probe for a match */
-		if (hash_search(pendingNotifies->hashtab,
-		,
-		HASH_FIND,
-		NULL))
+		NotificationHash *ent;
+
+		ent = hash_search(pendingNotifies->hashtab,
+		  ,
+		  HASH_FIND,
+		  NULL);
+		if (ent)
 			return true;
 	}
 	else
diff --git a/src/backend/utils/resowner/resowner.c b/src/backend/utils/resowner/resowner.c
index ab9343bc5c..3bde0eba4d 100644
--- a/src/backend/utils/resowner/resowner.c
+++ b/src/backend/utils/resowner/resowner.c
@@ -107,7 +107,7 @@ StaticAssertDecl(RESOWNER_HASH_MAX_ITEMS(RESOWNER_HASH_INIT_SIZE) >= RESOWNER_AR
 /*
  * ResourceOwner objects look like this
  */
-typedef struct ResourceOwnerData
+struct ResourceOwnerData
 {
 	ResourceOwner parent;		/* NULL if no parent (toplevel owner) */
 	ResourceOwner firstchild;	/* head of linked list of children */
@@ -155,7 +155,7 @@ typedef struct ResourceOwnerData
 
 	/* The local locks cache. */
 	LOCALLOCK  *locks[MAX_RESOWNER_LOCKS];	/* list of owned locks */
-} ResourceOwnerData;
+};
 
 
 /*
@@ -415,7 +415,7 @@ ResourceOwnerCreate(ResourceOwner parent, const char *name)
 	ResourceOwner owner;
 
 	owner = (ResourceOwner) MemoryContextAllocZero(TopMemoryContext,
-   sizeof(ResourceOwnerData));
+   sizeof(*owner));
 	owner->name = name;
 
 	if (parent)
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 76787a8267..1a1f11a943 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -19,14 +19,14 @@
 
 
 /* Sync methods */
-typedef enum WalSyncMethod
+enum WalSyncMethod
 {
 	WAL_SYNC_METHOD_FSYNC = 0,
 	WAL_SYNC_METHOD_FDATASYNC,
 	WAL_SYNC_METHOD_OPEN,		/* for O_SYNC */
 	WAL_SYNC_METHOD_FSYNC_WRITETHROUGH,
 	WAL_SYNC_METHOD_OPEN_DSYNC	/* for O_DSYNC */
-} WalSyncMethod;
+};
 extern PGDLLIMPORT int wal_sync_method;
 
 extern PGDLLIMPORT XLogRecPtr ProcLastRecPtr;
diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h
index 42211bfec4..edb7011743 100644
--- a/src/include/storage/bufmgr.h
+++ b/src/include/storage/bufmgr.h
@@ -107,14 +107,14 @@ typedef struct BufferManagerRelation
 #define BMR_REL(p_rel) ((BufferManagerRelation){.rel = p_rel})
 #define BMR_SMGR(p_smgr, p_relpersistence) ((BufferManagerRelation){.smgr = p_smgr, .relpersistence = p_relpersistence})
 
-typedef enum ReadBuffersFlags
+enum ReadBuffersFlags
 {
 	/* Zero out page if reading fails. */
 	READ_BUFFERS_ZERO_ON_ERROR = (1 << 0),
 
 	/* Call smgrprefetch() if I/O necessary. */
 	READ_BUFFERS_ISSUE_ADVICE = (1 << 1),
-} ReadBuffersFlags;
+};
 
 struct ReadBuffersOperation
 {
diff --git a/src/include/utils/wait_event.h b/src/include/utils/wait_event.h
index 080e92d1cf..72c4d60930 100644
--- a/src/include/utils/wait_event.h
+++ b/src/include/utils/wait_event.h
@@ -53,11 +53,11 @@ extern PGDLLIMPORT uint32 *my_wait_event_info;
  *
  * The ID retrieved can be used with pgstat_report_wait_start() or equivalent.
  */
-typedef enum
+enum WaitEventExtension
 {
 	WAIT_EVENT_EXTENSION = PG_WAIT_EXTENSION,
 	WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED,
-} WaitEventExtension;
+};
 
 extern void WaitEventExtensionShmemInit(void);
 extern Size 

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

On Wed, May 15, 2024 at 04:52:19PM -0400, Robert Haas wrote:
> On Wed, May 15, 2024 at 4:50 PM Tom Lane  wrote:
>> At this point my OCD got the better of me and I did a little
>> additional wordsmithing.  How about the attached?
> 
> No objections here.

+1

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Robert Haas]

On Wed, May 15, 2024 at 4:50 PM Tom Lane  wrote:
> At this point my OCD got the better of me and I did a little
> additional wordsmithing.  How about the attached?

No objections here.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Nathan Bossart  writes:
> On Wed, May 15, 2024 at 04:07:18PM -0400, Robert Haas wrote:
>> How's this?

> I compared this with my v1, and the only bit of information there that I
> see missing in v3 is that validation step 4 only applies in the
> once-per-cycle run (or if you forget to pgindent before committing a
> patch).  This might be why I was struggling to untangle the two types of
> pgindent runs in my attempt.  Perhaps it's worth adding a note to that step
> about when it is required.

Oh ... another problem is that the VALIDATION steps really apply to
both kinds of indent runs, but it's not clear to me that that's
obvious in v3.  Maybe the steps should be rearranged to be
(1) base case, (2) VALIDATION, (3) ONCE PER CYCLE.

At this point my OCD got the better of me and I did a little
additional wordsmithing.  How about the attached?

regards, tom lane

diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README
index b649a21f59..369f120eb0 100644
--- a/src/tools/pgindent/README
+++ b/src/tools/pgindent/README
@@ -1,8 +1,9 @@
 pgindent'ing the PostgreSQL source tree
 ===
 
-We run this process at least once in each development cycle,
-to maintain uniform layout style in our C and Perl code.
+pgindent is used to maintain uniform layout style in our C and Perl code,
+and should be run for every commit. There are additional code beautification
+tasks which should be performed at least once per release cycle.
 
 You might find this blog post interesting:
 http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html
@@ -25,45 +26,31 @@ PREREQUISITES:
Or if you have cpanm installed, you can just use:
cpanm https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
 
-DOING THE INDENT RUN:
 
-1) Change directory to the top of the source tree.
-
-2) Download the latest typedef file from the buildfarm:
-
-	wget -O src/tools/pgindent/typedefs.list https://buildfarm.postgresql.org/cgi-bin/typedefs.pl
+DOING THE INDENT RUN BEFORE A NORMAL COMMIT:
 
-   (See https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list for a full
-   list of typedef files, if you want to indent some back branch.)
+1) Change directory to the top of the source tree.
 
-3) Run pgindent on the C files:
+2) Run pgindent on the C files:
 
 	src/tools/pgindent/pgindent .
 
If any files generate errors, restore their original versions with
"git checkout", and see below for cleanup ideas.
 
-4) Indent the Perl code using perltidy:
-
-	src/tools/pgindent/pgperltidy .
-
-   If you want to use some perltidy version that's not in your PATH,
-   first set the PERLTIDY environment variable to point to it.
-
-5) Reformat the bootstrap catalog data files:
-
-	./configure # "make" will not work in an unconfigured tree
-	cd src/include/catalog
-	make reformat-dat-files
-	cd ../../..
-
-VALIDATION:
-
-1) Check for any newly-created files using "git status"; there shouldn't
+3) Check for any newly-created files using "git status"; there shouldn't
be any.  (pgindent leaves *.BAK files behind if it has trouble, while
perltidy leaves *.LOG files behind.)
 
-2) Do a full test build:
+4) If pgindent wants to change anything your commit wasn't touching,
+   stop and figure out why.  If it is making ugly whitespace changes
+   around typedefs your commit adds, you need to add those typedefs
+   to src/tools/pgindent/typedefs.list.
+
+5) If you have the patience, it's worth eyeballing the "git diff" output
+   for any egregiously ugly changes.  See below for cleanup ideas.
+
+6) Do a full test build:
 
 	make -s clean
 	make -s all	# look for unexpected warnings, and errors of course
@@ -75,14 +62,38 @@ VALIDATION:
header files that get copied into ecpg output; if so, adjust the
expected-files to match.
 
-3) If you have the patience, it's worth eyeballing the "git diff" output
-   for any egregiously ugly changes.  See below for cleanup ideas.
 
+AT LEAST ONCE PER RELEASE CYCLE:
+
+1) Download the latest typedef file from the buildfarm:
+
+	wget -O src/tools/pgindent/typedefs.list https://buildfarm.postgresql.org/cgi-bin/typedefs.pl
+
+   This step resolves any differences between the incrementally updated
+   version of the file and a clean, autogenerated one.
+   (See https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list for a full
+   list of typedef files, if you want to indent some back branch.)
+
+2) Run pgindent as above.
+
+3) Indent the Perl code using perltidy:
+
+	src/tools/pgindent/pgperltidy .
+
+   If you want to use some perltidy version that's not in your PATH,
+   first set the PERLTIDY environment variable to point to it.
+
+4) Reformat the bootstrap catalog data files:
+
+	./configure # "make" will not work in an unconfigured tree
+	cd src/include/catalog
+	make reformat-dat-files
+	cd ../../..
 
-When you're done, "git commit" everything including the typedefs.list file
-you used.
+5) 

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

On Wed, May 15, 2024 at 04:07:18PM -0400, Robert Haas wrote:
> On Wed, May 15, 2024 at 3:30 PM Nathan Bossart  
> wrote:
>> This is much cleaner, thanks.  The only thing that stands out to me is that
>> the "once per release cycle" section should probably say to do an indent
>> run after downloading the typedef file.
> 
> How's this?

I compared this with my v1, and the only bit of information there that I
see missing in v3 is that validation step 4 only applies in the
once-per-cycle run (or if you forget to pgindent before committing a
patch).  This might be why I was struggling to untangle the two types of
pgindent runs in my attempt.  Perhaps it's worth adding a note to that step
about when it is required.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Robert Haas  writes:
> On Wed, May 15, 2024 at 3:30 PM Nathan Bossart  
> wrote:
>> This is much cleaner, thanks.  The only thing that stands out to me is that
>> the "once per release cycle" section should probably say to do an indent
>> run after downloading the typedef file.

> How's this?

This works for me.  One point that could stand discussion while we're
here is whether the once-a-cycle run should use the verbatim buildfarm
results or it's okay to editorialize on that typedefs list.  I did a
little of the latter in da256a4a7, and I feel like we should either
bless that practice in this document or decide that it was a bad idea.

For reference, what I added to the buildfarm's list was

+InjectionPointCacheEntry
+InjectionPointCondition
+InjectionPointConditionType
+InjectionPointEntry
+InjectionPointSharedState
+NotificationHash
+ReadBuffersFlags
+ResourceOwnerData
+WaitEventExtension
+WalSyncMethod

I believe all of these must have been added manually during v17.
If I took any one of them out there was some visible disimprovement
in pgindent's results, so I kept them.  Was that the right decision?
If so we should explain it here.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Robert Haas]

On Wed, May 15, 2024 at 3:30 PM Nathan Bossart  wrote:
> On Wed, May 15, 2024 at 12:06:03PM -0400, Robert Haas wrote:
> > What jumps out at me when I read this patch is that it says that an
> > incremental run should do steps 1-3 of a complete run, and then
> > immediately backtracks and says not to do step 2, which seems a little
> > strange.
> >
> > I played around with this a bit and came up with the attached, which
> > takes a slightly different approach. Feel free to use, ignore, etc.
>
> This is much cleaner, thanks.  The only thing that stands out to me is that
> the "once per release cycle" section should probably say to do an indent
> run after downloading the typedef file.

How's this?

-- 
Robert Haas
EDB: http://www.enterprisedb.com


pgindent_readme_v3.patch
Description: Binary data

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

On Wed, May 15, 2024 at 12:06:03PM -0400, Robert Haas wrote:
> What jumps out at me when I read this patch is that it says that an
> incremental run should do steps 1-3 of a complete run, and then
> immediately backtracks and says not to do step 2, which seems a little
> strange.
> 
> I played around with this a bit and came up with the attached, which
> takes a slightly different approach. Feel free to use, ignore, etc.

This is much cleaner, thanks.  The only thing that stands out to me is that
the "once per release cycle" section should probably say to do an indent
run after downloading the typedef file.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Robert Haas]

On Tue, Apr 23, 2024 at 4:05 PM Nathan Bossart  wrote:
> Here is a first attempt.  I'm not tremendously happy with it, but it at
> least gets something on the page to build on.  I was originally going to
> copy/paste the relevant steps into the description of the incremental
> process, but that seemed kind-of silly, so I instead just pointed to the
> relevant steps of the "full" process, along with the deviations from those
> steps.  That's a little more work for the reader, but maybe it isn't too
> bad...  I plan to iterate on this patch some more.

What jumps out at me when I read this patch is that it says that an
incremental run should do steps 1-3 of a complete run, and then
immediately backtracks and says not to do step 2, which seems a little
strange.

I played around with this a bit and came up with the attached, which
takes a slightly different approach. Feel free to use, ignore, etc.

-- 
Robert Haas
EDB: http://www.enterprisedb.com


pgindent_readme_v2.patch
Description: Binary data

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Andrew Dunstan  writes:
> On 2024-04-24 We 06:12, Peter Eisentraut wrote:
>> Is the code to extract typedefs available somewhere independent of the 
>> buildfarm?  It would be useful sometimes to be able to run this 
>> locally, like before and after some patch, to keep the in-tree 
>> typedefs list updated.

> There's been talk about it but I don't think anyone's done it. I'd be 
> more than happy if the buildfarm client could just call something in the 
> core repo (c.f. src/test/perl/Postgres/Test/AdjustUpgrade.pm).

There is already src/tools/find_typedef, which looks like it might
be an ancestral version of the current buildfarm code (which is sub
find_typedefs in run_build.pl of the client distribution).  Perhaps
it'd be useful to bring that up to speed with the current BF code.

The main problem with this though is that a local run can only
give you the system-supplied typedefs for your own platform and
build options.  The value-add that the buildfarm brings is to
merge the results from several different platforms.

I suppose you could set up some merging process that would add
symbols from a local run to src/tools/pgindent/typedefs.list
but never remove any.  But that hardly removes the need for
an occasional cleanup pass.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Andrew Dunstan]

On 2024-04-24 We 06:12, Peter Eisentraut wrote:

On 22.04.24 22:28, Tom Lane wrote:

Nathan Bossart  writes:

On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote:

I think the actual plan now is that we'll sync the in-tree copy
with the buildfarm's results (and then do a tree-wide pgindent)
every so often, probably shortly before beta every year.
Okay.  Is this just to resolve the delta between the manual updates 
and a

clean autogenerated copy every once in a while?

The main reason there's a delta is that people don't manage to
maintain the in-tree copy perfectly (at least, they certainly
haven't done so for this past year).  So we need to do that
to clean up every now and then.

A secondary reason is that the set of typedefs we absorb from
system include files changes over time.


Is the code to extract typedefs available somewhere independent of the 
buildfarm?  It would be useful sometimes to be able to run this 
locally, like before and after some patch, to keep the in-tree 
typedefs list updated.






There's been talk about it but I don't think anyone's done it. I'd be 
more than happy if the buildfarm client could just call something in the 
core repo (c.f. src/test/perl/Postgres/Test/AdjustUpgrade.pm).


Regarding testing with your patch, some years ago I wrote this blog 
post: 




cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Peter Eisentraut]

On 22.04.24 22:28, Tom Lane wrote:

Nathan Bossart  writes:

On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote:

I think the actual plan now is that we'll sync the in-tree copy
with the buildfarm's results (and then do a tree-wide pgindent)
every so often, probably shortly before beta every year.

Okay.  Is this just to resolve the delta between the manual updates and a
clean autogenerated copy every once in a while?

The main reason there's a delta is that people don't manage to
maintain the in-tree copy perfectly (at least, they certainly
haven't done so for this past year).  So we need to do that
to clean up every now and then.

A secondary reason is that the set of typedefs we absorb from
system include files changes over time.


Is the code to extract typedefs available somewhere independent of the 
buildfarm?  It would be useful sometimes to be able to run this locally, 
like before and after some patch, to keep the in-tree typedefs list updated.

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Andrew Dunstan]

On 2024-04-23 Tu 06:23, Alvaro Herrera wrote:

But there are others: InjectionPointEntry, ResourceOwnerData,
JsonNonTerminal, JsonParserSem, ...



The last two are down to me. Let's just get rid of them like the 
attached (no need for a typedef at all)



cheers


andrew

--
Andrew Dunstan
EDB:https://www.enterprisedb.com
diff --git a/src/common/jsonapi.c b/src/common/jsonapi.c
index 12fabcaccf..fc0cb36974 100644
--- a/src/common/jsonapi.c
+++ b/src/common/jsonapi.c
@@ -50,16 +50,16 @@ typedef enum	/* contexts of JSON parser */
  * tokens, non-terminals, and semantic action markers.
  */
 
-typedef enum
+enum JsonNonTerminal
 {
 	JSON_NT_JSON = 32,
 	JSON_NT_ARRAY_ELEMENTS,
 	JSON_NT_MORE_ARRAY_ELEMENTS,
 	JSON_NT_KEY_PAIRS,
 	JSON_NT_MORE_KEY_PAIRS,
-} JsonNonTerminal;
+};
 
-typedef enum
+enum JsonParserSem
 {
 	JSON_SEM_OSTART = 64,
 	JSON_SEM_OEND,
@@ -72,7 +72,7 @@ typedef enum
 	JSON_SEM_AELEM_END,
 	JSON_SEM_SCALAR_INIT,
 	JSON_SEM_SCALAR_CALL,
-} JsonParserSem;
+};
 
 /*
  * struct containing the 3 stacks used in non-recursive parsing,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index d551ada325..ba48a3371d 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -1312,14 +1312,12 @@ JsonManifestParseIncrementalState
 JsonManifestParseState
 JsonManifestSemanticState
 JsonManifestWALRangeField
-JsonNonTerminal
 JsonObjectAgg
 JsonObjectConstructor
 JsonOutput
 JsonParseExpr
 JsonParseContext
 JsonParseErrorType
-JsonParserSem
 JsonParserStack
 JsonPath
 JsonPathBool

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

On Mon, Apr 22, 2024 at 03:20:10PM -0500, Nathan Bossart wrote:
> On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote:
>> The problem with the README is that it describes that process,
>> rather than the now-typical workflow of incrementally keeping
>> the tree indented.  I don't think we want to remove the info
>> about how to do the full-monty process, but you're right that
>> the README needs to explain the incremental method as being
>> the one most developers would usually use.
>> 
>> Want to write some text?
> 
> Yup, I'll put something together.

Here is a first attempt.  I'm not tremendously happy with it, but it at
least gets something on the page to build on.  I was originally going to
copy/paste the relevant steps into the description of the incremental
process, but that seemed kind-of silly, so I instead just pointed to the
relevant steps of the "full" process, along with the deviations from those
steps.  That's a little more work for the reader, but maybe it isn't too
bad...  I plan to iterate on this patch some more.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
diff --git a/src/tools/pgindent/README b/src/tools/pgindent/README
index b649a21f59..221e3fc010 100644
--- a/src/tools/pgindent/README
+++ b/src/tools/pgindent/README
@@ -1,8 +1,11 @@
 pgindent'ing the PostgreSQL source tree
 ===
 
-We run this process at least once in each development cycle,
-to maintain uniform layout style in our C and Perl code.
+The following process is used to maintain uniform layout style in our C and Perl
+code.  Once in each development cycle, a complete, independent run is performed.
+Additionally, an abridged run should be performed on every new commit to
+minimize deviations from pgindent's preferred style.  See below for more details
+on each type of run.
 
 You might find this blog post interesting:
 http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html
@@ -25,7 +28,20 @@ PREREQUISITES:
Or if you have cpanm installed, you can just use:
cpanm https://cpan.metacpan.org/authors/id/S/SH/SHANCOCK/Perl-Tidy-20230309.tar.gz
 
-DOING THE INDENT RUN:
+DOING AN INCREMENTAL, PER-COMMIT INDENT RUN:
+
+First, complete steps 1-3 of a complete run listed below.  Note that there is
+no need to download the typedef file from the buildfarm.  Standard practice is
+to manually update this file as needed in the same commit that adds or removes
+types.  The once-per-development-cycle run will resolve any differences between
+the incrementally updated version of the file and a clean, autogenerated one.
+
+Finally, complete steps 1-3 of the validation section below.  Ensure that any
+changes made by pgindent are included in the patch prior to committing.  If you
+forget to do so, add the missed pgindent changes in a follow-up commit and also
+do step 4 of the validation section below.
+
+DOING A COMPLETE, ONCE-PER-DEVELOPMENT-CYCLE INDENT RUN:
 
 1) Change directory to the top of the source tree.
 

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Alvaro Herrera  writes:
> On 2024-Apr-22, Tom Lane wrote:
>> The main reason there's a delta is that people don't manage to
>> maintain the in-tree copy perfectly (at least, they certainly
>> haven't done so for this past year).  So we need to do that
>> to clean up every now and then.

> Out of curiosity, I downloaded the buildfarm-generated file and
> re-indented the whole tree.  It turns out that most commits seem to have
> maintained the in-tree typedefs list correctly when adding entries (even
> if out of alphabetical order), but a few haven't; and some people have
> added entries that the buildfarm script does not detect.

Yeah.  I believe that happens when there is no C variable or field
anywhere that has that specific struct type.  In your example,
NotificationHash appears to only be referenced in a sizeof()
call, which suggests that maybe the coding is a bit squirrely
and could be done another way.

Having said that, there already are manually-curated lists of
inclusions and exclusions hard-wired into pgindent (see around
line 70).  I wouldn't have any great objection to adding more
entries there.  Or if somebody wanted to do the work, they
could be pulled out into separate files.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Robert Haas]

On Tue, Apr 23, 2024 at 6:23 AM Alvaro Herrera  wrote:
> I wonder if we're interested in keeping a (very short) manually-
> maintained list of symbols that we know are in use but the scripts
> don't extract for whatever reason.

+1. I think this idea has been proposed and rejected before, but I
think it's more important to have our code indented correctly than to
be able to rely on a 100% automated process for gathering typedefs.

There is of course the risk that the manually generated file will
accumulate stale cruft over time, but I don't really see that being a
big problem. First, it doesn't cost much to have a few extra symbols
in there. Second, I suspect someone will go through it at least every
couple of years, if not more often, and figure out which entries are
still doing something.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Alvaro Herrera]

On 2024-Apr-22, Tom Lane wrote:

> The main reason there's a delta is that people don't manage to
> maintain the in-tree copy perfectly (at least, they certainly
> haven't done so for this past year).  So we need to do that
> to clean up every now and then.

Out of curiosity, I downloaded the buildfarm-generated file and
re-indented the whole tree.  It turns out that most commits seem to have
maintained the in-tree typedefs list correctly when adding entries (even
if out of alphabetical order), but a few haven't; and some people have
added entries that the buildfarm script does not detect.  So the import
from BF will delete those entries and mess up the overall indent.  For
example it does stuff like

+++ b/src/backend/commands/async.c
@@ -399,7 +399,7 @@ typedef struct NotificationList
 typedef struct NotificationHash
 {
Notification *event;/* => the actual Notification struct */
-} NotificationHash;
+}  NotificationHash;

There's a good half dozen of those.

I wonder if we're interested in keeping a (very short) manually-
maintained list of symbols that we know are in use but the scripts
don't extract for whatever reason.


The change of NotificationHash looks surprising at first sight:
apparently 095d109ccd7 deleted the only use of that type as a variable
anywhere.  But then I wonder if that datatype is useful at all anymore,
since it only contains one pointer -- it seems we could just remove it.

But there are others: InjectionPointEntry, ResourceOwnerData,
JsonNonTerminal, JsonParserSem, ...

-- 
Álvaro Herrera PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"This is what I like so much about PostgreSQL.  Most of the surprises
are of the "oh wow!  That's cool" Not the "oh shit!" kind.  :)"
Scott Marlowe, http://archives.postgresql.org/pgsql-admin/2008-10/msg00152.php

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Nathan Bossart  writes:
> On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote:
>> I think the actual plan now is that we'll sync the in-tree copy
>> with the buildfarm's results (and then do a tree-wide pgindent)
>> every so often, probably shortly before beta every year.

> Okay.  Is this just to resolve the delta between the manual updates and a
> clean autogenerated copy every once in a while?

The main reason there's a delta is that people don't manage to
maintain the in-tree copy perfectly (at least, they certainly
haven't done so for this past year).  So we need to do that
to clean up every now and then.

A secondary reason is that the set of typedefs we absorb from
system include files changes over time.

regards, tom lane

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

On Mon, Apr 22, 2024 at 04:08:08PM -0400, Tom Lane wrote:
> I think the actual plan now is that we'll sync the in-tree copy
> with the buildfarm's results (and then do a tree-wide pgindent)
> every so often, probably shortly before beta every year.

Okay.  Is this just to resolve the delta between the manual updates and a
clean autogenerated copy every once in a while?

> The problem with the README is that it describes that process,
> rather than the now-typical workflow of incrementally keeping
> the tree indented.  I don't think we want to remove the info
> about how to do the full-monty process, but you're right that
> the README needs to explain the incremental method as being
> the one most developers would usually use.
> 
> Want to write some text?

Yup, I'll put something together.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Re: Why does pgindent's README say to download typedefs.list from the buildfarm?

[Tom Lane]

Nathan Bossart  writes:
> I used to do this step when I first started hacking on Postgres because
> that's what it says to do, but I've only ever used the in-tree one for many
> years now, and I'm not aware of any scenario where I might need to download
> a new version from the buildfarm.  I see that the in-tree copy wasn't added
> until 2010 (commit 1604057), so maybe this is just leftover from back then.

> Could we remove this note now?

I think the actual plan now is that we'll sync the in-tree copy
with the buildfarm's results (and then do a tree-wide pgindent)
every so often, probably shortly before beta every year.

The problem with the README is that it describes that process,
rather than the now-typical workflow of incrementally keeping
the tree indented.  I don't think we want to remove the info
about how to do the full-monty process, but you're right that
the README needs to explain the incremental method as being
the one most developers would usually use.

Want to write some text?

regards, tom lane

Why does pgindent's README say to download typedefs.list from the buildfarm?

[Nathan Bossart]

I used to do this step when I first started hacking on Postgres because
that's what it says to do, but I've only ever used the in-tree one for many
years now, and I'm not aware of any scenario where I might need to download
a new version from the buildfarm.  I see that the in-tree copy wasn't added
until 2010 (commit 1604057), so maybe this is just leftover from back then.

Could we remove this note now?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com