From: Tom Lane <tgl@sss.pgh.pa.us>
To: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
cc: David Rowley <dgrowleyml@gmail.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: automatically generating node support functions
In-reply-to: <c5906b07-220a-a3d4-8ff3-8ee593009424@enterprisedb.com>
References: <c1097590-a6a4-486a-64b1-e1f9cc0533ce@enterprisedb.com>
 <2f876203-7142-fa71-6d22-6ce00eb26869@enterprisedb.com>
 <c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com>
 <b85eec6e-a54b-6c72-97f8-2a6fbf4f9070@enterprisedb.com>
 <c683a356-4d22-4126-bf55-64ed54e06014@enterprisedb.com>
 <50fedc80-4104-bdc8-d777-04f030fc550f@dunslane.net>
 <7848b872-7a10-67b2-b55f-7c9c475ae863@enterprisedb.com>
 <40a2e977-98ba-0c01-af6c-9dfb39e864ba@enterprisedb.com>
 <812ea8eb-6827-ba32-6ec0-6714f7ffa5dd@enterprisedb.com>
 <2905876.1644858587@sss.pgh.pa.us>
 <bee9fdb0-cd10-5fdb-3027-c4b5a240bc74@enterprisedb.com>
 <CAApHDvppoxUxzRJbwF9oOF12ngdfNoPEMfDsu63+LREgGnheUw@mail.gmail.com>
 <968928d9-a526-a1b5-dc36-0355632a843e@enterprisedb.com>
 <c5906b07-220a-a3d4-8ff3-8ee593009424@enterprisedb.com>
Comments: In-reply-to Peter Eisentraut <peter.eisentraut@enterprisedb.com>
	message dated "Mon, 23 May 2022 07:49:52 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1294461.1656875649.1@sss.pgh.pa.us>
Date: Sun, 03 Jul 2022 15:14:09 -0400
Message-ID: <1294462.1656875649@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1294462.1656875649%40sss.pgh.pa.us>
Precedence: bulk

Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Here is a patch that reformats the relevant (and a few more) comments 
> that way.  This has been run through pgindent, so the formatting should 
> be stable.

Now that that's been pushed, the main patch is of course quite broken.
Are you working on a rebase?

I looked through the last published version of the main patch (Alvaro's
0002 from 2022-04-19), without trying to actually test it, and found
a couple of things that look wrong in the Makefiles:

* AFAICT, the infrastructure for removing the generated files at
"make *clean" is incomplete.  In particular I don't see any code
for removing the symlinks or the associated stamp file during
"make clean".  It looks like the existing header symlinks are
all cleaned up in src/include/Makefile's "clean" rule, so you
could do likewise for these.  Also, the "make maintainer-clean"
infrastructure seems incomplete --- shouldn't src/backend/Makefile's
maintainer-clean rule now also do
	$(MAKE) -C nodes $@
?

* There are some useful comments in backend/utils/Makefile that
I think should be carried over along with the make rules that
(it looks like) you cribbed from there, notably

# fmgr-stamp records the last time we ran Gen_fmgrtab.pl.  We don't rely on
# the timestamps of the individual output files, because the Perl script
# won't update them if they didn't change (to avoid unnecessary recompiles).

# These generated headers must be symlinked into builddir/src/include/,
# using absolute links for the reasons explained in src/backend/Makefile.
# We use header-stamp to record that we've done this because the symlinks
# themselves may appear older than fmgr-stamp.

and something similar to this for the "clean" rule:
# fmgroids.h, fmgrprotos.h, fmgrtab.c, fmgr-stamp, and errcodes.h are in the
# distribution tarball, so they are not cleaned here.


Also, I share David's upthread allergy to the option names "path_hackN"
and to documenting those only inside the conversion script.  I think
the existing text that you moved into the script, such as this bit:

		# We do not print the parent, else we'd be in infinite
		# recursion.  We can print the parent's relids for
		# identification purposes, though.  We print the pathtarget
		# only if it's not the default one for the rel.  We also do
		# not print the whole of param_info, since it's printed via
		# RelOptInfo; it's sufficient and less cluttering to print
		# just the required outer relids.

is perfectly adequate as documentation, it just needs to be somewhere else
(pathnodes.h seems fine, if not nodes.h) and labeled as to exactly which
pg_node_attr option invokes which behavior.

BTW, I think this: "Unknown attributes are ignored" is a seriously
bad idea; it will allow typos to escape detection.

			regards, tom lane