Horton

Thread

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

  1. automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-07T20:27:52Z

    I wrote a script to automatically generate the node support functions 
(copy, equal, out, and read, as well as the node tags enum) from the 
struct definitions.

The first eight patches are to clean up various inconsistencies to make 
parsing or generation easier.

The interesting stuff is in patch 0009.

For each of the four node support files, it creates two include files, 
e.g., copyfuncs.inc1.c and copyfuncs.inc2.c to include in the main file. 
  All the scaffolding of the main file stays in place.

In this patch, I have only ifdef'ed out the code to could be removed, 
mainly so that it won't constantly have merge conflicts.  Eventually, 
that should all be changed to delete the code.  When we do that, some 
code comments should probably be preserved elsewhere, so that will need 
another pass of consideration.

I have tried to mostly make the coverage of the output match what is 
currently there.  For example, one could do out/read coverage of utility 
statement nodes easily with this, but I have manually excluded those for 
now.  The reason is mainly that it's easier to diff the before and 
after, and adding a bunch of stuff like this might require a separate 
analysis and review.

Subtyping (TidScan -> Scan) is supported.

For the hard cases, you can just write a manual function and exclude 
generating one.

For the not so hard cases, there is a way of annotating struct fields to 
get special behaviors.  For example, pg_node_attr(equal_ignore) has the 
field ignored in equal functions.

There are a couple of additional minor issues mentioned in the script 
source.  But basically, it all seems to work.

  2. Re: automatically generating node support functions

    David Rowley <dgrowleyml@gmail.com> — 2021-06-08T13:40:06Z

    On Tue, 8 Jun 2021 at 08:28, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> I wrote a script to automatically generate the node support functions
> (copy, equal, out, and read, as well as the node tags enum) from the
> struct definitions.

Thanks for working on this. I agree that it would be nice to see
improvements in this area.

It's almost 2 years ago now, but I'm wondering if you saw what Andres
proposed in [1]?  The idea was basically to make a metadata array of
the node structs so that, instead of having to output large amounts of
.c code to do read/write/copy/equals, instead just have small
functions that loop over the elements in the array for the given
struct and perform the required operation based on the type.

There were still quite a lot of unsolved problems, for example, how to
determine the length of arrays so that we know how many bytes to
compare in equal funcs.   I had a quick look at what you've got and
see you've got a solution for that by looking at the last "int" field
before the array and using that. (I wonder if you'd be better to use
something more along the lines of your pg_node_attr() for that?)

There's quite a few advantages having the metadata array rather than
the current approach:

1. We don't need to compile 4 huge .c files and link them into the
postgres binary. I imagine this will make the binary a decent amount
smaller.
2. We can easily add more operations on nodes.  e.g serialize nodes
for sending plans to parallel workers.  or generating a hash value so
we can store node types in a hash table.

One disadvantage would be what Andres mentioned in [2].  He found
around a 5% performance regression.  However, looking at the
NodeTypeComponents struct in [1], we might be able to speed it up
further by shrinking that struct down a bit and just storing an uint16
position into a giant char array which contains all of the field
names. I imagine they wouldn't take more than 64k. fieldtype could see
a similar change. That would take the NodeTypeComponents struct from
26 bytes down to 14 bytes, which means about double the number of
field metadata we could fit on a cache line.

Do you have any thoughts about that approach instead?

David

[1] https://www.postgresql.org/message-id/20190828234136.fk2ndqtld3onfrrp@alap3.anarazel.de
[2] https://www.postgresql.org/message-id/20190920051857.2fhnvhvx4qdddviz@alap3.anarazel.de

  3. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-06-08T17:45:58Z

    On 08.06.21 15:40, David Rowley wrote:
> It's almost 2 years ago now, but I'm wondering if you saw what Andres
> proposed in [1]?  The idea was basically to make a metadata array of
> the node structs so that, instead of having to output large amounts of
> .c code to do read/write/copy/equals, instead just have small
> functions that loop over the elements in the array for the given
> struct and perform the required operation based on the type.

That project was technologically impressive, but it seemed to have 
significant hurdles to overcome before it can be useful.  My proposal is 
usable and useful today.  And it doesn't prevent anyone from working on 
a more sophisticated solution.

> There were still quite a lot of unsolved problems, for example, how to
> determine the length of arrays so that we know how many bytes to
> compare in equal funcs.   I had a quick look at what you've got and
> see you've got a solution for that by looking at the last "int" field
> before the array and using that. (I wonder if you'd be better to use
> something more along the lines of your pg_node_attr() for that?)

I considered that, but since the convention seemed to work everywhere, I 
left it.  But it wouldn't be hard to change.

  4. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2021-06-11T19:23:53Z

    Hi,

On 2021-06-08 19:45:58 +0200, Peter Eisentraut wrote:
> On 08.06.21 15:40, David Rowley wrote:
> > It's almost 2 years ago now, but I'm wondering if you saw what Andres
> > proposed in [1]?  The idea was basically to make a metadata array of
> > the node structs so that, instead of having to output large amounts of
> > .c code to do read/write/copy/equals, instead just have small
> > functions that loop over the elements in the array for the given
> > struct and perform the required operation based on the type.
> 
> That project was technologically impressive, but it seemed to have
> significant hurdles to overcome before it can be useful.  My proposal is
> usable and useful today.  And it doesn't prevent anyone from working on a
> more sophisticated solution.

I think it's short-sighted to further and further go down the path of
parsing "kind of C" without just using a proper C parser. But leaving
that aside, a big part of the promise of the approach in that thread
isn't actually tied to the specific way the type information is
collected: The perl script could output something like the "node type
metadata" I generated in that patchset, and then we don't need the large
amount of generated code and can much more economically add additional
operations handling node types.

Greetings,

Andres Freund

  5. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-14T21:42:10Z

    Andres Freund <andres@anarazel.de> writes:
> On 2021-06-08 19:45:58 +0200, Peter Eisentraut wrote:
>> On 08.06.21 15:40, David Rowley wrote:
>>> It's almost 2 years ago now, but I'm wondering if you saw what Andres
>>> proposed in [1]?

>> That project was technologically impressive, but it seemed to have
>> significant hurdles to overcome before it can be useful.  My proposal is
>> usable and useful today.  And it doesn't prevent anyone from working on a
>> more sophisticated solution.

> I think it's short-sighted to further and further go down the path of
> parsing "kind of C" without just using a proper C parser. But leaving
> that aside, a big part of the promise of the approach in that thread
> isn't actually tied to the specific way the type information is
> collected: The perl script could output something like the "node type
> metadata" I generated in that patchset, and then we don't need the large
> amount of generated code and can much more economically add additional
> operations handling node types.

I think the main reason that the previous patch went nowhere was general
resistance to making developers install something as complicated as
libclang --- that could be a big lift on non-mainstream platforms.
So IMO it's a feature not a bug that Peter's approach just uses a perl
script.  OTOH, the downstream aspects of your patch did seem appealing.
So I'd like to see a merger of the two approaches, using perl for the
data extraction and then something like what you'd done.  Maybe that's
the same thing you're saying.

I also see Peter's point that committing what he has here might be
a reasonable first step on that road.  Getting the data extraction
right is a big chunk of the job, and what we do with it afterward
could be improved later.

			regards, tom lane

  6. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2021-07-15T01:24:54Z

    Hi,

On 2021-07-14 17:42:10 -0400, Tom Lane wrote:
> I think the main reason that the previous patch went nowhere was general
> resistance to making developers install something as complicated as
> libclang --- that could be a big lift on non-mainstream platforms.

I'm still not particularly convinced it's and issue - I was suggesting
we commit the resulting metadata, so libclang would only be needed when
modifying node types. And even in case one needs to desperately modify
node types on a system without access to libclang, for an occasionally
small change one could just modify the committed metadata structs
manually.


> So IMO it's a feature not a bug that Peter's approach just uses a perl
> script.  OTOH, the downstream aspects of your patch did seem appealing.
> So I'd like to see a merger of the two approaches, using perl for the
> data extraction and then something like what you'd done.  Maybe that's
> the same thing you're saying.

Yes, that's what I was trying to say. I'm still doubtful it's a great
idea to go further down the "weird subset of C parsed by regexes" road,
but I can live with it.  If Peter could generate something roughly like
the metadata I emitted, I'd rebase my node functions ontop of that.


> I also see Peter's point that committing what he has here might be
> a reasonable first step on that road.  Getting the data extraction
> right is a big chunk of the job, and what we do with it afterward
> could be improved later.

To me that seems likely to just cause churn without saving much
effort. The needed information isn't really the same between generating
the node functions as text and collecting the metadata for "generic node
functions", and none of the output is the same.

Greetings,

Andres Freund

  7. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-07-19T06:59:18Z

    On 07.06.21 22:27, Peter Eisentraut wrote:
> I wrote a script to automatically generate the node support functions 
> (copy, equal, out, and read, as well as the node tags enum) from the 
> struct definitions.
> 
> The first eight patches are to clean up various inconsistencies to make 
> parsing or generation easier.

Are there any concerns about the patches 0001 through 0008?  Otherwise, 
maybe we could get those out of the way.

  8. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-26T21:25:27Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> The first eight patches are to clean up various inconsistencies to make 
>> parsing or generation easier.

> Are there any concerns about the patches 0001 through 0008?  Otherwise, 
> maybe we could get those out of the way.

I looked through those and don't have any complaints (though I just
eyeballed them, I didn't see what a compiler would say).  I see
you pushed a couple of them already.

			regards, tom lane

  9. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-08-17T14:36:45Z

    Here is another set of preparatory patches that clean up various special 
cases and similar in the node support.

0001-Remove-T_Expr.patch

Removes unneeded T_Expr.

0002-Add-COPY_ARRAY_FIELD-and-COMPARE_ARRAY_FIELD.patch
0003-Add-WRITE_INDEX_ARRAY.patch

These add macros to handle a few cases that were previously hand-coded.

0004-Make-node-output-prefix-match-node-structure-name.patch

Some nodes' output/read functions use a label that is slightly different 
from their node name, e.g., "NOTIFY" instead of "NOTIFYSTMT".  This 
cleans that up so that an automated approach doesn't have to deal with 
these special cases.

0005-Add-Cardinality-typedef.patch

Adds a typedef Cardinality for double fields that store an estimated row 
or other count.  Works alongside Cost and Selectivity.

This is useful because it appears that the serialization format for 
these float fields depends on their intent: Cardinality => %.0f, Cost => 
%.2f, Selectivity => %.4f.  The only remaining exception is allvisfrac, 
which uses %.6f.  Maybe that could also be a Selectivity, but I left it 
as is.  I think this improves the clarity in this area.

  10. Re: automatically generating node support functions

    Jacob Champion <pchampion@vmware.com> — 2021-09-02T18:53:37Z

    On Tue, 2021-08-17 at 16:36 +0200, Peter Eisentraut wrote:
