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

Nathan Bossart <nathandbossart@gmail.com> 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

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

Nathan Bossart <nathandbossart@gmail.com> 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

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

On Tue, Apr 23, 2024 at 6:23 AM Alvaro Herrera <alvherre@alvh.no-ip.org> 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

Alvaro Herrera <alvherre@alvh.no-ip.org> 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

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

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

On 22.04.24 22:28, Tom Lane wrote:

Nathan Bossart<nathandbossart@gmail.com> 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.

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

On 22.04.24 22:28, Tom Lane wrote:

Nathan Bossart<nathandbossart@gmail.com>  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:
<http://adpgtech.blogspot.com/2015/05/running-pgindent-on-non-core-code-or.html&gt;

cheers

andrew

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

Andrew Dunstan <andrew@dunslane.net> 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

On Tue, Apr 23, 2024 at 4:05 PM Nathan Bossart <nathandbossart@gmail.com> 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

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

On Wed, May 15, 2024 at 3:30 PM Nathan Bossart <nathandbossart@gmail.com> 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

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, May 15, 2024 at 3:30 PM Nathan Bossart <nathandbossart@gmail.com> 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

On Wed, May 15, 2024 at 04:07:18PM -0400, Robert Haas wrote:

On Wed, May 15, 2024 at 3:30 PM Nathan Bossart <nathandbossart@gmail.com> 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

Nathan Bossart <nathandbossart@gmail.com> 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

On Wed, May 15, 2024 at 4:50 PM Tom Lane <tgl@sss.pgh.pa.us> 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

On Wed, May 15, 2024 at 04:52:19PM -0400, Robert Haas wrote:

On Wed, May 15, 2024 at 4:50 PM Tom Lane <tgl@sss.pgh.pa.us> 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