Обсуждение: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()

My recent commit 688dc62, which was back-patched to v18, has made the
abi-compliance-check on buildfarm member baza unhappy:

    https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=baza&dt=2025-10-17%2013%3A11%3A11

Specifically, I replaced two functions related to lookups/privilege checks
for the new stats stuff in v18 with RangeVarGetRelidExtended().  FWIW I did
check codesearch.debian.net and GitHub for any third-party usage of these
functions before committing, and I found none.  Also, these functions are
only present in exactly one release (18.0).

My thinking was that this ABI breakage was probably fine, as I don't think
we really intended for these functions to be used elsewhere.  However,
since we have a buildfarm failure, I thought it best to broadcast my
thought process.  While I judged back-patching worth the risk, I could live
with reverting the change on v18 if anyone is concerned.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> My thinking was that this ABI breakage was probably fine, as I don't think
> we really intended for these functions to be used elsewhere.  However,
> since we have a buildfarm failure, I thought it best to broadcast my
> thought process.  While I judged back-patching worth the risk, I could live
> with reverting the change on v18 if anyone is concerned.

I don't have a problem with the change you made.  I do have a problem
with baza having spun up this check before we settled on a way to
manage it.  AFAIK we don't have any process by which we can decide
that a reported ABI change is acceptable and then clear the failure.
There was some discussion of how to control it [1], but nothing's been
done yet.

FWIW, I favor the approach of having an in-tree, per-branch file
containing the commit hash of a commit that is the current ABI
reference for that branch.  If the file doesn't exist (which it
wouldn't in master, and probably not in recently-forked branches),
skip ABI checking.  I think this is superior to the discussed
alternative of depending on git tags, because files are easy to
change or remove, while tags are not.  In particular, I think it'd
likely be impossible to make the ABI reference point go backwards
if we use tags.  Maybe that's not a case we'd ever need, but I'm
unconvinced of that.

            regards, tom lane

[1] https://www.postgresql.org/message-id/438875.1752433368%40sss.pgh.pa.us

On Fri, Oct 17, 2025 at 1:15 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't have a problem with the change you made.  I do have a problem
> with baza having spun up this check before we settled on a way to
> manage it.

+1.

> AFAIK we don't have any process by which we can decide
> that a reported ABI change is acceptable and then clear the failure.
> There was some discussion of how to control it [1], but nothing's been
> done yet.

The fact that this is now causing problems was entirely predictable
(and was in fact predicted). Having a way to suppress individual
warnings that are deemed to be invalid is 100% essential if we're
detecting ABI changes on a buildfarm animal like this.

--
Peter Geoghegan

On Fri, Oct 17, 2025 at 01:15:20PM -0400, Tom Lane wrote:
> FWIW, I favor the approach of having an in-tree, per-branch file
> containing the commit hash of a commit that is the current ABI
> reference for that branch.  If the file doesn't exist (which it
> wouldn't in master, and probably not in recently-forked branches),
> skip ABI checking.  I think this is superior to the discussed
> alternative of depending on git tags, because files are easy to
> change or remove, while tags are not.  In particular, I think it'd
> likely be impossible to make the ABI reference point go backwards
> if we use tags.  Maybe that's not a case we'd ever need, but I'm
> unconvinced of that.

I'm new to the topic, but IMHO the per-branch file approach is by far the
best approach.  Not only is it much more flexible, but we could even use it
as a centralized list of ABI breaks for a given branch with justification
for each.  I can't think of any strong advantages of keeping this stuff in
git metadata.  git itself uses a file for blame.ignoreRevsFile...

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Oct 17, 2025 at 01:15:20PM -0400, Tom Lane wrote:
>> FWIW, I favor the approach of having an in-tree, per-branch file
>> containing the commit hash of a commit that is the current ABI
>> reference for that branch.

> I'm new to the topic, but IMHO the per-branch file approach is by far the
> best approach.  Not only is it much more flexible, but we could even use it
> as a centralized list of ABI breaks for a given branch with justification
> for each.  I can't think of any strong advantages of keeping this stuff in
> git metadata.  git itself uses a file for blame.ignoreRevsFile...

Good idea.  We'd have to allow comments in the file, but that's
probably a good thing anyway.

            regards, tom lane

On Fri, Oct 17, 2025 at 02:45:12PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Fri, Oct 17, 2025 at 01:15:20PM -0400, Tom Lane wrote:
>>> FWIW, I favor the approach of having an in-tree, per-branch file
>>> containing the commit hash of a commit that is the current ABI
>>> reference for that branch.
> 
>> I'm new to the topic, but IMHO the per-branch file approach is by far the
>> best approach.  Not only is it much more flexible, but we could even use it
>> as a centralized list of ABI breaks for a given branch with justification
>> for each.  I can't think of any strong advantages of keeping this stuff in
>> git metadata.  git itself uses a file for blame.ignoreRevsFile...
> 
> Good idea.  We'd have to allow comments in the file, but that's
> probably a good thing anyway.

