Обсуждение: Do we still need gen_node_support.pl's nodetag ABI stability check?

Поиск
Список
Период
Сортировка

Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Tom Lane
Дата:
We're still a couple months away from cutting the REL_19_STABLE
branch, but I was contemplating that just now, and it occurred to me
to wonder whether we still need gen_node_support.pl's single-purpose
ABI check (cf commit eea9fa9b2) now that we have buildfarm animals
running general-purpose ABI stability checks.

On the one hand, there's much to be said for belt-and-suspenders-too
safety checks.  On the other hand, updating gen_node_support.pl is
an extra manual step while creating a branch, so it's easy to forget
or get wrong.  It's also not very clear why this particular sort
of ABI break in a stable branch is any worse than other hazards.

I'm not really set either way, but my first thought is to drop
the special mechanism.

            regards, tom lane



Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Tom Lane
Дата:
I wrote:
> We're still a couple months away from cutting the REL_19_STABLE
> branch, but I was contemplating that just now, and it occurred to me
> to wonder whether we still need gen_node_support.pl's single-purpose
> ABI check (cf commit eea9fa9b2) now that we have buildfarm animals
> running general-purpose ABI stability checks.

> On the one hand, there's much to be said for belt-and-suspenders-too
> safety checks.  On the other hand, updating gen_node_support.pl is
> an extra manual step while creating a branch, so it's easy to forget
> or get wrong.  It's also not very clear why this particular sort
> of ABI break in a stable branch is any worse than other hazards.

> I'm not really set either way, but my first thought is to drop
> the special mechanism.

Hearing no objections, I went ahead and wrote a patch for that.
Doing that reminded me that it's a really incomplete check anyway,
as it only verifies that the last auto-assigned NodeTag number
hasn't changed.  Re-ordering earlier entries, for example, would
not get detected.  So even on its own terms it's little more than
a stopgap; the buildfarm's libabigail checks are far more thorough.

Barring objections, I'll push this soon.

            regards, tom lane

From c01ddb5be22c5e2fd1ed7ee429ae489da8d4f0b2 Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Wed, 15 Apr 2026 14:08:12 -0400
Subject: [PATCH v1] Remove gen_node_support.pl's ad-hoc ABI stability check.

We installed this in commit eea9fa9b2 to protect against foreseeable
mistakes that would break ABI in stable branches by renumbering
NodeTag enum entries.  However, we now have much more thorough
ABI stability checks thanks to buildfarm members using libabigail
(see the .abi-compliance-history mechanism).  So this incomplete,
single-purpose check seems like an anachronism.  I wouldn't object
to keeping it were it not that it requires an additional manual step
when making a new stable git branch.  That seems like something easy
to screw up, so let's get rid of it.

This patch just removes the logic that checks for changes in the last
auto-assigned NodeTag value.  We still need eea9fa9b2's cross-check
on the supplied list of header files, to prevent divergence between
the makefile and meson build systems.  We'll also sometimes need the
nodetag_number() infrastructure for hand-assigning new NodeTags in
stable branches.

Discussion: https://postgr.es/m/1458883.1776143073@sss.pgh.pa.us
---
 src/backend/nodes/gen_node_support.pl | 25 +------------------------
 src/tools/RELEASE_CHANGES             |  4 ----
 2 files changed, 1 insertion(+), 28 deletions(-)

diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl
index a21f5a754bf..f4b1317e99f 100644
--- a/src/backend/nodes/gen_node_support.pl
+++ b/src/backend/nodes/gen_node_support.pl
@@ -96,20 +96,6 @@ my @nodetag_only_files = qw(
   nodes/supportnodes.h
 );

-# 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.  (Find these values in the last line of the current
-# nodetags.h file.)  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;

@@ -615,30 +601,21 @@ open my $nt, '>', "$output_path/nodetags.h$tmpext"
 printf $nt $header_comment, 'nodetags.h';

 my $tagno = 0;