> Here is another set of preparatory patches that clean up various special 
> cases and similar in the node support.
> 
> 0001-Remove-T_Expr.patch
> 
> Removes unneeded T_Expr.
> 
> 0002-Add-COPY_ARRAY_FIELD-and-COMPARE_ARRAY_FIELD.patch
> 0003-Add-WRITE_INDEX_ARRAY.patch
> 
> These add macros to handle a few cases that were previously hand-coded.

These look sane to me.

> 0004-Make-node-output-prefix-match-node-structure-name.patch
> 
> Some nodes' output/read functions use a label that is slightly different 
> from their node name, e.g., "NOTIFY" instead of "NOTIFYSTMT".  This 
> cleans that up so that an automated approach doesn't have to deal with 
> these special cases.

Is there any concern about the added serialization length, or is that
trivial in practice? The one that particularly caught my eye is
RANGETBLENTRY, which was previously RTE. But I'm not very well-versed
in all the places these strings can be generated and stored.

> 0005-Add-Cardinality-typedef.patch
> 
> Adds a typedef Cardinality for double fields that store an estimated row 
> or other count.  Works alongside Cost and Selectivity.

Should RangeTblEntry.enrtuples also be a Cardinality?

--Jacob

  11. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-07T08:57:02Z

    On 02.09.21 20:53, Jacob Champion wrote:
>> 0004-Make-node-output-prefix-match-node-structure-name.patch
>>
>> Some nodes' output/read functions use a label that is slightly different
>> from their node name, e.g., "NOTIFY" instead of "NOTIFYSTMT".  This
>> cleans that up so that an automated approach doesn't have to deal with
>> these special cases.
> 
> Is there any concern about the added serialization length, or is that
> trivial in practice? The one that particularly caught my eye is
> RANGETBLENTRY, which was previously RTE. But I'm not very well-versed
> in all the places these strings can be generated and stored.

These are just matters of taste.  Let's wait a bit more to see if anyone 
is concerned.

>> 0005-Add-Cardinality-typedef.patch
>>
>> Adds a typedef Cardinality for double fields that store an estimated row
>> or other count.  Works alongside Cost and Selectivity.
> 
> Should RangeTblEntry.enrtuples also be a Cardinality?

Yes, I'll add that.

  12. Re: automatically generating node support functions

    Noah Misch <noah@leadboat.com> — 2021-09-08T04:30:46Z

    On Tue, Sep 07, 2021 at 10:57:02AM +0200, Peter Eisentraut wrote:
> On 02.09.21 20:53, Jacob Champion wrote:
> >>0004-Make-node-output-prefix-match-node-structure-name.patch
> >>
> >>Some nodes' output/read functions use a label that is slightly different
> >>from their node name, e.g., "NOTIFY" instead of "NOTIFYSTMT".  This
> >>cleans that up so that an automated approach doesn't have to deal with
> >>these special cases.
> >
> >Is there any concern about the added serialization length, or is that
> >trivial in practice? The one that particularly caught my eye is
> >RANGETBLENTRY, which was previously RTE. But I'm not very well-versed
> >in all the places these strings can be generated and stored.
> 
> These are just matters of taste.  Let's wait a bit more to see if anyone is
> concerned.

I am not concerned about changing the serialization length this much.  The
format is already quite verbose, and this change is small relative to that
existing verbosity.

  13. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-09-15T19:01:33Z

    On 17.08.21 16:36, Peter Eisentraut wrote:
> Here is another set of preparatory patches that clean up various special 
> cases and similar in the node support.

This set of patches has been committed.  I'll close this commit fest 
entry and come back with the main patch series in the future.

  14. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-11T14:22:33Z

    On 15.09.21 21:01, Peter Eisentraut wrote:
> On 17.08.21 16:36, Peter Eisentraut wrote:
>> Here is another set of preparatory patches that clean up various 
>> special cases and similar in the node support.
> 
> This set of patches has been committed.  I'll close this commit fest 
> entry and come back with the main patch series in the future.

Here is an updated version of my original patch, so we have something to 
continue the discussion around.  This takes into account all the 
preparatory patches that have been committed in the meantime.  I have 
also changed it so that the array size of a pointer is now explicitly 
declared using pg_node_attr(array_size(N)) instead of picking the most 
recent scalar field, which was admittedly hacky.  I have also added MSVC 
build support and made the Perl code more portable, so that the cfbot 
doesn't have to be sad.

  15. Re: automatically generating node support functions

    Corey Huinker <corey.huinker@gmail.com> — 2021-10-12T01:06:50Z

    >
> build support and made the Perl code more portable, so that the cfbot
> doesn't have to be sad.
>

Was this also the reason for doing the output with print statements rather
than using one of the templating libraries? I'm mostly just curious, and
certainly don't want it to get in the way of working code.

  16. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-10-12T13:04:04Z

    On 12.10.21 03:06, Corey Huinker wrote:
>     build support and made the Perl code more portable, so that the cfbot
>     doesn't have to be sad.
> 
> 
> Was this also the reason for doing the output with print statements 
> rather than using one of the templating libraries? I'm mostly just 
> curious, and certainly don't want it to get in the way of working code.

Unless there is a templating library that ships with Perl (>= 5.8.3, 
apparently now), this seems impractical.

  17. Re: automatically generating node support functions

    Andrew Dunstan <andrew@dunslane.net> — 2021-10-12T13:52:15Z

    On 10/11/21 10:22 AM, Peter Eisentraut wrote:
>
> On 15.09.21 21:01, Peter Eisentraut wrote:
>> On 17.08.21 16:36, Peter Eisentraut wrote:
>>> Here is another set of preparatory patches that clean up various
>>> special cases and similar in the node support.
>>
>> This set of patches has been committed.  I'll close this commit fest
>> entry and come back with the main patch series in the future.
>
> Here is an updated version of my original patch, so we have something
> to continue the discussion around.  This takes into account all the
> preparatory patches that have been committed in the meantime.  I have
> also changed it so that the array size of a pointer is now explicitly
> declared using pg_node_attr(array_size(N)) instead of picking the most
> recent scalar field, which was admittedly hacky.  I have also added
> MSVC build support and made the Perl code more portable, so that the
> cfbot doesn't have to be sad.



I haven't been through the whole thing, but I did notice this: the
comment stripping code looks rather fragile. I think it would blow up if
there were a continuation line not starting with  qr/\s*\*/. It's a lot
simpler and more robust to do this if you slurp the file in whole.
Here's what we do in the buildfarm code:

    my $src = file_contents($_);
    # strip C comments
    # We used to use the recipe in perlfaq6 but there is actually no point.
    # We don't need to keep the quoted string values anyway, and
    # on some platforms the complex regex causes perl to barf and crash.
    $src =~ s{/\*.*?\*/}{}gs;

After you've done that splitting it into lines is pretty simple.

cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  18. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2021-12-29T11:08:17Z

    On 12.10.21 15:52, Andrew Dunstan wrote:
> I haven't been through the whole thing, but I did notice this: the
> comment stripping code looks rather fragile. I think it would blow up if
> there were a continuation line not starting with  qr/\s*\*/. It's a lot
> simpler and more robust to do this if you slurp the file in whole.
> Here's what we do in the buildfarm code:
> 
>      my $src = file_contents($_);
>      # strip C comments
>      # We used to use the recipe in perlfaq6 but there is actually no point.
>      # We don't need to keep the quoted string values anyway, and
>      # on some platforms the complex regex causes perl to barf and crash.
>      $src =~ s{/\*.*?\*/}{}gs;
> 
> After you've done that splitting it into lines is pretty simple.

Here is an updated patch, with some general rebasing, and the above 
improvement.  It now also generates #include lines necessary in 
copyfuncs etc. to pull in all the node types it operates on.

Further, I have looked more into the "metadata" approach discussed in 
[0].  It's pretty easy to generate that kind of output from the data 
structures my script produces.  You just loop over all the node types 
and print stuff and keep a few counters.  I don't plan to work on that 
at this time, but I just wanted to point out that if people wanted to 
move into that direction, my patch wouldn't be in the way.


[0]: 
https://www.postgresql.org/message-id/flat/20190828234136.fk2ndqtld3onfrrp%40alap3.anarazel.de

  19. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-01-24T15:15:48Z

    Rebased patch to resolve some merge conflicts

On 29.12.21 12:08, Peter Eisentraut wrote:
> On 12.10.21 15:52, Andrew Dunstan wrote:
>> I haven't been through the whole thing, but I did notice this: the
>> comment stripping code looks rather fragile. I think it would blow up if
>> there were a continuation line not starting with  qr/\s*\*/. It's a lot
>> simpler and more robust to do this if you slurp the file in whole.
>> Here's what we do in the buildfarm code:
>>
>>      my $src = file_contents($_);
>>      # strip C comments
>>      # We used to use the recipe in perlfaq6 but there is actually no 
>> point.
>>      # We don't need to keep the quoted string values anyway, and
>>      # on some platforms the complex regex causes perl to barf and crash.
>>      $src =~ s{/\*.*?\*/}{}gs;
>>
>> After you've done that splitting it into lines is pretty simple.
> 
> Here is an updated patch, with some general rebasing, and the above 
> improvement.  It now also generates #include lines necessary in 
> copyfuncs etc. to pull in all the node types it operates on.
> 
> Further, I have looked more into the "metadata" approach discussed in 
> [0].  It's pretty easy to generate that kind of output from the data 
> structures my script produces.  You just loop over all the node types 
> and print stuff and keep a few counters.  I don't plan to work on that 
> at this time, but I just wanted to point out that if people wanted to 
> move into that direction, my patch wouldn't be in the way.
> 
> 
> [0]: 
> https://www.postgresql.org/message-id/flat/20190828234136.fk2ndqtld3onfrrp%40alap3.anarazel.de

  20. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-02-14T10:15:57Z

    What do people think about this patch now?

I have received some feedback on several small technical issues, which 
have all been fixed.  This patch has been around for several commit 
fests now and AFAICT, nothing has broken it.  This is just to indicate 
that the parsing isn't as flimsy as one might fear.

One thing thing that is waiting behind this patch is that you currently 
cannot put utility commands into parse-time SQL functions, because there 
is no full out/read support for those.  This patch would fix that 
problem.  (There is a little bit of additional work necessary, but I 
have that mostly worked out in a separate branch.)