I've attached a first try.  You'll notice that I have borrowed heavily from
.git-blame-ignore-revs.  Some other things that might be worthwhile:

* Add commentary about when this file is needed (i.e., after the .0).
* Add instructions for creating file on new stable branch to
RELEASE_CHANGES.
* Adjust format for readability.  It is a bit comment-heavy at the moment.

Anything else?  I suppose this idea is entirely dependent on the
maintainers of the abi-compliance-check code to adapt to it, so we'll need
buy-in from them, too.

-- 
nathan

On Fri, Oct 17, 2025 at 3:11 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Anything else?  I suppose this idea is entirely dependent on the
> maintainers of the abi-compliance-check code to adapt to it, so we'll need
> buy-in from them, too.

That would require parsing the file and understanding that any
compliance failures associated with a given commit should be
suppressed. But that seems decidedly nontrivial to me. I can easily
think of (admittedly somewhat contrived) scenarios where it's
basically impossible to make this work due to transitive dependencies
across commits.

I suspect that any practical approach to solving this problem will
have to involve ignore files that look somewhat like a Valgrind
suppression file. It'll have to be based on symbol names, plus
possibly a specific ABI breakage  type.

--
Peter Geoghegan

Nathan Bossart <nathandbossart@gmail.com> writes:
> I've attached a first try.  You'll notice that I have borrowed heavily from
> .git-blame-ignore-revs.  Some other things that might be worthwhile:

There would need to be an initial entry at the time the file is
created, which would presumably point to some commit shortly before
the .0 version stamp is applied (or maybe we'd choose to do it around
rc1).  The mockup should include that.

I'd be slightly inclined to have just one non-comment line, which
is the active reference hash value, and all the rest be comments.
The way you have it here requires the reading code to be smart
about end-of-line comments, which is code complexity we don't need
and doesn't seem amazingly legible either.  OTOH, the precedent of
.git-blame-ignore-revs may be worth following regardless of our
personal druthers.

            regards, tom lane

On Fri, Oct 17, 2025 at 03:20:55PM -0400, Peter Geoghegan wrote:
> On Fri, Oct 17, 2025 at 3:11 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> Anything else?  I suppose this idea is entirely dependent on the
>> maintainers of the abi-compliance-check code to adapt to it, so we'll need
>> buy-in from them, too.
> 
> That would require parsing the file and understanding that any
> compliance failures associated with a given commit should be
> suppressed. But that seems decidedly nontrivial to me. I can easily
> think of (admittedly somewhat contrived) scenarios where it's
> basically impossible to make this work due to transitive dependencies
> across commits.

I was imagining this working more like what Tom suggested.  IOW we'd use
the latest commit listed in the file (perhaps always the first one) as the
baseline.  Of course, this doesn't work too well if we have a bunch of ABI
breaks between buildfarm checks.  But my guess is that we could deal with
that pretty easily (e.g., make sure the buildfarm member in question runs
for every commit on the stable branch).

-- 
nathan

Peter Geoghegan <pg@bowt.ie> writes:
> That would require parsing the file and understanding that any
> compliance failures associated with a given commit should be
> suppressed. But that seems decidedly nontrivial to me.

No, I do not think there is any expectation of that.  The idea of
this file IMO is to record a "blessed" commit which the buildfarm
should compare branch tip to.  Nathan is proposing that the file
should also have the function of recording all previous blessed
commits for historical purposes.  That's not essential but I can
see some value in it.  All it requires from the buildfarm code
is that it take the first non-comment entry in the file.

            regards, tom lane

On Fri, Oct 17, 2025 at 3:28 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> I was imagining this working more like what Tom suggested.  IOW we'd use
> the latest commit listed in the file (perhaps always the first one) as the
> baseline.

You said "I suppose this idea is entirely dependent on the maintainers
of the abi-compliance-check code to adapt to it", which I understood
to mean that you thought that the upstream tool would somehow be made
to accept these kinds of ignore files. Obviously I misunderstood.

> Of course, this doesn't work too well if we have a bunch of ABI
> breaks between buildfarm checks.  But my guess is that we could deal with
> that pretty easily (e.g., make sure the buildfarm member in question runs
> for every commit on the stable branch).

In practice I think that it would be up to the person writing the next
suppression to verify that there were no unrelated changes in the
interim between their new blessed/suppression commit and the prior
one. That doesn't seem super onerous to me, given that even false
positives don't seem to be all that common with
abi-compliance-checker.

--
Peter Geoghegan

On Fri, Oct 17, 2025 at 03:27:10PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> I've attached a first try.  You'll notice that I have borrowed heavily from
>> .git-blame-ignore-revs.  Some other things that might be worthwhile:
> 
> There would need to be an initial entry at the time the file is
> created, which would presumably point to some commit shortly before
> the .0 version stamp is applied (or maybe we'd choose to do it around
> rc1).  The mockup should include that.

Sure, makes sense.

> I'd be slightly inclined to have just one non-comment line, which
> is the active reference hash value, and all the rest be comments.
> The way you have it here requires the reading code to be smart
> about end-of-line comments, which is code complexity we don't need
> and doesn't seem amazingly legible either.  OTOH, the precedent of
> .git-blame-ignore-revs may be worth following regardless of our
> personal druthers.

