Horton

Re: automatically generating node support functions

Peter Eisentraut <peter.eisentraut@enterprisedb.com>

View thread View on postgr.es
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: David Rowley <dgrowleyml@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>
Date: 2022-07-06T10:28:31Z
Lists: pgsql-hackers

Attachments

The new patch addresses almost all of these issues.

 > Also, I share David's upthread allergy to the option names
 > "path_hackN" and to documenting those only inside the conversion
 > script.

I have given these real names now and documented them with the other 
attributes.

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

fixed

(I have also changed the inside of pg_node_attr to be comma-separated, 
rather than space-separated.  This matches better how attribute-type 
things look in C.)

> 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)
> { ...

done

> 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

done (no_copy is actually no_copy_equal, hence renamed)

> 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.

Hmm, at least for Equivalence..., this is repeated a bunch of times for 
each field.  I don't know if this is really a property of the type or 
something you can choose for each field? [not changed in v7 patch]

> 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?

This is an existing comment in copyfuncs.c.  I haven't looked into it 
any further.

> 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.

The definition of CoercionForm in primnodes.h says that the comparison 
behavior is a property of the type, so it needs to be handled somewhere 
centrally, not on each field. [not changed in v7 patch]

> 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.

I have replaced these with explicit ignore markings in pathnodes.h 
(PlannerGlobal, PlannerInfo, RelOptInfo).  (This could then use a bit 
more rearranging some of the per-field comments.)

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

fixed

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

fixed

> * 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.

fixed -- I think, could use more testing

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