Re: automatically generating node support functions
On Tue, Aug 9, 2022 at 12:14 AM Tom Lane 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.
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Amit Kapila writes:
> On Sun, Aug 7, 2022 at 8:19 PM Tom Lane 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
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 86cf1b39d0..b707a09f56 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -174,7 +174,7 @@ push @scalar_types, qw(QualCost);
## check that we have the expected number of files on the command line
-die "wrong number of input files, expected @all_input_files\n"
+die "wrong number of input files, expected:\n@all_input_files\ngot:\n@ARGV\n"
if ($#ARGV != $#all_input_files);
## read input
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index 383b8a7c62..cc82668457 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -797,36 +797,30 @@ EOF
close($chs);
}
- if (IsNewer(
- 'src/backend/nodes/node-support-stamp',
- 'src/backend/nodes/gen_node_support.pl'))
+ my $nmf = Project::read_file('src/backend/nodes/Makefile');
+ $nmf =~ s{\\\r?\n}{}g;
+ $nmf =~ /^node_headers\s*:?=(.*)$/gm
+ || croak "Could not find node_headers in Makefile\n";
+ my @node_headers = split /\s+/, $1;
+ @node_headers = grep { $_ ne '' } @node_headers;
+ my @node_files = map { "src/include/$_" } @node_headers;
+
+ my $need_node_support = 0;
+ foreach my $nodefile (@node_files)
{
- # XXX duplicates node_headers list in src/backend/nodes/Makefile
- my @node_headers = qw(
- 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
- );
-
- my @node_files = map { "src/include/$_" } @node_headers;
+ if (IsNewer('src/backend/nodes/node-support-stamp', $nodefile))
+ {
+ $need_node_support = 1;
+ last;
+ }
+ }
+ $need_node_support = 1
+ if IsNewer(
+ 'src/backend/nodes/node-support-stamp',
+ 'src/backend/nodes/gen_node_support.pl');
+ if ($need_node_support)
+ {
system("perl src/backend/nodes/gen_node_support.pl --outdir src/backend/nodes @node_files");
open(my $f, '>', 'src/backend/nodes/node-support-stamp')
|| confess "Could not touch node-support-stamp";
Re: automatically generating node support functions
On Sun, Aug 7, 2022 at 8:19 PM Tom Lane wrote: > > Amit Kapila writes: > > On Wed, Aug 3, 2022 at 7:16 PM Tom Lane 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. msvc-check-for-obsolete-node-support-2.patch Description: Binary data
Re: automatically generating node support functions
Amit Kapila writes:
> On Wed, Aug 3, 2022 at 7:16 PM Tom Lane 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
diff --git a/src/tools/msvc/Solution.pm b/src/tools/msvc/Solution.pm
index caacb965bb..40c962d43c 100644
--- a/src/tools/msvc/Solution.pm
+++ b/src/tools/msvc/Solution.pm
@@ -800,36 +800,29 @@ EOF
close($chs);
}
- if (IsNewer(
- 'src/backend/nodes/node-support-stamp',
- 'src/backend/nodes/gen_node_support.pl'))
+ my $nmf = Project::read_file('src/backend/nodes/Makefile');
+ $nmf =~ s{\\\r?\n}{}g;
+ $nmf =~ /^node_headers\s*:?=(.*)$/gm
+ || croak "Could not find node_headers in Makefile\n";
+ my @node_headers = split /\s+/, $1;
+ my @node_files = map { "src/include/$_" } @node_headers;
+
+ my $need_node_support = 0;
+ foreach my $nodefile (@node_files)
{
- # XXX duplicates node_headers list in src/backend/nodes/Makefile
- my @node_headers = qw(
- 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
- );
-
- my @node_files = map { "src/include/$_" } @node_headers;
+ if (IsNewer('src/backend/nodes/node-support-stamp', $nodefile))
+ {
+ $need_node_support = 1;
+ last;
+ }
+ }
+ $need_node_support = 1
+ if IsNewer(
+ 'src/backend/nodes/node-support-stamp',
+ 'src/backend/nodes/gen_node_support.pl');
+ if ($need_node_support)
+ {
system("perl src/backend/nodes/gen_node_support.pl --outdir src/backend/nodes @node_files");
open(my $f, '>', 'src/backend/nodes/node-support-stamp')
|| confess "Could not touch node-support-stamp";
Re: automatically generating node support functions
On Wed, Aug 3, 2022 at 7:16 PM Tom Lane wrote: > > Amit Kapila 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.
Re: automatically generating node support functions
Amit Kapila 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
Re: automatically generating node support functions
On Wed, Jul 13, 2022 at 12:34 AM Peter Eisentraut 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.
Re: automatically generating node support functions
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
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 96af17516a..35af4e231f 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -213,15 +213,34 @@ foreach my $infile (@ARGV)
}
$file_content .= $raw_file_content;
- my $lineno = 0;
+ my $lineno = 0;
+ my $prevline = '';
foreach my $line (split /\n/, $file_content)
{
+ # per-physical-line processing
$lineno++;
chomp $line;
$line =~ s/\s*$//;
next if $line eq '';
next if $line =~ /^#(define|ifdef|endif)/;
+ # within a node struct, don't process until we have whole logical line
+ if ($in_struct && $subline > 1)
+ {
+ if ($line =~ m/;$/)
+ {
+# found the end, re-attach any previous line(s)
+$line = $prevline . $line;
+$prevline = '';
+ }
+ else
+ {
+# set it aside for a moment
+$prevline .= $line . ' ';
+next;
+ }
+ }
+
# we are analyzing a struct definition
if ($in_struct)
{
@@ -394,7 +413,7 @@ foreach my $infile (@ARGV)
}
# normal struct field
elsif ($line =~
-/^\s*(.+)\s*\b(\w+)(\[\w+\])?\s*(?:pg_node_attr\(([\w(), ]*)\))?;/
+/^\s*(.+)\s*\b(\w+)(\[[\w\s+]+\])?\s*(?:pg_node_attr\(([\w(), ]*)\))?;/
)
{
if ($is_node_struct)
@@ -441,13 +460,46 @@ foreach my $infile (@ARGV)
$my_field_attrs{$name} = \@attrs;
}
}
- else
+ # function pointer field
+ elsif ($line =~
+/^\s*([\w\s*]+)\s*\(\*(\w+)\)\s*\((.*)\)\s*(?:pg_node_attr\(([\w(), ]*)\))?;/
+ )
{
if ($is_node_struct)
{
- #warn "$infile:$lineno: could not parse \"$line\"\n";
+ my $type = $1;
+ my $name = $2;
+ my $args = $3;
+ my $attrs = $4;
+
+ my @attrs;
+ if ($attrs)
+ {
+ @attrs = split /,\s*/, $attrs;
+ foreach my $attr (@attrs)
+ {
+ if ( $attr !~ /^copy_as\(\w+\)$/
+&& $attr !~ /^read_as\(\w+\)$/
+&& !elem $attr,
+qw(equal_ignore read_write_ignore))
+ {
+die
+ "$infile:$lineno: unrecognized attribute \"$attr\"\n";
+ }
+ }
+ }
+
+ push @my_fields, $name;
+ $my_field_types{$name} = 'function pointer';
+ $my_field_attrs{$name} = \@attrs;
}
}
+ else
+ {
+# We're not too picky about what's outside structs,
+# but we'd better understand everything inside.
+die "$infile:$lineno: could not parse \"$line\"\n";
+ }
}
# not in a struct
else
@@ -709,6 +761,12 @@ _equal${n}(const $n *a, const $n *b)
unless $equal_ignore;
}
}
+ elsif ($t eq 'function pointer')
+ {
+ # we can copy and compare as a scalar
+ print $cff "\tCOPY_SCALAR_FIELD($f);\n"unless $copy_ignore;
+ print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" unless $equal_ignore;
+ }
# node type
elsif ($t =~ /(\w+)\*/ and elem $1, @node_types)
{
@@ -980,6 +1038,12 @@ _read${n}(void)
unless $no_read;
}
}
+ elsif ($t eq 'function pointer')
+ {
+ # We don't print these, and we can't read them either
+ die "cannot read function pointer in struct \"$n\" field \"$f\"\n"
+ unless $no_read;
+ }
# Special treatments of several Path node fields
elsif ($t eq 'RelOptInfo*' && elem 'write_only_relids', @a)
{
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index b0c9c5f2ef..98fe1abaa2 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -303,26 +303,26 @@ typedef struct A_Expr
/*
* A_Const - a literal constant
+ *
+ * Value nodes are inline for performance. You can treat 'val' as a node,
+ * as in IsA(, Integer). 'val' is not valid if isnull is true.
*/
+union ValUnion
+{
+ Node node;
+ Integer ival;
+ Float fval;
+ Boolean boolval;
+ String sval;
+ BitString bsval;
+};
+
typedef struct A_Const
{
pg_node_attr(custom_copy_equal, custom_read_write, no_read)
NodeTag type;
-
- /*
- * Value nodes are inline for performance. You can treat 'val' as a node,
- * as in IsA(, Integer). 'val' is not valid if isnull is true.
- */
- union ValUnion
- {
- Node node;
- Integer ival;
- Float fval;
- Boolean boolval;
- String sval;
- BitString bsval;
- } val;
+ union ValUnion val;
bool isnull; /* SQL NULL constant */
int
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
On 2022-07-11 18:39:44 -0400, Tom Lane wrote: > Andres Freund 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.
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Hi, On 2022-07-11 18:09:15 -0400, Tom Lane wrote: > Andres Freund 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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Hi, On 2022-07-11 16:38:05 -0400, Tom Lane wrote: > Andres Freund 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
Re: automatically generating node support functions
I wrote:
> Andres Freund 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
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index 2c06609726..2c6766f537 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -34,6 +34,20 @@ sub elem
return grep { $_ eq $x } @_;
}
+
+# ARM ABI STABILITY CHECK HERE:
+#
+# In stable branches, set $last_nodetag to the name of the last node type
+# that should receive an auto-generated nodetag number, and $last_nodetag_no
+# to its number. The script will then complain if those values don't match
+# reality, providing a cross-check that we haven't broken ABI by adding or
+# removing nodetags.
+# In HEAD, these variables should be left undef, since we don't promise
+# ABI stability during development.
+
+my $last_nodetag= undef;
+my $last_nodetag_no = undef;
+
# output file names
my @output_files;
@@ -88,6 +102,9 @@ my @custom_copy_equal;
# Similarly for custom read/write implementations.
my @custom_read_write;
+# Track node types with manually assigned NodeTag numbers.
+my %manual_nodetag_number;
+
# EquivalenceClasses are never moved, so just shallow-copy the pointer
push @scalar_types, qw(EquivalenceClass* EquivalenceMember*);
@@ -267,6 +284,10 @@ foreach my $infile (@ARGV)
# does in fact exist.
push @no_read_write, $in_struct;
}
+ elsif ($attr =~ /^nodetag_number\((\d+)\)$/)
+ {
+ $manual_nodetag_number{$in_struct} = $1;
+ }
else
{
die
@@ -472,14 +493,31 @@ open my $nt, '>', 'nodetags.h' . $tmpext or die $!;
printf $nt $header_comment, 'nodetags.h';
-my $i = 1;
+my $tagno= 0;
+my $last_tag = undef;
foreach my $n (@node_types, @extra_tags)
{
next if elem $n, @abstract_types;
- print $nt "\tT_${n} = $i,\n";
- $i++;
+ if (defined $manual_nodetag_number{$n})
+ {
+ # do not change $tagno or $last_tag
+ print $nt "\tT_${n} = $manual_nodetag_number{$n},\n";
+ }
+ else
+ {
+ $tagno++;
+ $last_tag = $n;
+ print $nt "\tT_${n} = $tagno,\n";
+ }
}
+# verify that last auto-assigned nodetag stays stable
+die "ABI stability break: last nodetag is $last_tag not $last_nodetag\n"
+ if (defined $last_nodetag && $last_nodetag ne $last_tag);
+die
+ "ABI stability break: last nodetag number is $tagno not $last_nodetag_no\n"
+ if (defined $last_nodetag_no && $last_nodetag_no ne $tagno);
+
close $nt;
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index adc549002a..e0b336cd28 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -63,6 +63,11 @@ typedef enum NodeTag
*
* - special_read_write: Has special treatment in outNode() and nodeRead().
*
+ * - nodetag_number(VALUE): assign the specified nodetag number instead of
+ * an auto-generated number. Typically this would only be used in stable
+ * branches, to give a newly-added node type a number without breaking ABI
+ * by changing the numbers of existing node types.
+ *
* Node types can be supertypes of other types whether or not they are marked
* abstract: if a node struct appears as the first field of another struct
* type, then it is the supertype of that type. The no_copy, no_equal, and
diff --git a/src/tools/RELEASE_CHANGES b/src/tools/RELEASE_CHANGES
index e8de724fcd..73b02fa2a4 100644
--- a/src/tools/RELEASE_CHANGES
+++ b/src/tools/RELEASE_CHANGES
@@ -107,6 +107,10 @@ Starting a New Development Cycle
placeholder), "git rm" the previous one, and update release.sgml and
filelist.sgml to match.
+* In the newly-made branch, change src/backend/nodes/gen_node_support.pl
+ to enforce ABI stability of the NodeTag list (see "ARM ABI STABILITY
+ CHECK HERE" therein).
+
* Notify the private committers email list, to ensure all committers
are aware of the new branch even if they're not paying close attention
to pgsql-hackers.
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Hi, On 2022-07-11 16:17:28 -0400, Robert Haas wrote: > On Mon, Jul 11, 2022 at 3:54 PM 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, 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
Re: automatically generating node support functions
Hi, On 2022-07-11 15:54:22 -0400, Tom Lane wrote: > Robert Haas writes: > > On Mon, Jul 11, 2022 at 1:57 PM 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. > > > 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
Re: automatically generating node support functions
On Mon, Jul 11, 2022 at 3:54 PM 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, 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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Robert Haas writes: > On Mon, Jul 11, 2022 at 1:57 PM 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. > 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
On Mon, Jul 11, 2022 at 1:57 PM 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. 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
Re: automatically generating node support functions
I wrote: >> Andres Freund 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
Re: automatically generating node support functions
Hi, On 2022-07-11 12:07:09 -0400, Tom Lane wrote: > I wrote: > > Peter Eisentraut 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
Re: automatically generating node support functions
I wrote: > Peter Eisentraut 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
Re: automatically generating node support functions
Peter Eisentraut writes: > On 11.07.22 01:09, Tom Lane wrote: >> Andres Freund 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
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
On 11.07.22 01:09, Tom Lane wrote: Andres Freund 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"
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
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
diff --git a/src/backend/nodes/README b/src/backend/nodes/README
index b3dc9afaf7..d8ae35ce58 100644
--- a/src/backend/nodes/README
+++ b/src/backend/nodes/README
@@ -6,10 +6,30 @@ Node Structures
Introduction
+Postgres uses "node" types to organize parse trees, plan trees, and
+executor state trees. All objects that can appear in such trees must
+be declared as node types. In addition, a few object types that aren't
+part of parse/plan/execute node trees receive NodeTags anyway for
+identification purposes, usually because they are involved in APIs
+where we want to pass multiple object types through the same pointer.
+
The node structures are plain old C structures with the first field
being of type NodeTag. "Inheritance" is achieved by convention:
the first field can alternatively be of another node type.
+Node types typically have support for being copied by copyObject(),
+compared by equal(), serialized by outNode(), and deserialized by
+nodeRead(). For some classes of Nodes, not all of these support
+functions are required; for example, executor state nodes don't
+presently need any of them. So far as the system is concerned,
+output and read functions are only needed for node types that can
+appear in parse trees stored in the catalogs. However, we provide
+output functions for many other node types as well, because they
+are very handy for debugging.
+
+Relevant Files
+--
+
Utility functions for manipulating node structures reside in this
directory. Some support functions are automatically generated by the
gen_node_support.pl script, other functions are maintained manually.
@@ -40,7 +60,7 @@ FILES IN THIS DIRECTORY (src/backend/nodes/)
FILES IN src/include/nodes/
-Node definitions:
+Node definitions primarily appear in:
nodes.h - define node tags (NodeTag) (*)
primnodes.h - primitive nodes
parsenodes.h - parse tree nodes
diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index dca5819f95..6816c36e2b 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -124,19 +124,31 @@ foreach my $infile (@ARGV)
my $supertype_field;
my $node_attrs = '';
+ my $node_attrs_lineno;
my @my_fields;
my %my_field_types;
my %my_field_attrs;
open my $ifh, '<', $infile or die "could not open \"$infile\": $!";
- my $file_content = do { local $/; <$ifh> };
+ my $raw_file_content = do { local $/; <$ifh> };
- # strip C comments
- $file_content =~ s{/\*.*?\*/}{}gs;
+ # strip C comments, preserving newlines so we can count lines correctly
+ my $file_content = '';
+ while ($raw_file_content =~ m{^(.*?)(/\*.*?\*/)(.*)$}s)
+ {
+ $file_content .= $1;
+ my $comment = $2;
+ $raw_file_content = $3;
+ $comment =~ tr/\n//cd;
+ $file_content .= $comment;
+ }
+ $file_content .= $raw_file_content;
+ my $lineno = 0;
foreach my $line (split /\n/, $file_content)
{
+ $lineno++;
chomp $line;
$line =~ s/\s*$//;
next if $line eq '';
@@ -153,13 +165,14 @@ foreach my $infile (@ARGV)
$is_node_struct = 0;
$supertype = undef;
next if $line eq '{';
-die "$infile:$.: expected opening brace\n";
+die "$infile:$lineno: expected opening brace\n";
}
# second line could be node attributes
elsif ($subline == 2
&& $line =~ /^\s*pg_node_attr\(([\w(), ]*)\)$/)
{
-$node_attrs = $1;
+$node_attrs= $1;
+$node_attrs_lineno = $lineno;
# hack: don't count the line
$subline--;
next;
@@ -236,7 +249,7 @@ foreach my $infile (@ARGV)
else
{
die
- "$infile:$.: unrecognized attribute
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Alvaro Herrera 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
Re: automatically generating node support functions
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 HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/ >From 24185e0421cc1e22f9a78f56d03e4585a142e78e Mon Sep 17 00:00:00 2001 From: Alvaro Herrera Date: Fri, 8 Jul 2022 15:34:47 +0200 Subject: [PATCH] Forgot to add copy support in f10a025cfe97 --- src/backend/nodes/copyfuncs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 2c834e4d0d..b8a5715981 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -5978,11 +5978,12 @@ copyObjectImpl(const void *from) break; /* - * Lists of integers and OIDs don't need to be deep-copied, so we - * perform a shallow copy via list_copy() + * Lists of integers, OIDs and XIDs don't need to be deep-copied, + * so we perform a shallow copy via list_copy() */ case T_IntList: case T_OidList: + case T_XidList: retval = list_copy(from); break; -- 2.30.2
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Peter Eisentraut 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
From c82ee081a7a8cdc77b44f325d1df695b55a60b06 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 6 Jul 2022 12:13:32 +0200
Subject: [PATCH v7] Automatically generate node support functions
Add 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.
For each of the four node support files, it creates two include files,
e.g., copyfuncs.funcs.c and copyfuncs.switch.c, to include in the main
file. All the scaffolding of the main file stays in place.
TODO: 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. All the
code comments that are worth
Re: automatically generating node support functions
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.)
Re: automatically generating node support functions
The new patch addresses almost all of these issues.
> Also, I share David's upthread allergy to the option names
> "path_hackN" and to documenting those only inside the conversion
> script.
I have given these real names now and documented them with the other
attributes.
> BTW, I think this: "Unknown attributes are ignored" is a seriously
> bad idea; it will allow typos to escape detection.
fixed
(I have also changed the inside of pg_node_attr to be comma-separated,
rather than space-separated. This matches better how attribute-type
things look in C.)
I think we ought to put it into the *nodes.h headers as much as
possible, perhaps like this:
typedef struct A_Const pg_node_attr(custom_copy)
{ ...
done
So I propose that we handle these things via struct-level pg_node_attr
markers, rather than node-type lists embedded in the script:
abstract_types
no_copy
no_read_write
no_read
custom_copy
custom_readwrite
done (no_copy is actually no_copy_equal, hence renamed)
The hacks for scalar-copying EquivalenceClass*, EquivalenceMember*,
struct CustomPathMethods*, and CustomScan.methods should be replaced
with "pg_node_attr(copy_as_scalar)" labels on affected fields.
Hmm, at least for Equivalence..., this is repeated a bunch of times for
each field. I don't know if this is really a property of the type or
something you can choose for each field? [not changed in v7 patch]
I wonder whether this:
# We do not support copying Path trees, mainly
# because the circular linkages between RelOptInfo
# and Path nodes can't be handled easily in a
# simple depth-first traversal.
couldn't be done better by inventing an inheritable no_copy attr
to attach to the Path supertype. Or maybe it'd be okay to just
automatically inherit the no_xxx properties from the supertype?
This is an existing comment in copyfuncs.c. I haven't looked into it
any further.
I don't terribly like the ad-hoc mechanism for not comparing
CoercionForm fields. OTOH, I am not sure whether replacing it
with per-field equal_ignore attrs would be better; there's at least
an argument that that invites bugs of omission. But implementing
this with an uncommented test deep inside a script that most hackers
should not need to read is not good. On the whole I'd lean towards
the equal_ignore route.
The definition of CoercionForm in primnodes.h says that the comparison
behavior is a property of the type, so it needs to be handled somewhere
centrally, not on each field. [not changed in v7 patch]
I'm confused by the "various field types to ignore" at the end
of the outfuncs/readfuncs code. Do we really ignore those now?
How could that be safe? If it is safe, wouldn't it be better
to handle that with per-field pg_node_attrs? Silently doing
what might be the wrong thing doesn't seem good.
I have replaced these with explicit ignore markings in pathnodes.h
(PlannerGlobal, PlannerInfo, RelOptInfo). (This could then use a bit
more rearranging some of the per-field comments.)
* copyfuncs.switch.c and equalfuncs.switch.c are missing trailing
newlines.
fixed
* pgindent is not very happy with a lot of your comments in *nodes.h.
fixed
* I think we should add explicit dependencies in backend/nodes/Makefile,
along the lines of
copyfuncs.o: copyfuncs.c copyfuncs.funcs.c copyfuncs.switch.c
Otherwise the whole thing is a big gotcha for anyone not using
--enable-depend.
fixed -- I think, could use more testingFrom c82ee081a7a8cdc77b44f325d1df695b55a60b06 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 6 Jul 2022 12:13:32 +0200
Subject: [PATCH v7] Automatically generate node support functions
Add 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.
For each of the four node support files, it creates two include files,
e.g., copyfuncs.funcs.c and copyfuncs.switch.c, to include in the main
file. All the scaffolding of the main file stays in place.
TODO: 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. All the
code comments that are worth keeping from those sections have already
been moved to the header files where the structs are defined.
I have tried to mostly make the coverage of the output match what is
currently there. For example, one could now do out/read coverage of
utility statement nodes, 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
Re: automatically generating node support functions
... 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
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
On 03.07.22 21:14, Tom Lane wrote: Peter Eisentraut 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 pointFrom 0ad86183662b6637c9ec9a29374fecb4d11202e2 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 4 Jul 2022 14:17:02 +0200 Subject: [PATCH v6] Automatically generate node support functions Add 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. For each of the four node support files, it creates two include files, e.g., copyfuncs.funcs.c and copyfuncs.switch.c, to include in the main file. All the scaffolding of the main file stays in place. TODO: 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. All the code comments that are worth keeping from those sections have already been moved to the header files where the structs are defined. I have tried to mostly make the coverage of the output match what is currently there. For example, one could now do out/read coverage of utility statement nodes, 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. Discussion: https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com --- src/backend/Makefile | 10 +- src/backend/nodes/.gitignore | 4 + src/backend/nodes/Makefile| 54 ++ src/backend/nodes/copyfuncs.c | 19 +- src/backend/nodes/equalfuncs.c| 22 +- src/backend/nodes/gen_node_support.pl | 729 ++ src/backend/nodes/outfuncs.c | 34 +- src/backend/nodes/readfuncs.c | 23 +- src/include/Makefile | 1 + src/include/nodes/.gitignore | 2 + src/include/nodes/nodes.h | 27 + src/include/nodes/parsenodes.h| 4 +- src/include/nodes/pathnodes.h | 175 --- src/include/nodes/plannodes.h | 90 ++-- src/include/nodes/primnodes.h | 34 +- src/include/utils/rel.h | 6 +- src/tools/msvc/Solution.pm| 46 ++ 17 files changed, 1124 insertions(+), 156 deletions(-) create mode 100644 src/backend/nodes/.gitignore create mode 100644 src/backend/nodes/gen_node_support.pl create mode 100644 src/include/nodes/.gitignore diff --git a/src/backend/Makefile b/src/backend/Makefile index 4a02006788..953c80db5a 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw submake-catalog-headers: $(MAKE) -C catalog distprep generated-header-symlinks +# run this unconditionally to avoid needing to know its dependencies here: +submake-nodes-headers: + $(MAKE) -C nodes distprep generated-header-symlinks + # run this unconditionally to avoid needing to know its dependencies here: submake-utils-headers: $(MAKE) -C utils distprep generated-header-symlinks -.PHONY: submake-catalog-headers submake-utils-headers +.PHONY: submake-catalog-headers submake-nodes-headers submake-utils-headers # Make symlinks for these headers in the include directory. That way # we can cut down on the -I options. Also, a symlink is automatically @@ -162,7 +166,7 @@ submake-utils-headers: .PHONY: generated-headers -generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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.From 5eea69417e524779e2e3cc5164966646cb2c2c0e Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 23 May 2022 07:40:12 +0200 Subject: [PATCH] Reformat node comments --- src/include/nodes/parsenodes.h | 3 +- src/include/nodes/pathnodes.h | 686 ++--- src/include/nodes/plannodes.h | 423 ++-- src/include/nodes/primnodes.h | 166 +--- 4 files changed, 899 insertions(+), 379 deletions(-) diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 73f635b455..f93d866548 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -123,7 +123,8 @@ typedef struct Query QuerySource querySource;/* where did I come from? */ - uint64 queryId;/* query identifier (can be set by plugins) */ + /* query identifier (can be set by plugins) */ + uint64 queryId; boolcanSetTag; /* do I set the command result tag? */ diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index a6e5db4eec..b88cfb8dc0 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -226,8 +226,8 @@ struct PlannerInfo * even when using the hash table for lookups; this simplifies life for * GEQO. */ - List *join_rel_list; /* list of join-relation RelOptInfos */ - struct HTAB *join_rel_hash; /* optional hashtable for join relations */ + List *join_rel_list; + struct HTAB *join_rel_hash; /* * When doing a dynamic-programming-style join search, join_rel_level[k] @@ -329,11 +329,16 @@ struct PlannerInfo */ List *update_colnos; - /* Fields filled during create_plan() for use in setrefs.c */ - AttrNumber *grouping_map; /* for GroupingFunc fixup */ - List *minmax_aggs;/* List of MinMaxAggInfos */ + /* +* Fields filled during create_plan() for use in setrefs.c +*/ + /* for GroupingFunc fixup */ + AttrNumber *grouping_map; + /* List of MinMaxAggInfos */ + List *minmax_aggs; - MemoryContext planner_cxt; /* context holding PlannerInfo */ + /* context holding PlannerInfo */ + MemoryContext planner_cxt; Cardinality total_table_pages; /* # of pages in all non-dummy tables of * query */ @@ -369,9 +374,12 @@ struct PlannerInfo Relids curOuterRels; /* outer rels above current node */ List *curOuterParams; /* not-yet-assigned NestLoopParams */ - /* These fields are workspace for setrefs.c */ - bool *isAltSubplan; /* array corresponding to glob->subplans */ - bool *isUsedSubplan; /* array corresponding to glob->subplans */ + /* +* These fields are workspace for setrefs.c. Each is an array +* corresponding to glob->subplans. +*/ + bool *isAltSubplan; + bool *isUsedSubplan; /* optional private data for join_search_hook, e.g., GEQO */ void *join_search_private; @@ -678,21 +686,37 @@ typedef struct RelOptInfo RelOptKind reloptkind; - /* all relations included in this RelOptInfo */ - Relids relids; /* set of base relids (rangetable indexes) */ + /* +* all relations included in this RelOptInfo; set of base relids +* (rangetable indexes) +*/ + Relids relids; - /* size estimates generated by planner */ - Cardinality rows; /* estimated number of result tuples */ + /* +* size estimates generated by planner +*/ + /* estimated number of result tuples */ + Cardinality rows; - /* per-relation planner control flags */ - boolconsider_startup; /* keep cheap-startup-cost paths? */ - boolconsider_param_startup; /* ditto, for parameterized paths? */
Re: automatically generating node support functions
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)
Re: automatically generating node support functions
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?
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
Alvaro Herrera 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
Re: automatically generating node support functions
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 HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
"El miedo atento y previsor es la madre de la seguridad" (E. Burke)
>From 56cf5918f1a2abcb285ad2d4d880779eaed388d0 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 19 Apr 2022 13:36:21 +0200
Subject: [PATCH 1/2] Complete JsonTableColumnType typedef
---
src/include/nodes/parsenodes.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index da02658c81..b1f81feb46 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -1610,7 +1610,7 @@ typedef enum JsonQuotes
* JsonTableColumnType -
* enumeration of JSON_TABLE column types
*/
-typedef enum
+typedef enum JsonTableColumnType
{
JTC_FOR_ORDINALITY,
JTC_REGULAR,
--
2.30.2
>From 0cd2f44259c4778eb19568d9c2f3a81965642303 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Tue, 19 Apr 2022 12:03:19 +0200
Subject: [PATCH 2/2] Automatically generate node support functions
Add 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.
For each of the four node support files, it creates two include files,
e.g., copyfuncs.funcs.c and copyfuncs.switch.c, to include in the main
file. All the scaffolding of the main file stays in place.
TODO: 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. All the
code comments that are worth keeping from those sections have already
been moved to the header files where the structs are defined.
I have tried to mostly make the coverage of the output match what is
currently there. For example, one could now do out/read coverage of
utility statement nodes, 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.
Discussion: https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com
---
src/backend/Makefile | 8 +-
src/backend/nodes/.gitignore | 4 +
src/backend/nodes/Makefile| 46 ++
src/backend/nodes/copyfuncs.c | 19 +-
src/backend/nodes/equalfuncs.c| 22 +-
src/backend/nodes/gen_node_support.pl | 729 ++
src/backend/nodes/outfuncs.c | 34 +-
src/backend/nodes/readfuncs.c | 23 +-
src/include/nodes/.gitignore | 2 +
src/include/nodes/nodes.h | 28 +
src/include/nodes/parsenodes.h| 3 +-
src/include/nodes/pathnodes.h | 170 +++---
src/include/nodes/plannodes.h | 90 ++--
src/include/nodes/primnodes.h | 33 +-
src/include/utils/rel.h | 6 +-
src/tools/msvc/Solution.pm| 46 ++
16 files changed, 1114 insertions(+), 149 deletions(-)
create mode 100644 src/backend/nodes/.gitignore
create mode 100644 src/backend/nodes/gen_node_support.pl
create mode 100644 src/include/nodes/.gitignore
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 4a02006788..821bef2694 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw
submake-catalog-headers:
$(MAKE) -C catalog distprep generated-header-symlinks
+# run this unconditionally to avoid needing to know its dependencies here:
+submake-nodes-headers:
+ $(MAKE) -C nodes distprep generated-header-symlinks
+
# run this unconditionally to avoid needing to know its dependencies here:
submake-utils-headers:
$(MAKE) -C utils distprep generated-header-symlinks
-.PHONY: submake-catalog-headers submake-utils-headers
+.PHONY: submake-catalog-headers submake-nodes-headers submake-utils-headers
# Make symlinks for these headers in the include directory. That way
# we can cut down on the -I options. Also, a symlink is automatically
@@ -162,7 +166,7 @@ submake-utils-headers:
.PHONY: generated-headers
-generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h
Re: automatically generating node support functions
On 25.03.22 14:32, Tom Lane wrote: Peter Eisentraut 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.
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
On Fri, 18 Feb 2022 at 19:52, Peter Eisentraut 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
Re: automatically generating node support functions
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? rightFrom cdd9f2a6738b8c85cec8ba6169f281ebdc4d2e4d Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Fri, 18 Feb 2022 07:39:42 +0100 Subject: [PATCH v5] Automatically generate node support functions Add 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. For each of the four node support files, it creates two include files, e.g., copyfuncs.funcs.c and copyfuncs.switch.c, to include in the main file. All the scaffolding of the main file stays in place. TODO: 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. All the code comments that are worth keeping from those sections have already been moved to the header files where the structs are defined. I have tried to mostly make the coverage of the output match what is currently there. For example, one could now do out/read coverage of utility statement nodes, 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. Discussion: https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com --- src/backend/Makefile | 8 +- src/backend/nodes/.gitignore | 4 + src/backend/nodes/Makefile| 46 ++ src/backend/nodes/copyfuncs.c | 19 +- src/backend/nodes/equalfuncs.c| 22 +- src/backend/nodes/gen_node_support.pl | 729 ++ src/backend/nodes/outfuncs.c | 34 +- src/backend/nodes/readfuncs.c | 23 +- src/include/nodes/.gitignore | 2 + src/include/nodes/nodes.h | 27 + src/include/nodes/parsenodes.h| 3 +- src/include/nodes/pathnodes.h | 170 +++--- src/include/nodes/plannodes.h | 90 ++-- src/include/nodes/primnodes.h | 33 +- src/include/utils/rel.h | 6 +- src/tools/msvc/Solution.pm| 46 ++ 16 files changed, 1113 insertions(+), 149 deletions(-) create mode 100644 src/backend/nodes/.gitignore create mode 100644 src/backend/nodes/gen_node_support.pl create mode 100644 src/include/nodes/.gitignore diff --git a/src/backend/Makefile b/src/backend/Makefile index 4a02006788..821bef2694 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw submake-catalog-headers: $(MAKE) -C catalog distprep generated-header-symlinks +# run this unconditionally to avoid needing to know its dependencies here: +submake-nodes-headers: + $(MAKE) -C nodes distprep generated-header-symlinks + # run this unconditionally to avoid
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
Hi, On 2022-02-14 18:32:21 -0500, Tom Lane wrote: > Andres Freund 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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
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
From d99c818447ee4a68b762fb8a8c645ee6ef051ff9 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 24 Jan 2022 16:13:21 +0100
Subject: [PATCH v4] Automatically generate node support functions
Add 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.
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.
TODO: 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 now do out/read coverage of
utility statement nodes, 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.
Discussion:
https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com
---
src/backend/Makefile | 8 +-
src/backend/nodes/.gitignore | 3 +
src/backend/nodes/Makefile| 46 ++
src/backend/nodes/copyfuncs.c | 19 +-
src/backend/nodes/equalfuncs.c| 22 +-
src/backend/nodes/gen_node_support.pl | 660 ++
src/backend/nodes/outfuncs.c | 30 +-
src/backend/nodes/readfuncs.c | 23 +-
src/include/nodes/.gitignore | 2 +
src/include/nodes/nodes.h | 8 +
src/include/nodes/parsenodes.h| 2 +-
src/include/nodes/pathnodes.h | 136 +++---
src/include/nodes/plannodes.h | 90 ++--
src/include/nodes/primnodes.h | 20 +-
src/include/pg_config_manual.h| 4 +-
src/include/utils/rel.h | 6 +-
src/tools/msvc/Solution.pm| 46 ++
17 files changed, 977 insertions(+), 148 deletions(-)
create mode 100644 src/backend/nodes/.gitignore
create mode 100644 src/backend/nodes/gen_node_support.pl
create mode 100644 src/include/nodes/.gitignore
diff --git a/src/backend/Makefile b/src/backend/Makefile
index add9560be4..f9ce7cefcc 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h:
storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw
submake-catalog-headers:
$(MAKE) -C catalog distprep generated-header-symlinks
+# run this unconditionally to avoid needing to know its dependencies here:
+submake-nodes-headers:
+ $(MAKE) -C nodes distprep generated-header-symlinks
+
# run this unconditionally to avoid needing to know its dependencies here:
submake-utils-headers:
$(MAKE) -C utils distprep generated-header-symlinks
-.PHONY: submake-catalog-headers submake-utils-headers
+.PHONY:
Re: automatically generating node support functions
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.deFrom e2c08d8b793200a07b8fe5ae85dd23f401ddcef1 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 29 Dec 2021 12:00:41 +0100
Subject: [PATCH v3] Automatically generate node support functions
Add 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.
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.
TODO: 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 now do out/read coverage of
utility statement nodes, 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.
Discussion:
https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com
---
src/backend/Makefile | 8 +-
src/backend/nodes/.gitignore | 3 +
src/backend/nodes/Makefile| 46 ++
src/backend/nodes/copyfuncs.c | 19 +-
src/backend/nodes/equalfuncs.c| 22 +-
src/backend/nodes/gen_node_support.pl | 660 ++
src/backend/nodes/outfuncs.c | 30 +-
src/backend/nodes/readfuncs.c | 23 +-
src/include/nodes/.gitignore | 2 +
src/include/nodes/nodes.h | 8 +
src/include/nodes/parsenodes.h| 2 +-
src/include/nodes/pathnodes.h | 134 +++---
src/include/nodes/plannodes.h | 90 ++--
src/include/nodes/primnodes.h | 20 +-
src/include/pg_config_manual.h| 4 +-
src/include/utils/rel.h | 6 +-
src/tools/msvc/Solution.pm| 46 ++
17 files changed, 976 insertions(+), 147 deletions(-)
create mode 100644 src/backend/nodes/.gitignore
create mode 100644 src/backend/nodes/gen_node_support.pl
create mode 100644 src/include/nodes/.gitignore
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 0da848b1fd..a33db1ae01 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h:
storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw
submake-catalog-headers:
$(MAKE) -C catalog distprep generated-header-symlinks
+# run this unconditionally to avoid needing to know its dependencies here:
+submake-nodes-headers:
+ $(MAKE) -C nodes distprep generated-header-symlinks
+
# run this unconditionally to avoid needing to know its dependencies here:
submake-utils-headers:
$(MAKE) -C utils distprep generated-header-symlinks
-.PHONY: submake-catalog-headers submake-utils-headers
+.PHONY: submake-catalog-headers submake-nodes-headers submake-utils-headers
# Make symlinks for these
Re: automatically generating node support functions
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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
> > 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.
Re: automatically generating node support functions
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. From 86783a117c1542e292d9f12052e33d75e6107d92 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 11 Oct 2021 10:55:21 +0200 Subject: [PATCH v2] Automatically generate node support functions Add 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. 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. TODO: 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 now do out/read coverage of utility statement nodes, 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. Discussion: https://www.postgresql.org/message-id/flat/c1097590-a6a4-486a-64b1-e1f9cc0533ce%40enterprisedb.com --- src/backend/Makefile| 8 +- src/backend/nodes/.gitignore| 3 + src/backend/nodes/Makefile | 46 ++ src/backend/nodes/copyfuncs.c | 15 + src/backend/nodes/equalfuncs.c | 21 +- src/backend/nodes/gen_node_stuff.pl | 642 src/backend/nodes/outfuncs.c| 23 + src/backend/nodes/readfuncs.c | 19 +- src/include/nodes/.gitignore| 2 + src/include/nodes/nodes.h | 8 + src/include/nodes/parsenodes.h | 2 +- src/include/nodes/pathnodes.h | 132 +++--- src/include/nodes/plannodes.h | 90 ++-- src/include/nodes/primnodes.h | 20 +- src/include/pg_config_manual.h | 4 +- src/include/utils/rel.h | 6 +- src/tools/msvc/Solution.pm | 46 ++ 17 files changed, 954 insertions(+), 133 deletions(-) create mode 100644 src/backend/nodes/.gitignore create mode 100644 src/backend/nodes/gen_node_stuff.pl create mode 100644 src/include/nodes/.gitignore diff --git a/src/backend/Makefile b/src/backend/Makefile index 0da848b1fd..a33db1ae01 100644 --- a/src/backend/Makefile +++ b/src/backend/Makefile @@ -143,11 +143,15 @@ storage/lmgr/lwlocknames.h: storage/lmgr/generate-lwlocknames.pl storage/lmgr/lw submake-catalog-headers: $(MAKE) -C catalog distprep generated-header-symlinks +# run this unconditionally to avoid needing to know its dependencies here: +submake-nodes-headers: + $(MAKE) -C nodes distprep generated-header-symlinks + # run this unconditionally to avoid needing to know its dependencies here: submake-utils-headers: $(MAKE) -C utils distprep generated-header-symlinks -.PHONY: submake-catalog-headers submake-utils-headers +.PHONY: submake-catalog-headers submake-nodes-headers submake-utils-headers # Make symlinks for these headers in the include directory. That way # we can cut down on the -I options. Also, a symlink is automatically @@ -162,7 +166,7 @@ submake-utils-headers: .PHONY: generated-headers -generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-utils-headers +generated-headers: $(top_builddir)/src/include/parser/gram.h $(top_builddir)/src/include/storage/lwlocknames.h submake-catalog-headers submake-nodes-headers submake-utils-headers $(top_builddir)/src/include/parser/gram.h: parser/gram.h prereqdir=`cd '$(dir $<)'
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
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
Re: automatically generating node support functions
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.
From 4a9cc84d667f64bdcdffb1df8c89c4e73b02c1fb Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 17 Aug 2021 15:51:45 +0200
Subject: [PATCH 1/5] Remove T_Expr
This is an abstract node that shouldn't have a node tag defined.
---
src/include/nodes/nodes.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 6a4d82f0a8..5ac1996738 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -153,7 +153,6 @@ typedef enum NodeTag
T_Alias,
T_RangeVar,
T_TableFunc,
- T_Expr,
T_Var,
T_Const,
T_Param,
--
2.32.0
From a6f0e9031071ce695153fea600b5cb07a6507e11 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 17 Aug 2021 15:51:45 +0200
Subject: [PATCH 2/5] Add COPY_ARRAY_FIELD and COMPARE_ARRAY_FIELD
These handle node fields that are inline arrays (as opposed to
dynamically allocated arrays handled by COPY_POINTER_FIELD and
COMPARE_POINTER_FIELD). These cases were hand-coded until now.
---
src/backend/nodes/copyfuncs.c | 11 +++
src/backend/nodes/equalfuncs.c | 7 +++
2 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 38251c2b8e..05c8ca080c 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -53,6 +53,10 @@
#define COPY_STRING_FIELD(fldname) \
(newnode->fldname = from->fldname ? pstrdup(from->fldname) : (char *)
NULL)
+/* Copy a field that is an inline array */
+#define COPY_ARRAY_FIELD(fldname) \
+ memcpy(newnode->fldname, from->fldname, sizeof(newnode->fldname))
+
/* Copy a field that is a pointer to a simple palloc'd object of size sz */
#define COPY_POINTER_FIELD(fldname, sz) \
do { \
@@ -4931,10 +4935,9 @@ _copyForeignKeyCacheInfo(const ForeignKeyCacheInfo *from)
COPY_SCALAR_FIELD(conrelid);
COPY_SCALAR_FIELD(confrelid);
COPY_SCALAR_FIELD(nkeys);
- /* COPY_SCALAR_FIELD might work for these, but let's not assume that */
- memcpy(newnode->conkey, from->conkey, sizeof(newnode->conkey));
- memcpy(newnode->confkey, from->confkey, sizeof(newnode->confkey));
- memcpy(newnode->conpfeqop, from->conpfeqop, sizeof(newnode->conpfeqop));
+ COPY_ARRAY_FIELD(conkey);
+ COPY_ARRAY_FIELD(confkey);
+ COPY_ARRAY_FIELD(conpfeqop);
return newnode;
}
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 8a1762000c..6f656fab97 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -74,6 +74,13 @@
#define equalstr(a, b) \
(((a) != NULL && (b) != NULL) ? (strcmp(a, b) == 0) : (a) == (b))
+/* Compare a field that is an inline array */
+#define COMPARE_ARRAY_FIELD(fldname) \
+ do { \
+ if (memcmp(a->fldname, b->fldname, sizeof(a->fldname)) != 0) \
+ return false; \
+ } while (0)
+
/* Compare a field that is a pointer to a simple palloc'd object of size sz */
#define COMPARE_POINTER_FIELD(fldname, sz) \
do { \
--
2.32.0
From 7d1d7d6697d03b0a4ad3baaf37f315252f2b70c4 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Tue, 17 Aug 2021 15:51:45 +0200
Subject: [PATCH 3/5] Add WRITE_INDEX_ARRAY
We have a few WRITE_{name of type}_ARRAY macros, but the one case
using the Index type was hand-coded. Wrap it into a macro as well.
This also changes the behavior slightly: Before, the field name was
skipped if the length was zero. Now it prints the field name even in
that case. This is more consistent with how other array fields are
handled.
---
src/backend/nodes/outfuncs.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index
Re: automatically generating node support functions
Peter Eisentraut 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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
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
Re: automatically generating node support functions
Andres Freund 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
Re: automatically generating node support functions
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
Re: automatically generating node support functions
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.
Re: automatically generating node support functions
On Tue, 8 Jun 2021 at 08:28, 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. 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.fk2ndqtld3onf...@alap3.anarazel.de [2] https://www.postgresql.org/message-id/20190920051857.2fhnvhvx4qddd...@alap3.anarazel.de
automatically generating node support functions
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.
From c782871d6cc59e2fed232c78c307d63e72cbb3d5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 7 Jun 2021 15:45:14 +0200
Subject: [PATCH v1 01/10] Rename NodeTag of ExprState
Rename from tag to type, for consistency with all other node structs.
---
src/backend/executor/execExpr.c | 4 ++--
src/include/nodes/execnodes.h | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 8c9f8a6aeb..c6ba11d035 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -363,7 +363,7 @@ ExecBuildProjectionInfo(List *targetList,
projInfo->pi_exprContext = econtext;
/* We embed ExprState into ProjectionInfo instead of doing extra palloc
*/
- projInfo->pi_state.tag = T_ExprState;
+ projInfo->pi_state.type = T_ExprState;
state = >pi_state;
state->expr = (Expr *) targetList;
state->parent = parent;
@@ -531,7 +531,7 @@ ExecBuildUpdateProjection(List *targetList,
projInfo->pi_exprContext = econtext;
/* We embed ExprState into ProjectionInfo instead of doing extra palloc
*/
- projInfo->pi_state.tag = T_ExprState;
+ projInfo->pi_state.type = T_ExprState;
state = >pi_state;
if (evalTargetList)
state->expr = (Expr *) targetList;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 7795a69490..8fa9c8aff6 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -60,7 +60,7 @@ typedef Datum (*ExprStateEvalFunc) (struct ExprState
*expression,
typedef struct ExprState
{
- NodeTag tag;
+ NodeTag type;
uint8 flags; /* bitmask of EEO_FLAG_* bits,
see above */
--
2.31.1
From 7746ddd4f2a9534322b2b9226007638d3142c0c7 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 7 Jun 2021 15:47:56 +0200
Subject: [PATCH v1 02/10] Rename argument of _outValue()
Rename from value to node, for consistency with similar functions.
---
src/backend/nodes/outfuncs.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 04696f613c..b54c57d09f 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -3402,12 +3402,12 @@ _outAExpr(StringInfo str, const A_Expr *node)
}
static void
-_outValue(StringInfo str, const Value *value)
+_outValue(StringInfo str, const Value *node)
{
- switch (value->type)
+ switch (node->type)
{
case T_Integer:
- appendStringInfo(str, "%d", value->val.ival);
+ appendStringInfo(str, "%d", node->val.ival);
break;
case T_Float:
@@ -3415,7 +3415,7 @@ _outValue(StringInfo str, const Value *value)
* We assume the value is a valid numeric literal and
so does not
* need quoting.
*/
- appendStringInfoString(str, value->val.str);
+ appendStringInfoString(str, node->val.str);
break;
case T_String:
@@ -3424,20 +3424,20 @@ _outValue(StringInfo str, const Value
