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