That crossed my mind, too.  I'm personally not too concerned about small
deviations from .git-blame-ignore-revs, especially if it improves
machine/human readability.

-- 
nathan

On Fri, Oct 17, 2025 at 03:33:28PM -0400, Peter Geoghegan wrote:
> On Fri, Oct 17, 2025 at 3:28 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I was imagining this working more like what Tom suggested.  IOW we'd use
>> the latest commit listed in the file (perhaps always the first one) as the
>> baseline.
> 
> You said "I suppose this idea is entirely dependent on the maintainers
> of the abi-compliance-check code to adapt to it", which I understood
> to mean that you thought that the upstream tool would somehow be made
> to accept these kinds of ignore files. Obviously I misunderstood.

Sorry, I wasn't clear there.

>> Of course, this doesn't work too well if we have a bunch of ABI
>> breaks between buildfarm checks.  But my guess is that we could deal with
>> that pretty easily (e.g., make sure the buildfarm member in question runs
>> for every commit on the stable branch).
> 
> In practice I think that it would be up to the person writing the next
> suppression to verify that there were no unrelated changes in the
> interim between their new blessed/suppression commit and the prior
> one. That doesn't seem super onerous to me, given that even false
> positives don't seem to be all that common with
> abi-compliance-checker.

Agreed.  Even if someone forgets to do that validation, the chances of
missing something seem low.

-- 
nathan

Hackers,

Adding Mankirat, who developed the ABI checker for his GSoC project.

>> Good idea.  We'd have to allow comments in the file, but that's
>> probably a good thing anyway.
>
> I've attached a first try.  You'll notice that I have borrowed heavily from
> .git-blame-ignore-revs.  Some other things that might be worthwhile:
>
> * Add commentary about when this file is needed (i.e., after the .0).
> * Add instructions for creating file on new stable branch to
> RELEASE_CHANGES.
> * Adjust format for readability.  It is a bit comment-heavy at the moment.
>
> Anything else?  I suppose this idea is entirely dependent on the
> maintainers of the abi-compliance-check code to adapt to it, so we'll need
> buy-in from them, too.

Is the idea that the ABI checker just has to scan the first non-comment line that starts with a commit identifier (SHA
ortag)? Example from your patch: 

```
# This file lists commits on the REL_18_STABLE branch that break ABI
# compatibility in ways that have been deemed acceptable (e.g., removing an
# extern function with no third-party uses).  The primary intent of this file
# is to placate the ABI compliance checks on the buildfarm, but it also serves
# as a central location to document the justification for each.
#
# Add new entries by adding the output of the following to the top of the file:
#
# $ git log --pretty=format:"%H # %cd%n# %s" $ABIBREAKGITHASH -1 --date=iso
#
# Be sure to include additional context in a comment below the entry.

c8af5019bee5c57502db830f8005a01cba60fee0 # 2025-10-15 12:47:33 -0500
# Fix lookups in pg_{clear,restore}_{attribute,relation}_stats().
#
# This commit replaced two functions related to lookups/privilege checks for
# the new stats stuff in v18 with RangeVarGetRelidExtended().  These functions
# were not intended for use elsewhere, exist in exactly one release (18.0), and
# do not have any known third-party callers.
--
2.39.5 (Apple Git-154)
```

Seems totally do-able, though I don’t know what that `(Apple Git-154)` bit is doing at the end. I presume it would list
thehistory of changes in reverse chronological order, yes? 

If there is a tag _AFTER_ the listed SHA, should we prefer that tag as the baseline?

Best,

David

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Oct 17, 2025 at 03:33:28PM -0400, Peter Geoghegan wrote:
>> In practice I think that it would be up to the person writing the next
>> suppression to verify that there were no unrelated changes in the
>> interim between their new blessed/suppression commit and the prior
>> one. That doesn't seem super onerous to me, given that even false
>> positives don't seem to be all that common with
>> abi-compliance-checker.

> Agreed.  Even if someone forgets to do that validation, the chances of
> missing something seem low.

I don't see a race condition here.  What would happen is we make
some commit, realizing either at the time or later that it involves
an ABI break.  We verify via some later buildfarm run that the
break is as-expected (ie the commit doesn't introduce any unwanted
changes, nor is there anything hanging around from some older commit).
Then we push an update to the .abi_reference file that points at
that commit, and the buildfarm starts comparing ABI of branch tip
to that commit instead of whatever was the reference commit before.
No later activity breaks the conclusion that we were okay with the ABI
that that commit creates, nor can any earlier commit cause problems
so long as we did our due diligence in checking the ABI-break reports.

In theory, if two ABI-breaking commits go in so close together that
there was no ABI-checking buildfarm run in between, we might have
difficulty untangling their effects.  That doesn't seem very likely
in practice, and even if it happens, so what?  Either we're good with
the ABI-break report when we see it, or we're not.

            regards, tom lane