On 24.01.22 16:15, Peter Eisentraut wrote:
> Rebased patch to resolve some merge conflicts
> 
> On 29.12.21 12:08, Peter Eisentraut wrote:
>> On 12.10.21 15:52, Andrew Dunstan wrote:
>>> I haven't been through the whole thing, but I did notice this: the
>>> comment stripping code looks rather fragile. I think it would blow up if
>>> there were a continuation line not starting with  qr/\s*\*/. It's a lot
>>> simpler and more robust to do this if you slurp the file in whole.
>>> Here's what we do in the buildfarm code:
>>>
>>>      my $src = file_contents($_);
>>>      # strip C comments
>>>      # We used to use the recipe in perlfaq6 but there is actually no 
>>> point.
>>>      # We don't need to keep the quoted string values anyway, and
>>>      # on some platforms the complex regex causes perl to barf and 
>>> crash.
>>>      $src =~ s{/\*.*?\*/}{}gs;
>>>
>>> After you've done that splitting it into lines is pretty simple.
>>
>> Here is an updated patch, with some general rebasing, and the above 
>> improvement.  It now also generates #include lines necessary in 
>> copyfuncs etc. to pull in all the node types it operates on.
>>
>> Further, I have looked more into the "metadata" approach discussed in 
>> [0].  It's pretty easy to generate that kind of output from the data 
>> structures my script produces.  You just loop over all the node types 
>> and print stuff and keep a few counters.  I don't plan to work on that 
>> at this time, but I just wanted to point out that if people wanted to 
>> move into that direction, my patch wouldn't be in the way.
>>
>>
>> [0]: 
>> https://www.postgresql.org/message-id/flat/20190828234136.fk2ndqtld3onfrrp%40alap3.anarazel.de 
>>

  21. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-14T17:09:47Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> What do people think about this patch now?

I'm in favor of moving forward with this.  I do not like the
libclang-based approach that Andres was pushing, because of the
jump in developer tooling requirements that it'd cause.

Eyeballing the patch a bit, I do have some comments:

* It's time for action on the business about extracting comments
from the to-be-deleted code.

* The Perl script is kind of under-commented for my taste.
It lacks a copyright notice, too.

* In the same vein, I should not have to reverse-engineer what
the available pg_node_attr() properties are or do.  Perhaps they
could be documented in the comment for the pg_node_attr macro
in nodes.h.

* Maybe the generated file names could be chosen less opaquely,
say ".funcs" and ".switch" instead of ".inc1" and ".inc2".

* I don't understand why there are changes in the #include
lists in copyfuncs.c etc?

* I think that more thought needs to be put into the format
of the *nodes.h struct declarations, because I fear pgindent
is going to make a hash of what you've done here.  When we
did similar stuff in the catalog headers, I think we ended
up moving a lot of end-of-line comments onto their own lines.

* I assume the pg_config_manual.h changes are not meant for
commit?

			regards, tom lane

  22. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-02-14T23:23:48Z

    Hi,

On 2022-02-14 12:09:47 -0500, Tom Lane wrote:
> I'm in favor of moving forward with this.  I do not like the
> libclang-based approach that Andres was pushing, because of the
> jump in developer tooling requirements that it'd cause.

FWIW, while I don't love the way the header parsing stuff in the patch (vs
using libclang or such), I don't have a real problem with it.

I do however not think it's a good idea to commit something generating
something like the existing node functions vs going for a metadata based
approach at dealing with node functions. That aspect of my patchset is
independent of the libclang vs script debate.

Greetings,

Andres Freund

  23. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-14T23:32:21Z

    Andres Freund <andres@anarazel.de> writes:
> I do however not think it's a good idea to commit something generating
> something like the existing node functions vs going for a metadata based
> approach at dealing with node functions. That aspect of my patchset is
> independent of the libclang vs script debate.

I think that finishing out and committing this patch is a fine step
on the way to that.  Either that, or you should go ahead and merge
your backend work onto what Peter's done ... but that seems like
it'll be bigger and harder to review.

			regards, tom lane

  24. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-02-15T01:32:46Z

    Hi,

On 2022-02-14 18:32:21 -0500, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I do however not think it's a good idea to commit something generating
> > something like the existing node functions vs going for a metadata based
> > approach at dealing with node functions. That aspect of my patchset is
> > independent of the libclang vs script debate.
> 
> I think that finishing out and committing this patch is a fine step
> on the way to that.

I think most of gen_node_support.pl would change - a lot of that is generating
the node functions, which would not be needed anymore. And most of the
remainder would change as well.


> Either that, or you should go ahead and merge your backend work onto what
> Peter's done ...

I did offer to do part of that a while ago:

https://www.postgresql.org/message-id/20210715012454.bvwg63farhmfwb47%40alap3.anarazel.de

On 2021-07-14 18:24:54 -0700, Andres Freund wrote:
> If Peter could generate something roughly like the metadata I emitted, I'd
> rebase my node functions ontop of that.

Greetings,

Andres Freund

  25. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-02-15T01:47:33Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-02-14 18:32:21 -0500, Tom Lane wrote:
>> I think that finishing out and committing this patch is a fine step
>> on the way to that.

> I think most of gen_node_support.pl would change - a lot of that is generating
> the node functions, which would not be needed anymore. And most of the
> remainder would change as well.

Well, yeah, we'd be throwing away some of that Perl code.  So what?
I think that most of the intellectual content in this patch is getting
the data source nailed down, ie putting the annotations into the *nodes.h
files and building the code to parse that.  I don't have a problem
with throwing away and rewriting the back-end part of the patch later.

And, TBH, I am not really convinced that a pure metadata approach is going
to work out, or that it will have sufficient benefit over just automating
the way we do it now.  I notice that Peter's patch leaves a few
too-much-of-a-special-case functions unconverted, which is no real
problem for his approach; but it seems like you won't get to take such
shortcuts in a metadata-reading implementation.

The bottom line here is that I believe that Peter's patch could get us out
of the business of hand-maintaining the backend/nodes/*.c files in the
v15 timeframe, which would be a very nice thing.  I don't see how your
patch will be ready on anywhere near the same schedule.  When it is ready,
we can switch, but in the meantime I'd like the maintenance benefit.

			regards, tom lane

  26. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-02-15T02:10:25Z

    Hi,

On 2022-02-14 20:47:33 -0500, Tom Lane wrote:
> I think that most of the intellectual content in this patch is getting
> the data source nailed down, ie putting the annotations into the *nodes.h
> files and building the code to parse that.  I don't have a problem
> with throwing away and rewriting the back-end part of the patch later.

Imo that cuts the other way - without going for a metadata based approach we
don't know if we made the annotations rich enough...


> And, TBH, I am not really convinced that a pure metadata approach is going
> to work out, or that it will have sufficient benefit over just automating
> the way we do it now.  I notice that Peter's patch leaves a few
> too-much-of-a-special-case functions unconverted, which is no real
> problem for his approach; but it seems like you won't get to take such
> shortcuts in a metadata-reading implementation.

IMO my prototype of that approach pretty conclusively shows that it's feasible
and worthwhile.


> The bottom line here is that I believe that Peter's patch could get us out
> of the business of hand-maintaining the backend/nodes/*.c files in the v15
> timeframe, which would be a very nice thing.  I don't see how your patch
> will be ready on anywhere near the same schedule.  When it is ready, we can
> switch, but in the meantime I'd like the maintenance benefit.

I'm not going to try to prevent the patch from going in. But I don't think
it's a great idea to this without even trying to ensure the annotations are
rich enough...

Greetings,

Andres Freund

  27. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-02-18T06:51:56Z

    On 14.02.22 18:09, Tom Lane wrote:
> * It's time for action on the business about extracting comments
> from the to-be-deleted code.

done

> * The Perl script is kind of under-commented for my taste.
> It lacks a copyright notice, too.

done

> * In the same vein, I should not have to reverse-engineer what
> the available pg_node_attr() properties are or do.  Perhaps they
> could be documented in the comment for the pg_node_attr macro
> in nodes.h.

done

> * Maybe the generated file names could be chosen less opaquely,
> say ".funcs" and ".switch" instead of ".inc1" and ".inc2".

done

> * I don't understand why there are changes in the #include
> lists in copyfuncs.c etc?

Those are #include lines required for the definitions of various 
structs.  Since the generation script already knows which header files 
are relevant (they are its input files), it can just generate the 
required #include lines as well.  That way, the remaining copyfuncs.c 
only has #include lines for things that the (remaining) file itself 
needs, not what the files included by it need, and if a new header file 
were to be added, it doesn't have to be added in 4+ places.

> * I think that more thought needs to be put into the format
> of the *nodes.h struct declarations, because I fear pgindent
> is going to make a hash of what you've done here.  When we
> did similar stuff in the catalog headers, I think we ended
> up moving a lot of end-of-line comments onto their own lines.

I have tested pgindent repeatedly throughout this project, and it 
doesn't look too bad.  You are right that some manual curation of 
comment formatting would be sensible, but I think that might be better 
done as a separate patch.

> * I assume the pg_config_manual.h changes are not meant for
> commit?

right

  28. Re: automatically generating node support functions

    David Rowley <dgrowleyml@gmail.com> — 2022-03-24T21:57:29Z

    On Fri, 18 Feb 2022 at 19:52, Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> [ v5-0001-Automatically-generate-node-support-functions.patch ]

I've been looking over the patch and wondering the best way to move
this forward.

But first a couple of things I noted down from reading the patch:

1. You're written:

 * Unknown attributes are ignored.  Some additional attributes are used for
 * special "hack" cases.

I think these really should all be documented.  If someone needs to
use one of these hacks then they're going to need to trawl through
Perl code to see if you've implemented something that matches the
requirements.  I'd personally rather not have to look at the Perl code
to find out which attributes I need to use for my new field. I'd bet
I'm not the only one.

2. Some of these comment lines have become pretty long after having
added the attribute macro.

e.g.

PlannerInfo *subroot pg_node_attr(readwrite_ignore); /* modified
"root" for planning the subquery;
   not printed, too large, not interesting enough */

I wonder if you'd be better to add a blank line above, then put the
comment on its own line, i.e:

 /* modified "root" for planning the subquery; not printed, too large,
not interesting enough */
PlannerInfo *subroot pg_node_attr(readwrite_ignore);

3. My biggest concern with this patch is it introducing some change in
behaviour with node copy/equal/read/write.  I spent some time in my
diff tool comparing the files the Perl script built to the existing
code.  Unfortunately, that job is pretty hard due to various order
changes in the outputted functions.  I wonder if it's worth making a
pass in master and changing the function order to match what the
script outputs so that a proper comparison can be done just before
committing the patch.   The problem I see is that master is currently
a very fast-moving target and a detailed comparison would be much
easier to do if the functions were in the same order. I'd be a bit
worried that someone might commit something that requires some special
behaviour and that commit goes in sometime between when you've done a
detailed and when you commit the full patch.

Although, perhaps you've just been copying and pasting code into the
correct order before comparing, which might be good enough if it's
simple enough to do.

I've not really done any detailed review of the Perl code. I'm not the
best person for that, but I do feel like the important part is making
sure the outputted files logically match the existing files.

Also, I'm quite keen to see this work make it into v15.  Do you think
you'll get time to do that? Thanks for working on it.

David

  29. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-25T13:08:32Z

    On 24.03.22 22:57, David Rowley wrote:
