Re: automatically generating node support functions

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