Horton

Re: automatically generating node support functions

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
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>
Date: 2022-07-04T16:59:20Z
Lists: pgsql-hackers 
Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> [ v6-0001-Automatically-generate-node-support-functions.patch ]

I've now spent some time looking at this fairly carefully, and I think
this is a direction we can pursue, but I'm not yet happy about the
amount of magic knowledge that's embedded in the gen_node_support.pl
script rather than being encoded in pg_node_attr markers.  Once this
is in place, people will stop thinking about the nodes/*funcs.c
infrastructure altogether when they write patches, at least until
they get badly burned by it; so I don't want there to be big gotchas.
As an example, heaven help the future hacker who decides to change
the contents of A_Const and doesn't realize that that still has a
manually-implemented copyfuncs.c routine.  So rather than embedding
knowledge in gen_node_support.pl like this:

my @custom_copy = qw(A_Const Const ExtensibleNode);

I think we ought to put it into the *nodes.h headers as much as
possible, perhaps like this:

typedef struct A_Const pg_node_attr(custom_copy)
{ ...

I will grant that there are some things that are okay to embed
in gen_node_support.pl, such as the list of @scalar_types,
because if you need to add an entry there you will find it out
when the script complains it doesn't know how to process a field.
So there is some judgment involved here, but on the whole I want
to err on the side of exposing decisions in the headers.

So I propose that we handle these things via struct-level pg_node_attr
markers, rather than node-type lists embedded in the script:

abstract_types
no_copy
no_read_write
no_read
custom_copy
custom_readwrite

(The markings that "we are not publishing right now to stay level with the
manual system" are fine to apply in the script, since that's probably a
temporary thing anyway.  Also, I don't have a problem with applying
no_copy etc to the contents of whole files in the script, rather than
tediously labeling each struct in such files.)

The hacks for scalar-copying EquivalenceClass*, EquivalenceMember*,
struct CustomPathMethods*, and CustomScan.methods should be replaced
with "pg_node_attr(copy_as_scalar)" labels on affected fields.

I wonder whether this:

                    # We do not support copying Path trees, mainly
                    # because the circular linkages between RelOptInfo
                    # and Path nodes can't be handled easily in a
                    # simple depth-first traversal.

couldn't be done better by inventing an inheritable no_copy attr
to attach to the Path supertype.  Or maybe it'd be okay to just
automatically inherit the no_xxx properties from the supertype?

I don't terribly like the ad-hoc mechanism for not comparing
CoercionForm fields.  OTOH, I am not sure whether replacing it
with per-field equal_ignore attrs would be better; there's at least
an argument that that invites bugs of omission.  But implementing
this with an uncommented test deep inside a script that most hackers
should not need to read is not good.  On the whole I'd lean towards
the equal_ignore route.

I'm confused by the "various field types to ignore" at the end
of the outfuncs/readfuncs code.  Do we really ignore those now?
How could that be safe?  If it is safe, wouldn't it be better
to handle that with per-field pg_node_attrs?  Silently doing
what might be the wrong thing doesn't seem good.

In the department of nitpicks:

* copyfuncs.switch.c and equalfuncs.switch.c are missing trailing
newlines.

* pgindent is not very happy with a lot of your comments in *nodes.h.

* I think we should add explicit dependencies in backend/nodes/Makefile,
along the lines of

copyfuncs.o: copyfuncs.c copyfuncs.funcs.c copyfuncs.switch.c

Otherwise the whole thing is a big gotcha for anyone not using
--enable-depend.

I don't know if you have time right now to push forward with these
points, but if you don't I can take a stab at it.  I would like to
see this done and committed PDQ, because 835d476fd already broke
many patches that touch *nodes.h and I'd like to get the rest of
the fallout in place before rebasing affected patches.

			regards, tom lane

Commits

  1. Fix MSVC build script's check for obsolete node support functions.

  2. Improve performance of ORDER BY / DISTINCT aggregates

  3. doc: Fix typos in protocol.sgml

  4. Tighten up parsing logic in gen_node_support.pl.

  5. Add defenses against unexpected changes in the NodeTag enum list.

  6. Add copy/equal support for XID lists

  7. Rationalize order of input files for gen_node_support.pl.

  8. Make assorted quality-of-life improvements in gen_node_support.pl.

  9. Doc: rearrange high-level commentary about node support coverage.

  10. Automatically generate node support functions

  11. Adjust node serialization tag of A_Expr for consistency

  12. Remove T_Join and T_Plan

  13. Reformat some more node comments

  14. Reformat some node comments

  15. Remove JsonPathSpec typedef

  16. Add missing enum tag in enum used in nodes

  17. Add Cardinality typedef

  18. Make node output prefix match node structure name

  19. Add WRITE_INDEX_ARRAY

  20. Add COPY_ARRAY_FIELD and COMPARE_ARRAY_FIELD

  21. Remove T_Expr

  22. Change NestPath node to contain JoinPath node

  23. Change SeqScan node to contain Scan node

  24. Check the size in COPY_POINTER_FIELD

  25. Remove T_MemoryContext

  26. Add missing enum tags in enums used in nodes

  27. Rename some node support functions for consistency

  28. Rename argument of _outValue()

  29. Rename NodeTag of ExprState