>   * Unknown attributes are ignored.  Some additional attributes are used for
>   * special "hack" cases.
> 
> I think these really should all be documented.  If someone needs to
> use one of these hacks then they're going to need to trawl through
> Perl code to see if you've implemented something that matches the
> requirements.  I'd personally rather not have to look at the Perl code
> to find out which attributes I need to use for my new field. I'd bet
> I'm not the only one.

The only such hacks are the three path_hack[1-3] cases that correspond 
to the current _outPathInfo().  I've been thinking long and hard about 
how to generalize any of these but couldn't come up with much yet.  I 
suppose we could replace the names "path_hackN" with something more 
descriptive like "reloptinfo_light" and document those in nodes.h, which 
might address your concern on paper.  But I think you'd still need to 
understand all of that by looking at the definition of Path and its 
uses, so documenting those in nodes.h wouldn't really help, I think. 
Other ideas welcome.

> 2. Some of these comment lines have become pretty long after having
> added the attribute macro.
> 
> e.g.
> 
> PlannerInfo *subroot pg_node_attr(readwrite_ignore); /* modified
> "root" for planning the subquery;
>     not printed, too large, not interesting enough */
> 
> I wonder if you'd be better to add a blank line above, then put the
> comment on its own line, i.e:
> 
>   /* modified "root" for planning the subquery; not printed, too large,
> not interesting enough */
> PlannerInfo *subroot pg_node_attr(readwrite_ignore);

Yes, my idea was to make a separate patch first that reformats many of 
the structs and comments in that way.

> 3. My biggest concern with this patch is it introducing some change in
> behaviour with node copy/equal/read/write.  I spent some time in my
> diff tool comparing the files the Perl script built to the existing
> code.  Unfortunately, that job is pretty hard due to various order
> changes in the outputted functions.  I wonder if it's worth making a
> pass in master and changing the function order to match what the
> script outputs so that a proper comparison can be done just before
> committing the patch.

Just reordering won't really help.  The content of the functions will be 
different, for example because nodes that include Path will include its 
fields inline instead of calling out to _outPathInfo().

IMO, the confirmation that it works is in COPY_PARSE_PLAN_TREES etc.

> The problem I see is that master is currently
> a very fast-moving target and a detailed comparison would be much
> easier to do if the functions were in the same order. I'd be a bit
> worried that someone might commit something that requires some special
> behaviour and that commit goes in sometime between when you've done a
> detailed and when you commit the full patch.

> Also, I'm quite keen to see this work make it into v15.  Do you think
> you'll get time to do that? Thanks for working on it.

My thinking right now is to wait for the PG16 branch to open and then 
consider putting it in early.  That would avoid creating massive 
conflicts with concurrent patches that change node types, and it would 
also relax some concerns about undiscovered behavior changes.

If there is interest in getting it into PG15, I do have capacity to work 
on it.  But in my estimation, this feature is more useful for future 
development, so squeezing in just before feature freeze wouldn't provide 
additional benefit.

  30. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-03-25T13:32:44Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 24.03.22 22:57, David Rowley wrote:
>> Also, I'm quite keen to see this work make it into v15.  Do you think
>> you'll get time to do that? Thanks for working on it.

> My thinking right now is to wait for the PG16 branch to open and then 
> consider putting it in early.

+1.  However, as noted by David (and I think I made similar points awhile
ago), the patch could still use a lot of mop-up work.  It'd be prudent to
continue working on it so it will actually be ready to go when the branch
is made.

			regards, tom lane

  31. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-03-25T15:20:05Z

    On 25.03.22 14:32, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> On 24.03.22 22:57, David Rowley wrote:
>>> Also, I'm quite keen to see this work make it into v15.  Do you think
>>> you'll get time to do that? Thanks for working on it.
> 
>> My thinking right now is to wait for the PG16 branch to open and then
>> consider putting it in early.
> 
> +1.  However, as noted by David (and I think I made similar points awhile
> ago), the patch could still use a lot of mop-up work.  It'd be prudent to
> continue working on it so it will actually be ready to go when the branch
> is made.

The v5 patch was intended to address all the comments you made in your 
Feb. 14 mail.  I'm not aware of any open issues from that.

  32. Re: automatically generating node support functions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-04-19T11:40:42Z

    I rebased this mostly out of curiousity.  I fixed some smallish
conflicts and fixed a typedef problem new in JSON support; however, even
with these fixes it doesn't compile, because JsonPathSpec uses a novel
typedef pattern that apparently will need bespoke handling in the
gen_nodes_support.pl script.  It seemed better to post this even without
that, though.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)

  33. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-19T14:39:21Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> I rebased this mostly out of curiousity.  I fixed some smallish
> conflicts and fixed a typedef problem new in JSON support; however, even
> with these fixes it doesn't compile, because JsonPathSpec uses a novel
> typedef pattern that apparently will need bespoke handling in the
> gen_nodes_support.pl script.  It seemed better to post this even without
> that, though.

Maybe we should fix JsonPathSpec to be less creative while we
still can?  It's not real clear to me why that typedef even exists,
rather than using a String node, or just a plain char * field.

			regards, tom lane

  34. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-04-19T14:53:45Z

    On 19.04.22 16:39, Tom Lane wrote:
> Maybe we should fix JsonPathSpec to be less creative while we
> still can?  It's not real clear to me why that typedef even exists,
> rather than using a String node, or just a plain char * field.

Yeah, let's get rid of it and use char *.

I see in JsonCommon a pathspec is converted to a String node, so it's 
not like JsonPathSpec is some kind of universal representation of the 
thing anyway.

  35. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-05-04T15:45:55Z

    On 19.04.22 13:40, Alvaro Herrera wrote:
> I rebased this mostly out of curiousity.  I fixed some smallish
> conflicts and fixed a typedef problem new in JSON support; however, even
> with these fixes it doesn't compile, because JsonPathSpec uses a novel
> typedef pattern that apparently will need bespoke handling in the
> gen_nodes_support.pl script.  It seemed better to post this even without
> that, though.

I have committed your change to the JsonTableColumnType enum and the 
removal of JsonPathSpec.  Other than that and some whitespace changes, I 
didn't find anything in your 0002 patch that was different from my last 
submitted patch.  Did I miss anything?

  36. Re: automatically generating node support functions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-05-04T16:03:07Z

    On 2022-May-04, Peter Eisentraut wrote:

> I have committed your change to the JsonTableColumnType enum and the removal
> of JsonPathSpec.

Thanks!

> Other than that and some whitespace changes, I didn't find anything in
> your 0002 patch that was different from my last submitted patch.  Did
> I miss anything?

No, I had just fixed one simple conflict IIRC, but I had made no other
changes.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

  37. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-05-23T05:49:52Z

    On 25.03.22 14:08, Peter Eisentraut wrote:
>> 2. Some of these comment lines have become pretty long after having
>> added the attribute macro.
>>
>> e.g.
>>
>> PlannerInfo *subroot pg_node_attr(readwrite_ignore); /* modified
>> "root" for planning the subquery;
>>     not printed, too large, not interesting enough */
>>
>> I wonder if you'd be better to add a blank line above, then put the
>> comment on its own line, i.e:
>>
>>   /* modified "root" for planning the subquery; not printed, too large,
>> not interesting enough */
>> PlannerInfo *subroot pg_node_attr(readwrite_ignore);
> 
> Yes, my idea was to make a separate patch first that reformats many of 
> the structs and comments in that way.

Here is a patch that reformats the relevant (and a few more) comments 
that way.  This has been run through pgindent, so the formatting should 
be stable.

  38. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-03T19:14:09Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> Here is a patch that reformats the relevant (and a few more) comments 
> that way.  This has been run through pgindent, so the formatting should 
> be stable.

Now that that's been pushed, the main patch is of course quite broken.
Are you working on a rebase?

I looked through the last published version of the main patch (Alvaro's
0002 from 2022-04-19), without trying to actually test it, and found
a couple of things that look wrong in the Makefiles:

* AFAICT, the infrastructure for removing the generated files at
"make *clean" is incomplete.  In particular I don't see any code
for removing the symlinks or the associated stamp file during
"make clean".  It looks like the existing header symlinks are
all cleaned up in src/include/Makefile's "clean" rule, so you
could do likewise for these.  Also, the "make maintainer-clean"
infrastructure seems incomplete --- shouldn't src/backend/Makefile's
maintainer-clean rule now also do
	$(MAKE) -C nodes $@
?

* There are some useful comments in backend/utils/Makefile that
I think should be carried over along with the make rules that
(it looks like) you cribbed from there, notably

# fmgr-stamp records the last time we ran Gen_fmgrtab.pl.  We don't rely on
# the timestamps of the individual output files, because the Perl script
# won't update them if they didn't change (to avoid unnecessary recompiles).

# These generated headers must be symlinked into builddir/src/include/,
# using absolute links for the reasons explained in src/backend/Makefile.
# We use header-stamp to record that we've done this because the symlinks
# themselves may appear older than fmgr-stamp.

and something similar to this for the "clean" rule:
# fmgroids.h, fmgrprotos.h, fmgrtab.c, fmgr-stamp, and errcodes.h are in the
# distribution tarball, so they are not cleaned here.


Also, I share David's upthread allergy to the option names "path_hackN"
and to documenting those only inside the conversion script.  I think
the existing text that you moved into the script, such as this bit:

		# We do not print the parent, else we'd be in infinite
		# recursion.  We can print the parent's relids for
		# identification purposes, though.  We print the pathtarget
		# only if it's not the default one for the rel.  We also do
		# not print the whole of param_info, since it's printed via
		# RelOptInfo; it's sufficient and less cluttering to print
		# just the required outer relids.

is perfectly adequate as documentation, it just needs to be somewhere else
(pathnodes.h seems fine, if not nodes.h) and labeled as to exactly which
pg_node_attr option invokes which behavior.

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

			regards, tom lane

  39. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-04T12:23:36Z

    On 03.07.22 21:14, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> Here is a patch that reformats the relevant (and a few more) comments
>> that way.  This has been run through pgindent, so the formatting should
>> be stable.
> 
> Now that that's been pushed, the main patch is of course quite broken.
> Are you working on a rebase?

attached

> * AFAICT, the infrastructure for removing the generated files at
> "make *clean" is incomplete.

I have fixed all the makefiles per your suggestions.

> and something similar to this for the "clean" rule:
> # fmgroids.h, fmgrprotos.h, fmgrtab.c, fmgr-stamp, and errcodes.h are in the
> # distribution tarball, so they are not cleaned here.

Except this one, since there is no clean rule.  I think seeing that 
files are listed under a maintainer-clean target conveys that same message.

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

I'll look into that again.

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

good point

  40. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-04T16:59:20Z

    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

  41. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-06T00:54:46Z

    ... BTW, I thought of a consideration that we probably need some
