Re: automatically generating node support functions - Mailing list pgsql-hackers

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

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.