On Fri, Oct 17, 2025 at 03:53:09PM -0400, Tom Lane wrote:
> I don't see a race condition here.  What would happen is we make
> some commit, realizing either at the time or later that it involves
> an ABI break.  We verify via some later buildfarm run that the
> break is as-expected (ie the commit doesn't introduce any unwanted
> changes, nor is there anything hanging around from some older commit).
> Then we push an update to the .abi_reference file that points at
> that commit, and the buildfarm starts comparing ABI of branch tip
> to that commit instead of whatever was the reference commit before.
> No later activity breaks the conclusion that we were okay with the ABI
> that that commit creates, nor can any earlier commit cause problems
> so long as we did our due diligence in checking the ABI-break reports.
> 
> In theory, if two ABI-breaking commits go in so close together that
> there was no ABI-checking buildfarm run in between, we might have
> difficulty untangling their effects.  That doesn't seem very likely
> in practice, and even if it happens, so what?  Either we're good with
> the ABI-break report when we see it, or we're not.

Ah, I was thinking of a more proactive approach (e.g., I commit something
that I know introduces ABI breakage, and then I immediately update the ABI
reference file in the next commit).  I like the idea of simply reacting to
the reports and using that as an opportunity to verify it's what we expect.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Oct 17, 2025 at 03:53:09PM -0400, Tom Lane wrote:
>> I don't see a race condition here.  What would happen is we make
>> some commit, realizing either at the time or later that it involves
>> an ABI break.  We verify via some later buildfarm run that the
>> break is as-expected (ie the commit doesn't introduce any unwanted
>> changes, nor is there anything hanging around from some older commit).
>> Then we push an update to the .abi_reference file that points at
>> that commit,

> Ah, I was thinking of a more proactive approach (e.g., I commit something
> that I know introduces ABI breakage, and then I immediately update the ABI
> reference file in the next commit).  I like the idea of simply reacting to
> the reports and using that as an opportunity to verify it's what we expect.

Right.  This does mean that those BF members might stay red for a
little bit while we verify that we're seeing expected results, but
I think that's acceptable.  Trying to prevent the BF from ever
seeing a bad state seems to me to carry too much risk of masking
problems we didn't expect.

            regards, tom lane

On Fri, Oct 17, 2025 at 03:52:18PM -0400, David E. Wheeler wrote:
> Adding Mankirat, who developed the ABI checker for his GSoC project.

Thanks for chiming in.

> Is the idea that the ABI checker just has to scan the first non-comment
> line that starts with a commit identifier (SHA or tag)?

Yes.

> Seems totally do-able, though I don’t know what that `(Apple Git-154)`
> bit is doing at the end.

I think that's just telling you what version of git I used to create the
patch file.

> I presume it would list the history of changes in reverse chronological
> order, yes?

Yes, although we could change it to whatever we want.

> If there is a tag _AFTER_ the listed SHA, should we prefer that tag as
> the baseline?

I don't see any need to consider tags at all.  We'd initialize this file
when creating the new STABLE branch with a baseline commit near a release
candidate or the .0, and then we'd just add future baselines as needed.
The ABI checks would always use the latest baseline, even if it points to
something before the latest release tag.  (At least, this is how I'm
thinking about it.)

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Fri, Oct 17, 2025 at 03:52:18PM -0400, David E. Wheeler wrote:
>> If there is a tag _AFTER_ the listed SHA, should we prefer that tag as
>> the baseline?

> I don't see any need to consider tags at all.  We'd initialize this file
> when creating the new STABLE branch with a baseline commit near a release
> candidate or the .0, and then we'd just add future baselines as needed.
> The ABI checks would always use the latest baseline, even if it points to
> something before the latest release tag.  (At least, this is how I'm
> thinking about it.)

I agree.  I don't think the buildfarm should consider git tags at all
in this behavior.  One reason is that our release process dictates
applying tags at very specific times that aren't necessarily relevant
to deciding that an ABI break is or is not okay.  I think we want
moving the baseline to be a considered reaction to an observed ABI
report, and not an action that is automatic according to some other
process.

            regards, tom lane

On Oct 17, 2025, at 16:19, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I agree.  I don't think the buildfarm should consider git tags at all
> in this behavior.  One reason is that our release process dictates
> applying tags at very specific times that aren't necessarily relevant
> to deciding that an ABI break is or is not okay.  I think we want
> moving the baseline to be a considered reaction to an observed ABI
> report, and not an action that is automatic according to some other
> process.

Okay, so the rule is:

* If there is no .abi-compliance-history file, baseline from the latest tag
* Otherwise, baseline from the first SHA to appear in the file

Easy peasy.

D

"David E. Wheeler" <david@justatheory.com> writes:
> Okay, so the rule is:

> * If there is no .abi-compliance-history file, baseline from the latest tag
> * Otherwise, baseline from the first SHA to appear in the file

NO.  The rule is: if there's no such file, do not apply ABI checking.
We are not interested in ABI complaints against master.

            regards, tom lane