answer for.  As far as I can see, the patch assigns NodeTag values
sequentially in the order it sees the struct declarations in the
input files; an order that doesn't have a lot to do with our past
practice.  The problem with that is that it's next door to impossible
to control the tag value assigned to any one struct.  During normal
development that's not a big deal, but what if we need to add a
node struct in a released branch?  As nodes.h observes already,

 * Note that inserting or deleting node types changes the numbers of other
 * node types later in the list.  This is no problem during development, since
 * the node numbers are never stored on disk.  But don't do it in a released
 * branch, because that would represent an ABI break for extensions.

We used to have the option of sticking new nodetags at the end of
the list in this situation, but we won't anymore.

It might be enough to invent a struct-level attribute allowing
manual assignment of node tags, ie

typedef struct MyNewNode pg_node_attr(nodetag=466)

where it'd be the programmer's responsibility to pick a nonconflicting
tag number.  We'd only ever use that in ABI-frozen branches, so
manual assignment of the tag value should be workable.

Anyway, this isn't something we have to have before committing,
but I think we're going to need it at some point.

			regards, tom lane

  42. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-06T10:28:31Z

    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

  43. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-06T10:30:49Z

    On 06.07.22 02:54, Tom Lane wrote:
> It might be enough to invent a struct-level attribute allowing
> manual assignment of node tags, ie
> 
> typedef struct MyNewNode pg_node_attr(nodetag=466)
> 
> where it'd be the programmer's responsibility to pick a nonconflicting
> tag number.  We'd only ever use that in ABI-frozen branches, so
> manual assignment of the tag value should be workable.

Yes, I'm aware of this issue, and that was also more or less my idea.

(Well, before the introduction of per-struct attributes, I was thinking 
about parsing nodes.h to see if the tag is listed explicitly.  But this 
is probably better.)

  44. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-06T20:46:27Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> [ v7-0001-Automatically-generate-node-support-functions.patch ]

I have gone through this and made some proposed changes (attached),
and I think it is almost committable.  There is one nasty problem
we need a solution to, which is that pgindent is not at all on board
with this idea of attaching node attrs to typedefs.  It pushes them
to the next line, like this:

@@ -691,7 +709,8 @@
 	 (rel)->reloptkind == RELOPT_OTHER_JOINREL || \
 	 (rel)->reloptkind == RELOPT_OTHER_UPPER_REL)
 
-typedef struct RelOptInfo pg_node_attr(no_copy_equal, no_read)
+typedef struct RelOptInfo
+pg_node_attr(no_copy_equal, no_read)
 {
 	NodeTag		type;
 
which is already enough to break the simplistic parsing in
gen_node_support.pl.  Now, we could fix that parsing logic to deal
with this layout, but this also seems to change pgindent's opinion
of whether the subsequent braced material is part of a typedef or a
function.  That results in it injecting a lot of vertical space
that wasn't there before, which is annoying.

I experimented a bit and found that we could do it this way:

 typedef struct RelOptInfo
 {
+	pg_node_attr(no_copy_equal, no_read)
+
 	NodeTag		type;

without (AFAICT) confusing pgindent, but I've not tried to adapt
the perl script or the code to that style.

Anyway, besides that, I have some comments that I've implemented
in the attached delta patch.

* After further thought I'm okay with your theory that attaching
special copy/equal rules to specific field types is appropriate.
We might at some point want the pg_node_attr(copy_as_scalar)
approach too, but we can always add that later.  However, I thought
some more comments about it were needed in the *nodes.h files,
so I added those.  (My general feeling about this is that if
anyone needs to look into gen_node_support.pl to understand how
the backend works, we've failed at documentation.)

* As written, the patch created equal() support for all Plan structs,
which is quite a bit of useless code bloat.  I solved this by
separating no_copy and no_equal properties, so that we could mark
Plan as no_equal while still having copy support.

* I did not like the semantics of copy_ignore one bit: it was
relying on the pre-zeroing behavior of makeNode() to be sane at
all, and I don't want that to be a requirement.  (I foresee
wanting to flat-copy node contents and turn COPY_SCALAR_FIELD
into a no-op.)  I replaced it with copy_as(VALUE) to provide
better-defined semantics.

* Likewise, read_write_ignore left the contents of the field after
reading too squishy for me.  I invented read_as(VALUE) parallel
to copy_as() to fix the semantics, and added a check that you
can only use read_write_ignore if the struct is no_read or
you provide read_as().  (This could be factored differently
of course.)

* I threw in a bunch more no_read markers to bring the readfuncs.c
contents into closer alignment with what we have today.  Maybe
there is an argument for accepting that code bloat, but it's a
discussion to have later.  In any case, most of the pathnodes.h
structs HAVE to be marked no_read because there's no sane way
to reconstruct them from outfuncs output.

* I got rid of the code that stripped underscores from outfuncs
struct labels.  That seemed like an entirely unnecessary
behavioral change.

* FWIW, I'm okay with the question about

 		# XXX Previously, for subtyping, only the leaf field name is
 		# used. Ponder whether we want to keep it that way.

I thought that it might make the output too cluttered, but after
some study of the results from printing plans and planner data
structures, it's not a big addition, and indeed I kind of like it.

* Fixed a bug in write_only_req_outer code.

* Made Plan and Join into abstract nodes.

Anyway, if we can fix the impedance mismatch with pgindent,
I think this is committable.  There is a lot of follow-on
work that could be considered, but I'd like to get the present
changes in place ASAP so that other patches can be rebased
onto something stable.

I've attached a delta patch, and also repeated v7 so as not
to confuse the cfbot.

			regards, tom lane

  45. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-06T21:46:27Z

    I wrote:
> I have gone through this and made some proposed changes (attached),
> and I think it is almost committable.

I see from the cfbot that it now needs to be taught about RelFileNumber...

			regards, tom lane

  46. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-08T12:44:00Z

    On 06.07.22 22:46, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> [ v7-0001-Automatically-generate-node-support-functions.patch ]
> 
> I have gone through this and made some proposed changes (attached),

I have included those.

> and I think it is almost committable.  There is one nasty problem
> we need a solution to, which is that pgindent is not at all on board
> with this idea of attaching node attrs to typedefs.  It pushes them
> to the next line, like this:
> 
> @@ -691,7 +709,8 @@
>   	 (rel)->reloptkind == RELOPT_OTHER_JOINREL || \
>   	 (rel)->reloptkind == RELOPT_OTHER_UPPER_REL)
>   
> -typedef struct RelOptInfo pg_node_attr(no_copy_equal, no_read)
> +typedef struct RelOptInfo
> +pg_node_attr(no_copy_equal, no_read)
>   {
>   	NodeTag		type;

I have found that putting the attributes at the end of the struct 
definition, right before the semicolon, works, so I have changed it that 
way.  (This is also where a gcc __attribute__() would go, so it seems 
reasonable.)

The attached patch is stable under pgindent.

Finally, I have updated src/backend/nodes/README a bit.

I realize I've been confused various times about when a catversion 
change is required when changing nodes.  (I think the bump in 251154bebe 
was probably not needed.)  I have tried to put that in the README.  This 
could perhaps be expanded.

I think for this present patch, I would do a catversion bump, just to be 
sure, in case some of the printed node fields are different now.

It was also my plan to remove the #ifdef OBSOLETE sections in a separate 
commit right after, just to be clear.

Final thoughts?

  47. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-08T13:52:46Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 06.07.22 22:46, Tom Lane wrote:
>> ...  There is one nasty problem
>> we need a solution to, which is that pgindent is not at all on board
>> with this idea of attaching node attrs to typedefs.

> I have found that putting the attributes at the end of the struct 
> definition, right before the semicolon, works, so I have changed it that 
> way.  (This is also where a gcc __attribute__() would go, so it seems 
> reasonable.)

That was the first solution I thought of as well, but I do not like
it from a cosmetic standpoint.  The node attributes are a pretty
critical part of the node definition (especially "abstract"),
so shoving them to the very end is not helpful for readability.
IMO anyway.

> I think for this present patch, I would do a catversion bump, just to be 
> sure, in case some of the printed node fields are different now.

I know from comparing the code that some printed node tags have
changed, and so has the print order of some fields.  It might be
that none of those changes are in node types that can appear in
stored rules --- but I'm not sure, so I concur that doing a
catversion bump for this commit is advisable.

> It was also my plan to remove the #ifdef OBSOLETE sections in a separate 
> commit right after, just to be clear.

Yup, my thought as well.  There are a few other mop-up things
I want to do shortly after (e.g. add copyright-notice headers
to the emitted files), but let's wait for the buildfarm's
opinion of the main commit first.

> Final thoughts?

I'll re-read the patch today, but how open are you to putting the
struct attributes at the top?  I'm willing to do the legwork.

			regards, tom lane

  48. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-08T15:46:28Z

    On 08.07.22 15:52, Tom Lane wrote:
> I'll re-read the patch today, but how open are you to putting the
> struct attributes at the top?  I'm willing to do the legwork.

I agree near the top would be preferable.  I think it would even be 
feasible to parse the whole thing if pgindent split it across lines.  I 
sort of tried to maintain the consistency with C/C++ attributes like 
__attribute__ and [[attribute]], hoping that that would confuse other 
tooling the least.  Feel free to experiment further.

  49. Re: automatically generating node support functions

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2022-07-08T16:45:34Z

    While going over this patch, I noticed that I forgot to add support for
XidList in copyfuncs.c.  OK if I push this soon quickly?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

  50. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-08T18:15:57Z

    Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> While going over this patch, I noticed that I forgot to add support for
> XidList in copyfuncs.c.  OK if I push this soon quickly?

Yeah, go ahead, that part of copyfuncs is still going to be manually
maintained, so we need the fix.

What about equalfuncs etc?

			regards, tom lane

  51. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-08T20:03:45Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 08.07.22 15:52, Tom Lane wrote:
>> I'll re-read the patch today, but how open are you to putting the
>> struct attributes at the top?  I'm willing to do the legwork.

> I agree near the top would be preferable.  I think it would even be 
> feasible to parse the whole thing if pgindent split it across lines.  I 
> sort of tried to maintain the consistency with C/C++ attributes like 
> __attribute__ and [[attribute]], hoping that that would confuse other 
> tooling the least.  Feel free to experiment further.

I went through and did that, and I do like this way better.

I did a final round of review, and found a few cosmetic things, as
well as serious bugs in the code I'd contributed for copy_as/read_as:
they did the wrong thing for VALUE of "0" because I should have
written "if (defined $foo)" not "if ($foo)".  Also, read_as did
not generate correct code for the case where we don't have
read_write_ignore; in that case we have to read the value outfuncs.c
wrote and then override it.

0001 attached repeats your v8 (to please the cfbot).

0002 includes some suggestions for the README file as well as
cosmetic and not-so-cosmetic fixes for gen_node_support.pl.

0003 moves the node-level attributes as discussed.

Lastly, I think we ought to apply pgperltidy to the Perl code.
In case you don't have that installed, 0004 is the diffs I got.

I think this is ready to go (don't forget the catversion bump).

			regards, tom lane

  52. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-08T20:16:07Z

    I wrote:
> 0003 moves the node-level attributes as discussed.

Meh.  Just realized that I forgot to adjust the commentary in nodes.h
about where to put node attributes.

Maybe like

- * Attributes can be attached to a node as a whole (the attribute
- * specification must be at the end of the struct or typedef, just before the
- * semicolon) or to a specific field (must be at the end of the line).  The
+ * Attributes can be attached to a node as a whole (place the attribute
+ * specification on the first line after the struct's opening brace)
+ * or to a specific field (place it at the end of that field's line).  The
  * argument is a comma-separated list of attributes.  Unrecognized attributes
  * cause an error.

			regards, tom lane

  53. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-09T14:37:22Z

    On 08.07.22 22:03, Tom Lane wrote:
> I think this is ready to go (don't forget the catversion bump).

This is done now, after a brief vpath-shaped scare from the buildfarm 
earlier today.

  54. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-09T15:03:54Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 08.07.22 22:03, Tom Lane wrote:
>> I think this is ready to go (don't forget the catversion bump).

> This is done now, after a brief vpath-shaped scare from the buildfarm 
> earlier today.

Doh ... never occurred to me either to try that :-(

			regards, tom lane

  55. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-09T16:58:50Z

    Here's some follow-on patches, as I threatened yesterday.

0001 adds some material to nodes/README in hopes of compensating for
a couple of removed comments.

0002 fixes gen_node_support.pl's rather badly broken error reporting.
As it stands, it always says that an error is on line 1 of the respective
input file, because it relies for that on perl's "$." which is only
workable when we are reading the file a line at a time.  The scheme
of sucking in the entire file so that we can suppress multi-line C
comments easily doesn't play well with that.  I concluded that the
best way to fix that was to adjust the C-comment-deletion code to
preserve any newlines within a comment, and then we can easily count
lines manually.  The new C-comment-deletion code is a bit brute-force;
maybe there is a better way?

0003 adds boilerplate header comments to the output files, using
wording pretty similar to those written by genbki.pl.

0004 fixes things so that we don't leave a mess of temporary files
if the script dies partway through.  genbki.pl perhaps could use
this as well, but my experience is that genbki usually reports any
errors before starting to write files.  gen_node_support.pl not
so much --- I had to manually clean up the mess several times while
reviewing/testing.

			regards, tom lane

  56. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-10T21:46:22Z

    Hi,

On 2022-07-09 16:37:22 +0200, Peter Eisentraut wrote:
> On 08.07.22 22:03, Tom Lane wrote:
> > I think this is ready to go (don't forget the catversion bump).
> 
> This is done now, after a brief vpath-shaped scare from the buildfarm
> earlier today.

I was just rebasing meson ontop of this and was wondering whether the input
filenames were in a particular order:


node_headers = \
	nodes/nodes.h \
	nodes/execnodes.h \
	nodes/plannodes.h \
	nodes/primnodes.h \
	nodes/pathnodes.h \
	nodes/extensible.h \
	nodes/parsenodes.h \
	nodes/replnodes.h \
	nodes/value.h \
	commands/trigger.h \
	commands/event_trigger.h \
	foreign/fdwapi.h \
	access/amapi.h \
	access/tableam.h \
	access/tsmapi.h \
	utils/rel.h \
	nodes/supportnodes.h \
	executor/tuptable.h \
	nodes/lockoptions.h \
	access/sdir.h

Can we either order them alphabetically or add a comment explaining the order?

- Andres

  57. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-10T23:09:57Z

    Andres Freund <andres@anarazel.de> writes:
> I was just rebasing meson ontop of this and was wondering whether the input
> filenames were in a particular order:

That annoyed me too.  I think it's sensible to list the "main" input
files first, but I'd put them in our traditional pipeline order:

> 	nodes/nodes.h \
> 	nodes/primnodes.h \
> 	nodes/parsenodes.h \
> 	nodes/pathnodes.h \
> 	nodes/plannodes.h \
> 	nodes/execnodes.h \

The rest could probably be alphabetical.  I was also wondering if
all of them really need to be read at all --- I'm unclear on what
access/sdir.h is contributing, for example.

			regards, tom lane

  58. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-11T14:09:24Z

    On 11.07.22 01:09, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
>> I was just rebasing meson ontop of this and was wondering whether the input
>> filenames were in a particular order:

First, things used by later files need to be found in earlier files.  So 
that constrains the order a bit.

Second, the order of the files determines the ordering of the output. 
The current order of the files reflects approximately the order how the 
manual code was arranged.  That could be changed.  We could also just 
sort the node types in the script and dump out everything alphabetically.

> That annoyed me too.  I think it's sensible to list the "main" input
> files first, but I'd put them in our traditional pipeline order:
> 
>> 	nodes/nodes.h \
>> 	nodes/primnodes.h \
>> 	nodes/parsenodes.h \
>> 	nodes/pathnodes.h \
>> 	nodes/plannodes.h \
>> 	nodes/execnodes.h \

The seems worth trying out.

> The rest could probably be alphabetical.  I was also wondering if
> all of them really need to be read at all --- I'm unclear on what
> access/sdir.h is contributing, for example.

could not handle type "ScanDirection" in struct "IndexScan" field 
"indexorderdir"

  59. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T14:22:59Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 11.07.22 01:09, Tom Lane wrote:
>> The rest could probably be alphabetical.  I was also wondering if
>> all of them really need to be read at all --- I'm unclear on what
>> access/sdir.h is contributing, for example.

> could not handle type "ScanDirection" in struct "IndexScan" field 
> "indexorderdir"

Ah, I see.  Still, we could also handle that with

push @enum_types, qw(ScanDirection);

which would be exactly one place that needs to know about this, rather
than the three (soon to be four) places that know that access/sdir.h
needs to be read and then mostly ignored.

			regards, tom lane

  60. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T15:37:39Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 11.07.22 01:09, Tom Lane wrote:
>> Andres Freund <andres@anarazel.de> writes:
> I was just rebasing meson ontop of this and was wondering whether the input
> filenames were in a particular order:

> First, things used by later files need to be found in earlier files.  So 
> that constrains the order a bit.

Yeah, the script needs to see supertype nodes before subtype nodes,
else it will not realize that the subtypes are nodes at all.  However,
there is not very much cross-header-file subtyping.  I experimented with
rearranging the input-file order, and found that the *only* thing that
breaks it is to put primnodes.h after pathnodes.h (which fails because
PlaceHolderVar is a subtype of Expr).  You don't even need nodes.h to be
first, which astonished me initially, but then I realized that both
NodeTag and struct Node are special-cased in gen_node_support.pl,
so we know enough to get by even before reading nodes.h.

More generally, the main *nodes.h files themselves are arranged in
pipeline order, eg parsenodes.h #includes primnodes.h.  So that seems
to be a pretty safe thing to rely on even if we grow more cross-header
subtyping cases later.  But I'd vote for putting the incidental files
in alphabetical order.

> Second, the order of the files determines the ordering of the output. 
> The current order of the files reflects approximately the order how the 
> manual code was arranged.  That could be changed.  We could also just 
> sort the node types in the script and dump out everything alphabetically.

+1 for sorting alphabetically.  I experimented with that and it's a
really trivial change.

			regards, tom lane

  61. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T16:07:09Z

    I wrote:
> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
>> could not handle type "ScanDirection" in struct "IndexScan" field 
>> "indexorderdir"

> Ah, I see.  Still, we could also handle that with
> push @enum_types, qw(ScanDirection);

I tried that, and it does work.  The only other input file we could
get rid of that way is nodes/lockoptions.h, which likewise contributes
only a couple of enum type names.  Not sure it's worth messing with
--- both ways seem crufty, though for different reasons.

			regards, tom lane

  62. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T16:14:04Z

    Hi,

On 2022-07-11 12:07:09 -0400, Tom Lane wrote:
> I wrote:
> > Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> >> could not handle type "ScanDirection" in struct "IndexScan" field
> >> "indexorderdir"
>
> > Ah, I see.  Still, we could also handle that with
> > push @enum_types, qw(ScanDirection);
>
> I tried that, and it does work.  The only other input file we could
> get rid of that way is nodes/lockoptions.h, which likewise contributes
> only a couple of enum type names.

Kinda wonder if those headers are even worth having. Plenty other enums in
primnodes.h.


> Not sure it's worth messing with --- both ways seem crufty, though for
> different reasons.

Not sure either.

Greetings,

Andres Freund

  63. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T17:57:38Z

    I wrote:
>> Andres Freund <andres@anarazel.de> writes:
>>> I was just rebasing meson ontop of this and was wondering whether the input
>>> filenames were in a particular order:

Pushed a patch to make that a bit less random-looking.

> +1 for sorting alphabetically.  I experimented with that and it's a
> really trivial change.

I had second thoughts about that, after noticing that alphabetizing
the NodeTag enum increased the backend's size by 20K or so.  Presumably
that's telling us that a bunch of switch statements got less dense,
which might possibly cause performance issues thanks to poorer cache
behavior or the like.  Maybe it's still appropriate to do, but it's
not as open-and-shut as I first thought.

More generally, I'm having second thoughts about the wisdom of
auto-generating the NodeTag enum at all.  With the current setup,
I am absolutely petrified about the risk of silent ABI breakage
thanks to the enum order changing.  In particular, if the meson
build fails to use the same input-file order as the makefile build,
then we will get different enum orders from the two builds, causing
an ABI discrepancy that nobody would notice until we had catastrophic
extension-compatibility issues in the field.

Of course, sorting the tags by name is a simple way to fix that.
But I'm not sure I want to buy into being forced to do it like that,
because of the switch-density question.

So at this point I'm rather attracted to the idea of reverting to
a manually-maintained NodeTag enum.  We know how to avoid ABI
breakage with that, and it's not exactly the most painful part
of adding a new node type.  Plus, that'd remove (most of?) the
need for gen_node_support.pl to deal with "node-tag-only" structs
at all.

Thoughts?

			regards, tom lane

  64. Re: automatically generating node support functions

    Robert Haas <robertmhaas@gmail.com> — 2022-07-11T18:17:44Z

    On Mon, Jul 11, 2022 at 1:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> More generally, I'm having second thoughts about the wisdom of
> auto-generating the NodeTag enum at all.  With the current setup,
> I am absolutely petrified about the risk of silent ABI breakage
> thanks to the enum order changing.  In particular, if the meson
> build fails to use the same input-file order as the makefile build,
> then we will get different enum orders from the two builds, causing
> an ABI discrepancy that nobody would notice until we had catastrophic
> extension-compatibility issues in the field.

I think this is a valid concern, but having it be automatically
generated is awfully handy, so I think it would be nice to find some
way of preserving that.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  65. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T18:29:15Z

    Hi,

On 2022-07-11 13:57:38 -0400, Tom Lane wrote:
> More generally, I'm having second thoughts about the wisdom of
> auto-generating the NodeTag enum at all.  With the current setup,
> I am absolutely petrified about the risk of silent ABI breakage
> thanks to the enum order changing.  In particular, if the meson
> build fails to use the same input-file order as the makefile build,
> then we will get different enum orders from the two builds, causing
> an ABI discrepancy that nobody would notice until we had catastrophic
> extension-compatibility issues in the field.

Ugh, yes. And it already exists due to Solution.pm, although that's perhaps
less likely to be encountered "in the wild".

Additionally, I think we've had to add tags to the enum in minor releases
before and I'm afraid this now would end up looking even more awkward?


> Of course, sorting the tags by name is a simple way to fix that.
> But I'm not sure I want to buy into being forced to do it like that,
> because of the switch-density question.
> 
> So at this point I'm rather attracted to the idea of reverting to
> a manually-maintained NodeTag enum.

+0.5 - there might be a better solution to this, but I'm not immediately
seeing it.

Greetings,

Andres Freund

  66. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T19:54:22Z

    Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jul 11, 2022 at 1:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More generally, I'm having second thoughts about the wisdom of
>> auto-generating the NodeTag enum at all.  With the current setup,
>> I am absolutely petrified about the risk of silent ABI breakage
>> thanks to the enum order changing.

> I think this is a valid concern, but having it be automatically
> generated is awfully handy, so I think it would be nice to find some
> way of preserving that.

Agreed.  The fundamental problem seems to be that each build toolchain
has its own source of truth about the file processing order, but we now
see that there had better be only one.  We could make the sole source
of truth about that be gen_node_support.pl itself, I think.

We can't simply move the file list into gen_node_support.pl, because
(a) the build system has to know about the dependencies involved, and
(b) gen_node_support.pl wouldn't know what to do in VPATH situations.
However, we could have gen_node_support.pl contain a canonical list
of the files it expects to be handed, and make it bitch if its
arguments don't match that.

That's ugly I admit, but the set of files of interest doesn't change
so often that maintaining one additional copy would be a big problem.

Anybody got a better idea?

			regards, tom lane

  67. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T20:04:00Z

    Andres Freund <andres@anarazel.de> writes:
> Additionally, I think we've had to add tags to the enum in minor releases
> before and I'm afraid this now would end up looking even more awkward?

Peter and I already had a discussion about that upthread --- we figured
that if there's a way to manually assign a nodetag's number, you could use
that option when you have to add a tag in a stable branch.  We didn't
actually build out that idea, but I can go do that, if we can solve the
more fundamental problem of keeping the autogenerated numbers stable.

One issue with that idea, of course, is that you have to remember to do
it like that when back-patching a node addition.  Ideally there'd be
something that'd carp if the last autogenerated tag moves in a stable
branch, but I'm not very sure where to put that.

			regards, tom lane

  68. Re: automatically generating node support functions

    Robert Haas <robertmhaas@gmail.com> — 2022-07-11T20:17:28Z

    On Mon, Jul 11, 2022 at 3:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> We can't simply move the file list into gen_node_support.pl, because
> (a) the build system has to know about the dependencies involved, and
> (b) gen_node_support.pl wouldn't know what to do in VPATH situations.
> However, we could have gen_node_support.pl contain a canonical list
> of the files it expects to be handed, and make it bitch if its
> arguments don't match that.

Sorry if I'm being dense, but why do we have to duplicate the list of
files instead of having gen_node_support.pl just sort whatever list
the build system provides to it?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

  69. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T20:17:55Z

    Hi,

On 2022-07-11 15:54:22 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> > On Mon, Jul 11, 2022 at 1:57 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> More generally, I'm having second thoughts about the wisdom of
> >> auto-generating the NodeTag enum at all.  With the current setup,
> >> I am absolutely petrified about the risk of silent ABI breakage
> >> thanks to the enum order changing.
> 
> > I think this is a valid concern, but having it be automatically
> > generated is awfully handy, so I think it would be nice to find some
> > way of preserving that.
> 
> Agreed.  The fundamental problem seems to be that each build toolchain
> has its own source of truth about the file processing order, but we now
> see that there had better be only one.  We could make the sole source
> of truth about that be gen_node_support.pl itself, I think.
> 
> We can't simply move the file list into gen_node_support.pl, because

> (a) the build system has to know about the dependencies involved

Meson has builtin support for tools like gen_node_support.pl reporting which
files they've read and then to use those as dependencies. It'd not be a lot of
effort to open-code that with make either.

Doesn't look like we have dependency handling in Solution.pm?


> (b) gen_node_support.pl wouldn't know what to do in VPATH situations.

We could easily add a --include-path argument or such. That'd be trivial to
set for all of the build solutions.

FWIW, for meson I already needed to add an option to specify the location of
output files (since scripts are called from the root of the build directory).

Greetings,

Andres Freund

  70. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T20:26:46Z

    Hi,

On 2022-07-11 16:17:28 -0400, Robert Haas wrote:
> On Mon, Jul 11, 2022 at 3:54 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > We can't simply move the file list into gen_node_support.pl, because
> > (a) the build system has to know about the dependencies involved, and
> > (b) gen_node_support.pl wouldn't know what to do in VPATH situations.
> > However, we could have gen_node_support.pl contain a canonical list
> > of the files it expects to be handed, and make it bitch if its
> > arguments don't match that.
> 
> Sorry if I'm being dense, but why do we have to duplicate the list of
> files instead of having gen_node_support.pl just sort whatever list
> the build system provides to it?

Because right now there's two buildsystems already (look at
Solution.pm). Looks like we'll briefly have three, then two again.

Greetings,

Andres Freund

  71. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T20:36:02Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-07-11 16:17:28 -0400, Robert Haas wrote:
>> Sorry if I'm being dense, but why do we have to duplicate the list of
>> files instead of having gen_node_support.pl just sort whatever list
>> the build system provides to it?

> Because right now there's two buildsystems already (look at
> Solution.pm). Looks like we'll briefly have three, then two again.

There are two things we need: (1) be sure that the build system knows
about all the files of interest, and (2) process them in the correct
order, which is *not* alphabetical.  "Just sort" won't achieve either.

			regards, tom lane

  72. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T20:38:05Z

    Andres Freund <andres@anarazel.de> writes:
> On 2022-07-11 15:54:22 -0400, Tom Lane wrote:
>> We can't simply move the file list into gen_node_support.pl, because
>> (a) the build system has to know about the dependencies involved

> Meson has builtin support for tools like gen_node_support.pl reporting which
> files they've read and then to use those as dependencies. It'd not be a lot of
> effort to open-code that with make either.

If you want to provide code for that, sure, but I don't know how to do it.

>> (b) gen_node_support.pl wouldn't know what to do in VPATH situations.

> We could easily add a --include-path argument or such. That'd be trivial to
> set for all of the build solutions.

True.

			regards, tom lane

  73. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T21:18:57Z

    I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Additionally, I think we've had to add tags to the enum in minor releases
>> before and I'm afraid this now would end up looking even more awkward?

> Peter and I already had a discussion about that upthread --- we figured
> that if there's a way to manually assign a nodetag's number, you could use
> that option when you have to add a tag in a stable branch.  We didn't
> actually build out that idea, but I can go do that, if we can solve the
> more fundamental problem of keeping the autogenerated numbers stable.

> One issue with that idea, of course, is that you have to remember to do
> it like that when back-patching a node addition.  Ideally there'd be
> something that'd carp if the last autogenerated tag moves in a stable
> branch, but I'm not very sure where to put that.

One way to do it is to provide logic in gen_node_support.pl to check
that, and activate that logic only in back branches.  If we make that
part of the branch-making procedure, we'd not forget to do it.

Proposed patch attached.

			regards, tom lane

  74. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T21:37:55Z

    Hi,

On 2022-07-11 16:38:05 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2022-07-11 15:54:22 -0400, Tom Lane wrote:
> >> We can't simply move the file list into gen_node_support.pl, because
> >> (a) the build system has to know about the dependencies involved
> 
> > Meson has builtin support for tools like gen_node_support.pl reporting which
> > files they've read and then to use those as dependencies. It'd not be a lot of
> > effort to open-code that with make either.
> 
> If you want to provide code for that, sure, but I don't know how to do it.

It'd basically be something like a --deps option providing a path to a file
(e.g. .deps/nodetags.Po) where the script would emit something roughly
equivalent to

path/to/nodetags.h: path/to/nodes/nodes.h
path/to/nodetags.h: path/to/nodes/primnodes.h
...
path/to/readfuncs.c: path/to/nodetags.h

It might or might not make sense to output this as one rule instead of
multiple ones.

I think our existing dependency support would do the rest.


We'd still need a dependency on node-support-stamp (or nodetags.h or ...), to
trigger the first invocation of gen_node_support.pl.


I don't think it's worth worrying about this not working reliably for non
--enable-depend builds, there's a lot more broken than this. But it might be a
bit annoying to deal with either a) creating the .deps directory even without
--enable-depend, or b) specifying --deps only optionally.

I can give it a go if this doesn't sound insane.

Greetings,

Andres Freund

  75. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T22:09:15Z

    Andres Freund <andres@anarazel.de> writes:
> I don't think it's worth worrying about this not working reliably for non
> --enable-depend builds, there's a lot more broken than this.

Well, *I* care about that, and I won't stand for making the
non-enable-depend case significantly more broken than it is now.
In particular, what you're proposing would mean that "make clean"
followed by rebuild wouldn't be sufficient to update everything
anymore; you'd have to resort to maintainer-clean or "git clean -dfx"
after touching any node definition file, else gen_node_support.pl
would not get re-run.  Up with that I will not put.

			regards, tom lane

  76. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T22:27:58Z

    Hi,

On 2022-07-11 18:09:15 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I don't think it's worth worrying about this not working reliably for non
> > --enable-depend builds, there's a lot more broken than this.
>
> Well, *I* care about that, and I won't stand for making the
> non-enable-depend case significantly more broken than it is now.
>
> In particular, what you're proposing would mean that "make clean"
> followed by rebuild wouldn't be sufficient to update everything
> anymore; you'd have to resort to maintainer-clean or "git clean -dfx"
> after touching any node definition file, else gen_node_support.pl
> would not get re-run.  Up with that I will not put.

I'm not sure it'd have to mean that, but we could just implement the
dependency stuff independent of the existing autodepend logic. Something like:

# ensure that dependencies of
-include gen_node_support.pl.deps
node-support-stamp: gen_node_support.pl
	$(PERL) --deps $^.deps $^

I guess we'd have to distribute gen_node_support.pl.deps to make this work in
tarball builds - which is probably fine? Not really different than including
stamp files.

I'm not entirely sure how well either the existing or the sketch above works
when doing a VPATH build using tarball sources, and updating the files.

Greetings,

Andres Freund

  77. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-11T22:39:44Z

    Andres Freund <andres@anarazel.de> writes:
> I'm not entirely sure how well either the existing or the sketch above works
> when doing a VPATH build using tarball sources, and updating the files.

Seems like an awful lot of effort to avoid having multiple copies
of the file list.  I think we should just do what I sketched earlier,
ie put the master list into gen_node_support.pl and have it cross-check
that against its command line.  If the meson system can avoid having
its own copy of the list, great; but I don't feel like we have to make
that happen for the makefiles or Solution.pm.

			regards, tom lane

  78. Re: automatically generating node support functions

    Andres Freund <andres@anarazel.de> — 2022-07-11T22:41:30Z

    On 2022-07-11 18:39:44 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > I'm not entirely sure how well either the existing or the sketch above works
> > when doing a VPATH build using tarball sources, and updating the files.
> 
> Seems like an awful lot of effort to avoid having multiple copies
> of the file list.  I think we should just do what I sketched earlier,
> ie put the master list into gen_node_support.pl and have it cross-check
> that against its command line.  If the meson system can avoid having
> its own copy of the list, great; but I don't feel like we have to make
> that happen for the makefiles or Solution.pm.

WFM.

  79. Re: automatically generating node support functions

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-07-12T19:03:47Z

    On 11.07.22 19:57, Tom Lane wrote:
> So at this point I'm rather attracted to the idea of reverting to
> a manually-maintained NodeTag enum.  We know how to avoid ABI
> breakage with that, and it's not exactly the most painful part
> of adding a new node type.

One of the nicer features is that you now get to see the numbers 
assigned to the enum tags, like

     T_LockingClause = 91,
     T_XmlSerialize = 92,
     T_PartitionElem = 93,

so that when you get an error like "unsupported node type: %d", you can 
just look up what it is.

  80. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-12T19:49:11Z

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> On 11.07.22 19:57, Tom Lane wrote:
>> So at this point I'm rather attracted to the idea of reverting to
>> a manually-maintained NodeTag enum.  We know how to avoid ABI
>> breakage with that, and it's not exactly the most painful part
>> of adding a new node type.

> One of the nicer features is that you now get to see the numbers 
> assigned to the enum tags, like

>      T_LockingClause = 91,
>      T_XmlSerialize = 92,
>      T_PartitionElem = 93,

> so that when you get an error like "unsupported node type: %d", you can 
> just look up what it is.

Yeah, I wasn't thrilled about reverting that either.  I think the
defenses I installed in eea9fa9b2 should be sufficient to deal
with the risk.

			regards, tom lane

  81. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-07-14T00:49:39Z

    Just one more thing here ... I really don't like the fact that
gen_node_support.pl's response to unparseable input is to silently
ignore it.  That's maybe tolerable outside a node struct, but
I think we need a higher standard inside.  I experimented with
promoting the commented-out "warn" to "die", and soon learned
that there are two shortcomings:

* We can't cope with the embedded union inside A_Const.
Simplest fix is to move it outside.

* We can't cope with function-pointer fields.  The only real
problem there is that some of them spread across multiple lines,
but really that was a shortcoming we'd have to fix sometime
anyway.

Proposed patch attached.

			regards, tom lane

  82. Re: automatically generating node support functions

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-03T06:21:37Z

    On Wed, Jul 13, 2022 at 12:34 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>

I have a question related to commit 964d01ae90. Today, after getting
the latest code, when I compiled it on my windows machine, it lead to
a compilation error because the outfuncs.funcs.c was not regenerated.
I did the usual steps which I normally perform after getting the
latest code (a) run "perl mkvcbuild.pl" and (b) then build the code
using MSVC. Now, after that, I manually removed "node-support-stamp"
from folder src/backend/nodes/ and re-did the steps and I see that the
outfuncs.funcs.c got regenerated, and the build is also successful. I
see that there is handling to clean the file "node-support-stamp" in
nodes/Makefile but not sure how it works for windows. I think I am
missing something here. Can you please guide me?

-- 
With Regards,
Amit Kapila.

  83. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-03T13:46:44Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> I have a question related to commit 964d01ae90. Today, after getting
> the latest code, when I compiled it on my windows machine, it lead to
> a compilation error because the outfuncs.funcs.c was not regenerated.
> I did the usual steps which I normally perform after getting the
> latest code (a) run "perl mkvcbuild.pl" and (b) then build the code
> using MSVC. Now, after that, I manually removed "node-support-stamp"
> from folder src/backend/nodes/ and re-did the steps and I see that the
> outfuncs.funcs.c got regenerated, and the build is also successful. I
> see that there is handling to clean the file "node-support-stamp" in
> nodes/Makefile but not sure how it works for windows. I think I am
> missing something here. Can you please guide me?

More likely, we need to add something explicit to Mkvcbuild.pm
for this.  I recall that it has stanzas to deal with updating
other autogenerated files; I bet we either missed that or
fat-fingered it for node-support-stamp.

			regards, tom lane

  84. Re: automatically generating node support functions

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-05T12:22:36Z

    On Wed, Aug 3, 2022 at 7:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > I have a question related to commit 964d01ae90. Today, after getting
> > the latest code, when I compiled it on my windows machine, it lead to
> > a compilation error because the outfuncs.funcs.c was not regenerated.
> > I did the usual steps which I normally perform after getting the
> > latest code (a) run "perl mkvcbuild.pl" and (b) then build the code
> > using MSVC. Now, after that, I manually removed "node-support-stamp"
> > from folder src/backend/nodes/ and re-did the steps and I see that the
> > outfuncs.funcs.c got regenerated, and the build is also successful. I
> > see that there is handling to clean the file "node-support-stamp" in
> > nodes/Makefile but not sure how it works for windows. I think I am
> > missing something here. Can you please guide me?
>
> More likely, we need to add something explicit to Mkvcbuild.pm
> for this.  I recall that it has stanzas to deal with updating
> other autogenerated files; I bet we either missed that or
> fat-fingered it for node-support-stamp.
>

I see below logic added by commit which seems to help regenerate the
required files.

+++ b/src/tools/msvc/Solution.pm
@@ -839,6 +839,54 @@ EOF
        close($chs);
    }

+   if (IsNewer(
+           'src/backend/nodes/node-support-stamp',
+           'src/backend/nodes/gen_node_support.pl'))
...
...

Now, in commit 1349d2790b, we didn't change anything in
gen_node_support.pl but changed "typedef struct AggInfo" due to which
we expect the files like outfuncs.funcs.c gets regenerated. However,
as there is no change in gen_node_support.pl, the files didn't get
regenerated.

-- 
With Regards,
Amit Kapila.

  85. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-07T14:49:12Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Wed, Aug 3, 2022 at 7:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> More likely, we need to add something explicit to Mkvcbuild.pm
>> for this.  I recall that it has stanzas to deal with updating
>> other autogenerated files; I bet we either missed that or
>> fat-fingered it for node-support-stamp.

> I see below logic added by commit which seems to help regenerate the
> required files.

Meh ... it's not checking the data files themselves.  Here's
a patch based on the logic for invoking genbki.  Completely
untested, would somebody try it?

			regards, tom lane

  86. Re: automatically generating node support functions

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-08T06:53:25Z

    On Sun, Aug 7, 2022 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila16@gmail.com> writes:
> > On Wed, Aug 3, 2022 at 7:16 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> More likely, we need to add something explicit to Mkvcbuild.pm
> >> for this.  I recall that it has stanzas to deal with updating
> >> other autogenerated files; I bet we either missed that or
> >> fat-fingered it for node-support-stamp.
>
> > I see below logic added by commit which seems to help regenerate the
> > required files.
>
> Meh ... it's not checking the data files themselves.  Here's
> a patch based on the logic for invoking genbki.  Completely
> untested, would somebody try it?
>

I tried it on commit a69959fab2 just before the commit (1349d2790b)
which was causing problems for me. On running "perl mkvcbuild.pl", I
got the below error:
wrong number of input files, expected nodes/nodes.h nodes/primnodes.h
nodes/parsenodes.h nodes/pathnodes.h nodes/plannodes.h
nodes/execnodes.h access/amapi.h access/sdir.h access/tableam.h
access/tsmapi.h commands/event_trigger.h commands/trigger.h
executor/tuptable.h foreign/fdwapi.h nodes/extensible.h
nodes/lockoptions.h nodes/replnodes.h nodes/supportnodes.h
nodes/value.h utils/rel.h

This error seems to be originating from gen_node_support.pl. If I
changed the @node_headers to what it was instead of getting it from
Makefile then the patch works and the build is also successful. See
attached.

-- 
With Regards,
Amit Kapila.

  87. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-08T18:06:00Z

    Amit Kapila <amit.kapila16@gmail.com> writes:
> On Sun, Aug 7, 2022 at 8:19 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Meh ... it's not checking the data files themselves.  Here's
>> a patch based on the logic for invoking genbki.  Completely
>> untested, would somebody try it?

> I tried it on commit a69959fab2 just before the commit (1349d2790b)
> which was causing problems for me. On running "perl mkvcbuild.pl", I
> got the below error:
> wrong number of input files, expected nodes/nodes.h nodes/primnodes.h
> nodes/parsenodes.h nodes/pathnodes.h nodes/plannodes.h
> nodes/execnodes.h access/amapi.h access/sdir.h access/tableam.h
> access/tsmapi.h commands/event_trigger.h commands/trigger.h
> executor/tuptable.h foreign/fdwapi.h nodes/extensible.h
> nodes/lockoptions.h nodes/replnodes.h nodes/supportnodes.h
> nodes/value.h utils/rel.h

Ah.  It'd help if that complaint said what the command input actually
is :-(.  But on looking closer, I missed stripping the empty strings
that "split" will produce at the ends of the array.  I think the
attached will do the trick, and I really do want to get rid of this
copy of the file list if possible.

			regards, tom lane

  88. Re: automatically generating node support functions

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-08-08T18:44:29Z

    I wrote:
> Ah.  It'd help if that complaint said what the command input actually
> is :-(.  But on looking closer, I missed stripping the empty strings
> that "split" will produce at the ends of the array.  I think the
> attached will do the trick, and I really do want to get rid of this
> copy of the file list if possible.

I tried this version on the cfbot, and it seems happy, so pushed.

			regards, tom lane

  89. Re: automatically generating node support functions

    Amit Kapila <amit.kapila16@gmail.com> — 2022-08-09T13:16:34Z

    On Tue, Aug 9, 2022 at 12:14 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> I wrote:
> > Ah.  It'd help if that complaint said what the command input actually
> > is :-(.  But on looking closer, I missed stripping the empty strings
> > that "split" will produce at the ends of the array.  I think the
> > attached will do the trick, and I really do want to get rid of this
> > copy of the file list if possible.
>
> I tried this version on the cfbot, and it seems happy, so pushed.
>

Thank you. I have verified the committed patch and it works.

-- 
With Regards,
Amit Kapila.