From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
        David Rowley <dgrowleyml@gmail.com>,
        pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: automatically generating node support functions
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>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Mon, 11 Jul 2022 16:04:00 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1248765.1657574239.0@sss.pgh.pa.us>
Date: Mon, 11 Jul 2022 17:18:57 -0400
Message-ID: <1249010.1657574337@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1249010.1657574337%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1248765.1657574239.1@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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="add-ABI-stability-provisions-for-nodetag-list.patch";
	charset="us-ascii"
Content-ID: <1248765.1657574239.2@sss.pgh.pa.us>
Content-Description: add-ABI-stability-provisions-for-nodetag-list.patch
Content-Transfer-Encoding: quoted-printable

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 mat=
ch
+# reality, providing a cross-check that we haven't broken ABI by adding o=
r
+# removing nodetags.
+# In HEAD, these variables should be left undef, since we don't promise
+# ABI stability during development.
+
+my $last_nodetag    =3D undef;
+my $last_nodetag_no =3D 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 =3D~ /^nodetag_number\((\d+)\)$/)
+						{
+							$manual_nodetag_number{$in_struct} =3D $1;
+						}
 						else
 						{
 							die
@@ -472,14 +493,31 @@ open my $nt, '>', 'nodetags.h' . $tmpext or die $!;
 =

 printf $nt $header_comment, 'nodetags.h';
 =

-my $i =3D 1;
+my $tagno    =3D 0;
+my $last_tag =3D undef;
 foreach my $n (@node_types, @extra_tags)
 {
 	next if elem $n, @abstract_types;
-	print $nt "\tT_${n} =3D $i,\n";
-	$i++;
+	if (defined $manual_nodetag_number{$n})
+	{
+		# do not change $tagno or $last_tag
+		print $nt "\tT_${n} =3D $manual_nodetag_number{$n},\n";
+	}
+	else
+	{
+		$tagno++;
+		$last_tag =3D $n;
+		print $nt "\tT_${n} =3D $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_n=
o\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 o=
f
+ *   an auto-generated number.  Typically this would only be used in stab=
le
+ *   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 ma=
rked
  * abstract: if a node struct appears as the first field of another struc=
t
  * type, then it is the supertype of that type.  The no_copy, no_equal, a=
nd
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.

------- =_aaaaaaaaaa0--