Re: automatically generating node support functions

Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)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