On Oct 17, 2025, at 17:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> Okay, so the rule is:
>
>> * If there is no .abi-compliance-history file, baseline from the latest tag
>> * Otherwise, baseline from the first SHA to appear in the file
>
> NO.  The rule is: if there's no such file, do not apply ABI checking.
> We are not interested in ABI complaints against master.

It only runs against maintenance branches.

D

"David E. Wheeler" <david@justatheory.com> writes:
> On Oct 17, 2025, at 17:51, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> NO.  The rule is: if there's no such file, do not apply ABI checking.
>> We are not interested in ABI complaints against master.

> It only runs against maintenance branches.

That seems overcomplicated: how does the buildfarm know
what's a maintenance branch?  I think the rule should be
just "run ABI checks if the control file exists, else not".

As an example of why that's better, what if we did decide
we wanted ABI checks on master?

            regards, tom lane

Mankirat Singh <mankiratsingh1315@gmail.com> writes:
> But if we want a much clearer way based on symbol names, we could use a
> standard per branch suppression file, which the abidiff tool directly
> supports for this purpose[1].

I am strongly against relying on suppression files.  I think that
is a very imprecise technology, and it is certainly harder to use
than the "choose a blessed reference point" approach.

            regards, tom lane

On Oct 17, 2025, at 19:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> That seems overcomplicated: how does the buildfarm know
> what's a maintenance branch?  I think the rule should be
> just "run ABI checks if the control file exists, else not".
>
> As an example of why that's better, what if we did decide
> we wanted ABI checks on master?

It’s part of the design of the build farm. The setup() function[0] checks various things to see if it should be run,
e.g.,

```perl
    if ($^O ne 'linux')
    {
        emit("Only Linux is supported for ABICompCheck Module, skipping.");
        return;
    }

    # Only proceed if this is a stable branch with git SCM, not using msvc
    if ($conf->{scm} ne 'git')
    {
        emit("Only git SCM is supported for ABICompCheck Module, skipping.");
        return;
    }
    if ($branch !~ /_STABLE$/)
    {
        emit("Skipping ABI check; '$branch' is not a stable branch.");
        return;
    }
```

So as long as the branch naming remains consistent it should work.

D


[0]
https://github.com/PGBuildFarm/client-code/pull/38/files#diff-207ca93813cc123f656dbb12b7723d305e9ade5e03d7b1cdb406180e4eaab9a2R194

On Oct 18, 2025, at 11:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I am strongly against relying on suppression files.  I think that
> is a very imprecise technology, and it is certainly harder to use
> than the "choose a blessed reference point" approach.

Seconded. I’m also not keen on using something specific to libabigail if, long term, we want to enable similar checks
onother platforms using other tools that may not support it suppression file format. 

Let’s do the baseline SHA file and see how it goes.

Best,

David

On Oct 18, 2025, at 22:48, Mankirat Singh <mankiratsingh1315@gmail.com> wrote:

> I've implemented the support for .abi-compliance-history file. If the file is present in a STABLE branch, it will be
usedby default; otherwise, the latest tag will be used. You can view the changes here[1]. 

I’ve now deployed this change (well, a subsequent change that does it better) to baza, so once the
.abi-compliance-historyhas been committed to the rev 18 branch (assuming the format is compatible), it should start
passingagain. 

Best,

David

"David E. Wheeler" <david@justatheory.com> writes:
> I’ve now deployed this change (well, a subsequent change that does it better) to baza, so once the
.abi-compliance-historyhas been committed to the rev 18 branch (assuming the format is compatible), it should start
passingagain. 

OK, I pushed a placeholder following Nathan's formatting proposal.
The ABI checker should still complain, because I made it point at
REL_18_0^, which is what I expect we'd do in practice.  After we
see it respond to that, we can move the reference point to where
it needs to be.

            regards, tom lane

On Oct 20, 2025, at 16:39, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> OK, I pushed a placeholder following Nathan's formatting proposal.
> The ABI checker should still complain, because I made it point at
> REL_18_0^, which is what I expect we'd do in practice.  After we
> see it respond to that, we can move the reference point to where
> it needs to be.

Nice timing, as Mankirat also recently updated the code to determine the files to compare based on feedback from Peter
E;see today’s failure[1] for an example. Looks like this: 

```
Branch: REL_18_STABLE
Git HEAD: 399a9e04e5491f8a76ffb482f4a86b9acb6f91fb
Changes since: REL_18_0

Binaries compared:
bin/postgres
lib/libecpg.so
lib/libecpg_compat.so
lib/libpgtypes.so
lib/libpq.so

log files for step abi-compliance-check:

 Leaf changes summary: 2 artifacts changed
Changed leaf types summary: 0 leaf type changed
Removed/Changed/Added functions summary: 2 Removed, 0 Changed, 0 Added function (2 filtered out)
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable

2 Removed functions:

  [D] 'function void stats_lock_check_privileges(Oid)'    {stats_lock_check_privileges}
  [D] 'function Oid stats_lookup_relid(const char*, const char*)'    {stats_lookup_relid}
```

Should look the same tomorrow but instead say “Changes since REL_18_0^”.

Best,

David

[1]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-10-20%2013%3A11%3A05&stg=abi-compliance-check

On Mon, Oct 20, 2025 at 10:39:19AM -0400, Tom Lane wrote:
> OK, I pushed a placeholder following Nathan's formatting proposal.
> The ABI checker should still complain, because I made it point at
> REL_18_0^, which is what I expect we'd do in practice.  After we
> see it respond to that, we can move the reference point to where
> it needs to be.

Thanks.  Here's a new patch set.  The v18 patch is just an update to the
.abi-compliance-history file that Tom committed.  The master patch adds
instructions for generating the file to RELEASE_NOTES.  I imagine we'll
want to add .abi-compliance-history files for the back-branches, too
(except for perhaps v13, which is about to go out of support in a couple
weeks).

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> Thanks.  Here's a new patch set.  The v18 patch is just an update to the
> .abi-compliance-history file that Tom committed.  The master patch adds
> instructions for generating the file to RELEASE_NOTES.

I'd tend to s/placate/control/, otherwise the proposed wording in the
file looks good.  I doubt we really need a script to generate the
file in the first place -- why wouldn't copying another branch's
boilerplate be good enough?  If you're set on having a script,
at least make it pre-fill the initial entry.  (Using branch HEAD
ought to be good enough for that.)

> I imagine we'll
> want to add .abi-compliance-history files for the back-branches, too
> (except for perhaps v13, which is about to go out of support in a couple
> weeks).

Agreed, but let's get v18 in shape first.  I imagine the back branches
will require some effort to fill in the correct reference commits.
I was expecting we'd commit initial values pointing at the .0 releases
and then seeing what the ABI checker moans about in each branch ...

            regards, tom lane

On Mon, Oct 20, 2025 at 01:07:04PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Thanks.  Here's a new patch set.  The v18 patch is just an update to the
>> .abi-compliance-history file that Tom committed.  The master patch adds
>> instructions for generating the file to RELEASE_NOTES.
> 
> I'd tend to s/placate/control/, otherwise the proposed wording in the
> file looks good.  I doubt we really need a script to generate the
> file in the first place -- why wouldn't copying another branch's
> boilerplate be good enough?  If you're set on having a script,
> at least make it pre-fill the initial entry.  (Using branch HEAD
> ought to be good enough for that.)

I'm fine with leaving out the script if you are.  It was only aimed at
making the release checklist a little less cumbersome, but even without the
script it's a whopping minute or two of effort that only needs to happen
once per year.  I've probably already spent far more time automating it
than makes sense [0].

>> I imagine we'll
>> want to add .abi-compliance-history files for the back-branches, too
>> (except for perhaps v13, which is about to go out of support in a couple
>> weeks).
> 
> Agreed, but let's get v18 in shape first.  I imagine the back branches
> will require some effort to fill in the correct reference commits.
> I was expecting we'd commit initial values pointing at the .0 releases
> and then seeing what the ABI checker moans about in each branch ...

Agreed.

[0] https://xkcd.com/1205/

-- 
nathan

On Mon, Oct 20, 2025 at 12:14:09PM -0500, Nathan Bossart wrote:
> On Mon, Oct 20, 2025 at 01:07:04PM -0400, Tom Lane wrote:
>> I'd tend to s/placate/control/, otherwise the proposed wording in the
>> file looks good.  I doubt we really need a script to generate the
>> file in the first place -- why wouldn't copying another branch's
>> boilerplate be good enough?  If you're set on having a script,
>> at least make it pre-fill the initial entry.  (Using branch HEAD
>> ought to be good enough for that.)
> 
> I'm fine with leaving out the script if you are.  It was only aimed at
> making the release checklist a little less cumbersome, but even without the
> script it's a whopping minute or two of effort that only needs to happen
> once per year.  I've probably already spent far more time automating it
> than makes sense [0].

Here is an updated patch set.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> Here is an updated patch set.

The v3 patches work for me.

BTW, not critical right now, but thought I'd mention it: ISTM
this mechanism obviates the need for the single-purpose NodeTag ABI
checks installed by commit eea9fa9b2.  We still need the checks in
gen_node_support.pl to ensure that the makefiles and meson files
build things the same way, but we shouldn't need the parts that were
intended to prevent accidental ABI changes in the back branches.
Since that stuff requires nonzero manual maintenance, I plan to get
rid of it once we're satisfied that the ABI checker is working well.

            regards, tom lane

On Mon, Oct 20, 2025 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
> > Here is an updated patch set.
>
> The v3 patches work for me.
>
> BTW, not critical right now, but thought I'd mention it: ISTM
> this mechanism obviates the need for the single-purpose NodeTag ABI
> checks installed by commit eea9fa9b2.  We still need the checks in
> gen_node_support.pl to ensure that the makefiles and meson files
> build things the same way, but we shouldn't need the parts that were
> intended to prevent accidental ABI changes in the back branches.
> Since that stuff requires nonzero manual maintenance, I plan to get
> rid of it once we're satisfied that the ABI checker is working well.

Hmm, but: the code in gen_node_support.pl would tell you about trouble
before you committed, whereas the ABI checks would only tell you about
trouble after you commit. It seems to me that we are in desperate need
of reducing, rather than increasing, the number of mistakes you can
make and find out about only after commit.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Oct 20, 2025 at 3:34 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> BTW, not critical right now, but thought I'd mention it: ISTM
>> this mechanism obviates the need for the single-purpose NodeTag ABI
>> checks installed by commit eea9fa9b2.

> Hmm, but: the code in gen_node_support.pl would tell you about trouble
> before you committed, whereas the ABI checks would only tell you about
> trouble after you commit. It seems to me that we are in desperate need
> of reducing, rather than increasing, the number of mistakes you can
> make and find out about only after commit.

There are enough other ways to break ABI that I don't think catching
just this one before commit will move the needle for anyone.  Instead,
the mistake I'm hoping to eliminate here is forgetting to arm the
NodeTag ABI check, or doing it wrong.

I do take your point that being able to find things before commit
is helpful.  But I think the answer to that is to get this
general-purpose ABI check mechanism sufficiently well product-ized
that committers can run it locally if they choose.  Ideally we'd
have multiple BF animals running it, so there's definitely motivation
to get it at least to the point where it doesn't require hand-feeding
by BF owners.  (If memory serves, we've had ABI breaks that affected
only 32 bit or only 64 bit machines, and of course there's the
possibility of ones that only manifest with particular feature
selections.  So I'm not content with just one animal running it.)

            regards, tom lane

On Mon, Oct 20, 2025 at 03:33:53PM -0400, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> Here is an updated patch set.
> 
> The v3 patches work for me.

Thanks.  If baza's next run looks good, I'll proceed with committing.

-- 
nathan

On Oct 20, 2025, at 19:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Agreed, but let's get v18 in shape first.  I imagine the back branches
> will require some effort to fill in the correct reference commits.
> I was expecting we'd commit initial values pointing at the .0 releases
> and then seeing what the ABI checker moans about in each branch …

FWIW they all fail. I pinned them all to the .1 tags for a couple days last month. Results:

curiously, all the back branches failed when comparing to their .1 tags.

* [REL_18_RC1 =>
c70b6db](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2013%3A10%3A34&stg=abi-compliance-check)
* [REL_17_1 =>
6195afb](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2012%3A54%3A38&stg=abi-compliance-check)
* [REL_16_1 =>
21a61b8](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2012%3A37%3A45&stg=abi-compliance-check)
* [REL_15_1 =>
f871fba](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2012%3A21%3A55&stg=abi-compliance-check)
* [REL_14_1 =>
ea65c88](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2012%3A10%3A11&stg=abi-compliance-check)
* [REL_13_1 =>
dbef9cb](https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-05%2012%3A00%3A02&stg=abi-compliance-check)

D

On Oct 20, 2025, at 22:52, Nathan Bossart <nathandbossart@gmail.com> wrote:

> Thanks.  If baza's next run looks good, I'll proceed with committing.

I suggest mentioning that new entries should be at the top, that the list should be in reverse chronological order.

Otherwise this looks great, love seeing it!

D

On Oct 20, 2025, at 22:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I do take your point that being able to find things before commit
> is helpful.  But I think the answer to that is to get this
> general-purpose ABI check mechanism sufficiently well product-ized
> that committers can run it locally if they choose.  Ideally we'd
> have multiple BF animals running it, so there's definitely motivation
> to get it at least to the point where it doesn't require hand-feeding
> by BF owners.  (If memory serves, we've had ABI breaks that affected
> only 32 bit or only 64 bit machines, and of course there's the
> possibility of ones that only manifest with particular feature
> selections.  So I'm not content with just one animal running it.)

FWIW, running it on a Linux animal currently requires just a couple steps

1. Download the module:

```sh
curl -LO
https://raw.githubusercontent.com/MankiratSingh1315/pg-bf-client-code/refs/heads/abi-comp-check/PGBuild/Modules/ABICompCheck.pm
mv ABICompCheck.pm build-farm-path/PGBuild/Modules/
```

2. Add it to `modules` in `build-farm.conf`, e.g.,

    modules => [qw(TestUpgrade ABICompCheck)],

3. Install the abigail suite; I believe the Debian packages are `abigail-tools` and `libabigail0`

I think that’s it. I use `run_branches.pl --run-all` to test all the current maintenance branches. It does not run
againstmaster. 

Best,

David

On Mon, Oct 20, 2025 at 11:22:29PM +0200, David E. Wheeler wrote:
> I suggest mentioning that new entries should be at the top, that the list
> should be in reverse chronological order.

I felt that this was already covered in the existing commentary.

> Otherwise this looks great, love seeing it!

Thanks for looking.  Now that we have a new run from baza that appears to
be using the updated baseline, I've committed the remaining patches.

For the rest of the back-branches, I'm considering starting with a baseline
of the latest minor version stamps.  While it would be nice to have a
comprehensive history of the ABI compatibility for each major version,
we've lived this long without it, and I think it's unlikely that we'd act
on any breakages that predate the latest release set.  Thoughts?

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> For the rest of the back-branches, I'm considering starting with a baseline
> of the latest minor version stamps.  While it would be nice to have a
> comprehensive history of the ABI compatibility for each major version,
> we've lived this long without it, and I think it's unlikely that we'd act
> on any breakages that predate the latest release set.  Thoughts?

Agreed that building a full list of ABI-changing commits in those
branches is probably not worth the trouble at this point.  (My OCD
side kind of wants to do it anyway ... but it's hard to argue that
we'd get real value out of it, or that we'd change anything now
unless we get complaints.)

I searched the commit log, and found that the most recent intentional,
or at least documented-in-the-log, ABI break was here:

Author: Masahiko Sawada <msawada@postgresql.org>
Branch: master Release: REL_18_BR [d87d07b7a] 2025-06-16 17:36:01 -0700
Branch: REL_17_STABLE Release: REL_17_6 [45c357e0e] 2025-06-16 17:35:58 -0700
Branch: REL_16_STABLE Release: REL_16_10 [b2ae07720] 2025-06-16 17:35:55 -0700
Branch: REL_15_STABLE Release: REL_15_14 [fc0fb77c5] 2025-06-16 17:35:53 -0700
Branch: REL_14_STABLE Release: REL_14_19 [983b36362] 2025-06-16 17:35:50 -0700
Branch: REL_13_STABLE Release: REL_13_22 [1230be12f] 2025-06-16 17:35:48 -0700

    ...
    Note that this commit adds two new fields to ReorderBufferTXN to store
    the distributed transactions. This change breaks ABI compatibility in
    back branches, affecting third-party extensions that depend on the
    size of the ReorderBufferTXN struct, though this scenario seems
    unlikely.
    ...

I propose that we should make .abi-compliance-history point at these
commits rather than REL_17_6 et al.  If there is anything later than
this that affected ABI, it'd be worth finding out I think, even though
we might choose not to do anything about it.  (In effect, we'd be
starting the ABI compliance histories there rather than all the way
back at the .0 releases.)

            regards, tom lane

On Tue, Oct 21, 2025 at 04:13:37PM -0400, Tom Lane wrote:
> I propose that we should make .abi-compliance-history point at these
> commits rather than REL_17_6 et al.  If there is anything later than
> this that affected ABI, it'd be worth finding out I think, even though
> we might choose not to do anything about it.  (In effect, we'd be
> starting the ABI compliance histories there rather than all the way
> back at the .0 releases.)

WFM.  I'll make it so.

I see that baza is currently using the latest tags for <v18.  David, will
it start using the .abi-compliance-history file on the rest of the
back-branches once it is added?

-- 
nathan

On Oct 21, 2025, at 22:21, Nathan Bossart <nathandbossart@gmail.com> wrote:

> I see that baza is currently using the latest tags for <v18.  David, will
> it start using the .abi-compliance-history file on the rest of the
> back-branches once it is added?

It should, yes. I added the latest changes from Mankirat to add support for it.

D

On Tue, Oct 21, 2025 at 10:44:19PM +0200, David E. Wheeler wrote:
> On Oct 21, 2025, at 22:21, Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I see that baza is currently using the latest tags for <v18.  David, will
>> it start using the .abi-compliance-history file on the rest of the
>> back-branches once it is added?
> 
> It should, yes. I added the latest changes from Mankirat to add support
> for it.

Great.  All back-branches now have an .abi-compliance-history file.

-- 
nathan

Nathan Bossart <nathandbossart@gmail.com> writes:
> Great.  All back-branches now have an .abi-compliance-history file.

Looks like we're good on v13-v16, but v17 is complaining

1 Removed function:

  [D] 'function bool check_max_slot_wal_keep_size(int*, void**, GucSource)'    {check_max_slot_wal_keep_size}

so I guess we need to add 24f6c1bd4 to that history.

            regards, tom lane

On Wed, Oct 22, 2025 at 10:40:50AM -0400, Tom Lane wrote:
> so I guess we need to add 24f6c1bd4 to that history.

Yup.  Will do.

-- 
nathan

On Wed, Oct 22, 2025 at 11:33:20AM -0500, Nathan Bossart wrote:
> On Wed, Oct 22, 2025 at 10:40:50AM -0400, Tom Lane wrote:
>> so I guess we need to add 24f6c1bd4 to that history.
> 
> Yup.  Will do.

Ope, looks like you took care of it already, thanks.

-- 
nathan

On Oct 22, 2025, at 18:36, Nathan Bossart <nathandbossart@gmail.com> wrote:

>>> so I guess we need to add 24f6c1bd4 to that history.
>>
>> Yup.  Will do.
>
> Ope, looks like you took care of it already, thanks.

Looks like everything’s back to green. Thanks everyone, this sort of setup is exactly what I had hoped to see from
Mankirat’sproject, and I appreciate hackers’ support for it! 

Best,

David