-my $last_tag = undef;
 foreach my $n (@node_types, @extra_tags)
 {
     next if elem $n, @abstract_types;
     if (defined $manual_nodetag_number{$n})
     {
-        # do not change $tagno or $last_tag
+        # do not change $tagno
         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 != $tagno);
-
 close $nt;


diff --git a/src/tools/RELEASE_CHANGES b/src/tools/RELEASE_CHANGES
index f0d005a6141..653318fd770 100644
--- a/src/tools/RELEASE_CHANGES
+++ b/src/tools/RELEASE_CHANGES
@@ -118,10 +118,6 @@ Starting a New Development Cycle
 * In the newly-made branch, replace "devel" with the branch's major version
   number in the URLs appearing in the top-level README and Makefile files.

-* 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.
--
2.43.7


Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Daniel Gustafsson
Дата:
> On 15 Apr 2026, at 20:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Barring objections, I'll push this soon.

+1, the new checks are far more comprehensive.  Looks good from a readthrough
and grepping the codebase doesn't turn up any other mentions.

--
Daniel Gustafsson




Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Peter Eisentraut
Дата:
On 14.04.26 07:04, Tom Lane wrote:
> On the one hand, there's much to be said for belt-and-suspenders-too
> safety checks.  On the other hand, updating gen_node_support.pl is
> an extra manual step while creating a branch, so it's easy to forget
> or get wrong.  It's also not very clear why this particular sort
> of ABI break in a stable branch is any worse than other hazards.

This might still be helpful because it checks during local builds and 
doesn't rely on the buildfarm.




Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Daniel Gustafsson
Дата:
> On 15 Apr 2026, at 21:30, Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 14.04.26 07:04, Tom Lane wrote:
>> On the one hand, there's much to be said for belt-and-suspenders-too
>> safety checks.  On the other hand, updating gen_node_support.pl is
>> an extra manual step while creating a branch, so it's easy to forget
>> or get wrong.  It's also not very clear why this particular sort
>> of ABI break in a stable branch is any worse than other hazards.
>
> This might still be helpful because it checks during local builds and doesn't rely on the buildfarm.

But does it actually give a good enough answer to be relied upon when passing
the local check can fail the buildfarm check?

--
Daniel Gustafsson




Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Tom Lane
Дата:
Daniel Gustafsson <daniel@yesql.se> writes:
> On 15 Apr 2026, at 21:30, Peter Eisentraut <peter@eisentraut.org> wrote:
>> This might still be helpful because it checks during local builds and doesn't rely on the buildfarm.

> But does it actually give a good enough answer to be relied upon when passing
> the local check can fail the buildfarm check?

Yeah, my answer to that is still "why is this particular case more
important than any other ABI breakage you might cause while hacking
on a back branch?".  I quite agree that being able to check for ABI
breakage locally can be useful.  But what we ought to do is make it
easier for people to use libabigail for that without spinning up a
local buildfarm instance.  Perhaps we could extract the buildfarm's
ABICompCheck.pm script into some standalone tool.

            regards, tom lane



Re: Do we still need gen_node_support.pl's nodetag ABI stability check?

От
Daniel Gustafsson
Дата:
> On 16 Apr 2026, at 03:46, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Daniel Gustafsson <daniel@yesql.se> writes:
>> On 15 Apr 2026, at 21:30, Peter Eisentraut <peter@eisentraut.org> wrote:
>>> This might still be helpful because it checks during local builds and doesn't rely on the buildfarm.
>
>> But does it actually give a good enough answer to be relied upon when passing
>> the local check can fail the buildfarm check?
>
> Yeah, my answer to that is still "why is this particular case more
> important than any other ABI breakage you might cause while hacking
> on a back branch?".  I quite agree that being able to check for ABI
> breakage locally can be useful.

Agreed.

> But what we ought to do is make it
> easier for people to use libabigail for that without spinning up a
> local buildfarm instance.  Perhaps we could extract the buildfarm's
> ABICompCheck.pm script into some standalone tool.

While I have zero insights into how complicated that would be, off the cuff it
seems like the right approach.

--
Daniel Gustafsson