abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()

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]/messages/by-id/438875.1752433368@sss.pgh.pa.us, 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]: /messages/by-id/438875.1752433368@sss.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 or tag)? 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 the history 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