From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: David Rowley <dgrowleyml@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: automatically generating node support functions
Date: Mon, 11 Jul 2022 17:18:57 -0400
Message-ID: <1249010.1657574337@sss.pgh.pa.us> (raw)
In-Reply-To: <1231810.1657569840@sss.pgh.pa.us>
References: <b9acf63c-6693-d87c-f010-ca8ad502a39f@enterp! risedb.com>
<3302658.1657288366@sss.pgh.pa.us>
<611d2e! 93-6330-47fa-0970-72316aed50f2@enterprisedb.com>
<3488143.1657310625@sss.pgh.pa.us>
<11148777-fbe2-5188-195d-48f5083573c5@enterprisedb.com>
<20220710214622.haiektrjzisob6rl@awork3.anarazel.de>
<815831.1657494597@sss.pgh.pa.us>
<45f508cb-e30b-c252-e081-4548deafa34c@enterprisedb.com>
<1116889.1657553859@sss.pgh.pa.us>
<1201553.1657562258@sss.pgh.pa.us>
<20220711182915.jd2ibajdcbncqdqd@awork3.anarazel.de>
<1231810.1657569840@sss.pgh.pa.us>
I wrote:
> Andres Freund <andres@anarazel.de> writes:
>> Additionally, I think we've had to add tags to the enum in minor releases
>> before and I'm afraid this now would end up looking even more awkward?
> Peter and I already had a discussion about that upthread --- we figured
> that if there's a way to manually assign a nodetag's number, you could use
> that option when you have to add a tag in a stable branch. We didn't
> actually build out that idea, but I can go do that, if we can solve the
> more fundamental problem of keeping the autogenerated numbers stable.
> One issue with that idea, of course, is that you have to remember to do
> it like that when back-patching a node addition. Ideally there'd be
> something that'd carp if the last autogenerated tag moves in a stable
> branch, but I'm not very sure where to put that.
One way to do it is to provide logic in gen_node_support.pl to check
that, and activate that logic only in back branches. If we make that
part of the branch-making procedure, we'd not forget to do it.
Proposed patch attached.
regards, tom lane
Attachments:
[text/x-diff] add-ABI-stability-provisions-for-nodetag-list.patch (3.8K, ../1249010.1657574337@sss.pgh.pa.us/2-add-ABI-stability-provisions-for-nodetag-list.patch)
download | inline diff:diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.plindex 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.hindex 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_CHANGESindex 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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, peter.eisentraut@enterprisedb.com, dgrowleyml@gmail.com
Subject: Re: automatically generating node support functions
In-Reply-To: <1249010.1657574337@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox