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>
Subject: Re: automatically generating node support functions
In-reply-to: <57df9f30-d2cf-9126-28d4-ae40a2e733a9@enterprisedb.com>
References: <c1097590-a6a4-486a-64b1-e1f9cc0533ce@enterprisedb.com>
 <2f876203-7142-fa71-6d22-6ce00eb26869@enterprisedb.com>
 <c091e5cd-45f8-69ee-6a9b-de86912cc7e7@enterprisedb.com>
 <b85eec6e-a54b-6c72-97f8-2a6fbf4f9070@enterprisedb.com>
 <c683a356-4d22-4126-bf55-64ed54e06014@enterprisedb.com>
 <50fedc80-4104-bdc8-d777-04f030fc550f@dunslane.net>
 <7848b872-7a10-67b2-b55f-7c9c475ae863@enterprisedb.com>
 <40a2e977-98ba-0c01-af6c-9dfb39e864ba@enterprisedb.com>
 <812ea8eb-6827-ba32-6ec0-6714f7ffa5dd@enterprisedb.com>
 <2905876.1644858587@sss.pgh.pa.us>
 <bee9fdb0-cd10-5fdb-3027-c4b5a240bc74@enterprisedb.com>
 <CAApHDvppoxUxzRJbwF9oOF12ngdfNoPEMfDsu63+LREgGnheUw@mail.gmail.com>
 <968928d9-a526-a1b5-dc36-0355632a843e@enterprisedb.com>
 <c5906b07-220a-a3d4-8ff3-8ee593009424@enterprisedb.com>
 <1294462.1656875649@sss.pgh.pa.us>
 <57df9f30-d2cf-9126-28d4-ae40a2e733a9@enterprisedb.com>
Comments: In-reply-to Peter Eisentraut <peter.eisentraut@enterprisedb.com>
	message dated "Mon, 04 Jul 2022 14:23:36 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1593977.1656953960.1@sss.pgh.pa.us>
Date: Mon, 04 Jul 2022 12:59:20 -0400
Message-ID: <1593978.1656953960@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1593978.1656953960%40sss.pgh.pa.us>
Precedence: bulk

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