Thread
Commits
-
Add commit 24f6c1bd4 to v17 .abi-compliance-history.
- 5c659da980e1 17.7 landed
-
Add .abi-compliance-history to back-branches.
- eaf73340cd46 16.11 landed
- d9c5e7129d2b 15.15 landed
- a91f2d822522 17.7 landed
- 7d47278933f4 13.23 landed
- 58ba7e5ebfdf 14.20 landed
-
Add reminder to create .abi-compliance-history.
- b97d8d843a2d 19 (unreleased) landed
-
Update .abi-compliance-history file.
- 93fb76ca4e69 18.1 landed
-
Add .abi-compliance-history to v18 branch.
- a72f7d97bea9 18.1 landed
-
Fix thinko in commit 7d129ba54.
- 399a9e04e549 18.1 cited
-
Fix lookups in pg_{clear,restore}_{attribute,relation}_stats().
- c8af5019bee5 18.1 cited
- 688dc6299a5b 19 (unreleased) cited
-
Add defenses against unexpected changes in the NodeTag enum list.
- eea9fa9b250f 16.0 cited
-
abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T16:54:52Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T17:15:20Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Peter Geoghegan <pg@bowt.ie> — 2025-10-17T17:22:19Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T18:06:16Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T18:45:12Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T19:11:10Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Peter Geoghegan <pg@bowt.ie> — 2025-10-17T19:20:55Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T19:27:10Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T19:28:33Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T19:30:56Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Peter Geoghegan <pg@bowt.ie> — 2025-10-17T19:33:28Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T19:35:12Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T19:39:15Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-17T19:52:18Z
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 -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T19:53:09Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T20:01:53Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T20:06:10Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-17T20:11:35Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T20:19:42Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-17T21:41:27Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T21:51:44Z
"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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-17T22:51:32Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-17T23:07:36Z
"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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Mankirat Singh <mankiratsingh1315@gmail.com> — 2025-10-18T13:04:17Z
On Sat, 18 Oct 2025 at 01:22, David E. Wheeler <david@justatheory.com> wrote: > > Adding Mankirat, who developed the ABI checker for his GSoC project. Thanks! Really interesting discussion. On Sat, 18 Oct 2025 at 00:51, Peter Geoghegan <pg@bowt.ie> 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 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. The per-branch file containing the commit hash reference for that branch is a good solution and can be easily implemented since perl is really good at working with files. 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]. For example, each branch could have an `abi.suppr` file with content like this: # commit hash: 123456abc [suppress_function] name=stats_lock_check_privileges # comments for this function # commit hash: oldercommit345 [suppress_type] type_kind = struct name = some_struct_name changed_data_members = updated_member_name If needed, the contents of this file could be truncated on a new minor release maybe? Added a patch for the same for handling current abi compliance failures on baza. Regards, Mankirat [1] - https://sourceware.org/libabigail/manual/suppression-specifications.html#code-examples -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-18T15:14:49Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-18T15:46:23Z
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 -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-18T15:47:40Z
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 on other 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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Mankirat Singh <mankiratsingh1315@gmail.com> — 2025-10-18T20:48:58Z
On Sat, 18 Oct 2025 at 21:17, David E. Wheeler <david@justatheory.com> wrote: > > 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 on other platforms using other tools that may not support it suppression file format. > > Let’s do the baseline SHA file and see how it goes. Oh right, I didn't think about the other platforms. I've implemented the support for .abi-compliance-history file. If the file is present in a STABLE branch, it will be used by default; otherwise, the latest tag will be used. You can view the changes here[1]. Regards, Mankirat [1] - https://github.com/PGBuildFarm/client-code/pull/38/commits/f88873625ba0466e9609225947d40092748ff555
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-19T20:37:42Z
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 used by 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-history has been committed to the rev 18 branch (assuming the format is compatible), it should start passing again. Best, David
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T14:39:19Z
"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-history has been committed to the rev 18 branch (assuming the format is compatible), it should start passing again. 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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-20T15:36:16Z
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 -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-20T16:46:27Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T17:07:04Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-20T17:14:09Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-20T18:30:49Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T19:33:53Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Robert Haas <robertmhaas@gmail.com> — 2025-10-20T19:55:26Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-20T20:14:38Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-20T20:52:16Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-20T21:18:33Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-20T21:22:29Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-20T21:39:10Z
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 against master. Best, David -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-21T17:30:32Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-21T20:13:37Z
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 -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-21T20:21:44Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-21T20:44:19Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-21T21:44:32Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-22T14:40:50Z
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 -
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-22T16:33:20Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-22T16:36:01Z
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
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-24T17:26:20Z
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’s project, and I appreciate hackers’ support for it! Best, David
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Bruce Momjian <bruce@momjian.us> — 2025-10-29T21:24:39Z
On Fri, Oct 17, 2025 at 07:07:36PM -0400, Tom Lane wrote: > "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? I assume we would want ABI breakage checks on master between Beta 1 and the time we branch for the new major release in July. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-29T21:37:36Z
Bruce Momjian <bruce@momjian.us> writes: > On Fri, Oct 17, 2025 at 07:07:36PM -0400, Tom Lane 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". > I assume we would want ABI breakage checks on master between Beta 1 and > the time we branch for the new major release in July. In the past we've never really thought that ABI was more than mildly solidified until around rc1. On the whole I'd rather wait until after the branch before starting to check ABI, simply because I don't care for the idea of adding .abi-compliance-history in the master branch only to remove it again later. Having said that, it would be good if we *could* choose to do that, so I still do not like having any policy decisions about which branches to check hard-wired into the buildfarm client. regards, tom lane
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-29T23:13:32Z
On Oct 29, 2025, at 17:37, Tom Lane <tgl@sss.pgh.pa.us> wrote: > In the past we've never really thought that ABI was more than mildly > solidified until around rc1. On the whole I'd rather wait until after > the branch before starting to check ABI, simply because I don't care > for the idea of adding .abi-compliance-history in the master branch > only to remove it again later. Having said that, it would be good > if we *could* choose to do that, so I still do not like having any > policy decisions about which branches to check hard-wired into the > buildfarm client. Well that’s pretty easily addressed by adding a configuration for it. Maybe a regex to match branches, defaulting to its current value[0], `/_STABLE$/`. D [0]: https://github.com/MankiratSingh1315/pg-bf-client-code/blob/abi-comp-check/PGBuild/Modules/ABICompCheck.pm#L248
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-29T23:52:11Z
"David E. Wheeler" <david@justatheory.com> writes: > On Oct 29, 2025, at 17:37, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> ... I still do not like having any >> policy decisions about which branches to check hard-wired into the >> buildfarm client. > Well that’s pretty easily addressed by adding a configuration for it. Maybe a regex to match branches, defaulting to its current value[0], `/_STABLE$/`. I'm asking to remove complexity, not add more. The buildfarm client should not be checking either branch names or git tags to control this, when we have a perfectly good convention about the existence of .abi-compliance-history to control it. regards, tom lane
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-30T00:49:35Z
On Oct 29, 2025, at 19:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm asking to remove complexity, not add more. The buildfarm client > should not be checking either branch names or git tags to control > this, when we have a perfectly good convention about the existence > of .abi-compliance-history to control it. We can remove the branch check, of course, but then you would have to maintain .abi-compliance-history in master, no? D
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-30T02:49:24Z
"David E. Wheeler" <david@justatheory.com> writes: > On Oct 29, 2025, at 19:52, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I'm asking to remove complexity, not add more. The buildfarm client >> should not be checking either branch names or git tags to control >> this, when we have a perfectly good convention about the existence >> of .abi-compliance-history to control it. > We can remove the branch check, of course, but then you would have to maintain .abi-compliance-history in master, no? No; my proposal was "don't run the ABI check unless the branch has a .abi-compliance-history file". master would normally not contain such a file, thus no check. As I was just discussing with Bruce, we could put one there for awhile if we wanted to run ABI checks in advance of forking a stable branch. The reason I'm so allergic to having any of these decisions made on the buildfarm-client side is years of herding cats^W^W trying to get buildfarm owners to update their script versions and/or config files. It's close to hopeless. Thus, your proposal a message or three back to add another BF client config setting to control this sounds like the worst of all possible worlds. If we needed a change in the setting, getting the farm to converge to that would take months if not years. The idea that we could transiently enable checks between beta1 and branch fork on the basis of that approach is downright risible. On the other hand, if the decisions are driven purely by what is in our git tree, a change is the work of moments. regards, tom lane
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-30T12:41:57Z
On Oct 29, 2025, at 22:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> We can remove the branch check, of course, but then you would have to maintain .abi-compliance-history in master, no? > > No; my proposal was "don't run the ABI check unless the branch has > a .abi-compliance-history file". master would normally not contain > such a file, thus no check. As I was just discussing with Bruce, > we could put one there for awhile if we wanted to run ABI checks in > advance of forking a stable branch. Oh, I see, that’s certainly do-able. > The reason I'm so allergic to having any of these decisions made on > the buildfarm-client side is years of herding cats^W^W trying to get > buildfarm owners to update their script versions and/or config files. > It's close to hopeless. Thus, your proposal a message or three back > to add another BF client config setting to control this sounds like > the worst of all possible worlds. If we needed a change in the > setting, getting the farm to converge to that would take months if not > years. The idea that we could transiently enable checks between beta1 > and branch fork on the basis of that approach is downright risible. > On the other hand, if the decisions are driven purely by what is in > our git tree, a change is the work of moments. That makes excellent sense, I appreciate the context. Might I suggest, however, that it also run for branches matching /_STABLE$/ even if there is no .abi-hstory-file? The whole point of this exercise was to increase the confidence in ABI stability in stable branches. Running it in such branches would help remind maintainers to add the file. OTOH, if someone ran it for really old branches it could bs super annoying, so maybe not… D
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-30T13:55:42Z
"David E. Wheeler" <david@justatheory.com> writes: > Might I suggest, however, that it also run for branches matching /_STABLE$/ even if there is no .abi-hstory-file? The whole point of this exercise was to increase the confidence in ABI stability in stable branches. Running it in such branches would help remind maintainers to add the file. Trouble is, you then need an arbitrary client-made choice about which commit to run the ABI check against. If that code does something we realize we don't want, we're back up against the problem of moving the buildfarm configuration to fix it. I'd rather the decision be opt-in. (Also, the only rules I heard proposed for such client-driven choices involved git tags. I already explained why I don't want that: git tags are hard to modify and subject to too many other constraints.) regards, tom lane
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-30T14:10:43Z
On Oct 30, 2025, at 09:55, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Trouble is, you then need an arbitrary client-made choice about which > commit to run the ABI check against. It’s currently coded to use the most recent tag or, if there is none in the branch, the branch root. > If that code does something we > realize we don't want, we're back up against the problem of moving the > buildfarm configuration to fix it. I'd rather the decision be opt-in. Fair. Just means that if no one adds a history file to a branch that branch will never be tested and there’s no automated way to realize it. > (Also, the only rules I heard proposed for such client-driven choices > involved git tags. I already explained why I don't want that: git > tags are hard to modify and subject to too many other constraints.) Yeah, it just went with the most recent tag to keep it simple, no other metadata. D
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Mankirat Singh <mankiratsingh1315@gmail.com> — 2025-10-30T15:22:08Z
On Thu, 30 Oct 2025 at 19:40, David E. Wheeler <david@justatheory.com> wrote: > > On Oct 30, 2025, at 09:55, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > Trouble is, you then need an arbitrary client-made choice about which > > commit to run the ABI check against. > > It’s currently coded to use the most recent tag or, if there is none in the branch, the branch root. Yes, like before the addition of .abi-compliance-history in the REL_18_STABLE branch, REL_18_0 was being used as the baseline. > > If that code does something we > > realize we don't want, we're back up against the problem of moving the > > buildfarm configuration to fix it. I'd rather the decision be opt-in. No changes to individual animal configurations will be required. Once a STABLE branch gets the .abi-compliance-history file, the baseline will update automatically from the lastest tag to the mentioned commit SHA for all clients. :D > Fair. Just means that if no one adds a history file to a branch that branch will never be tested and there’s no automated way to realize it. Although I don’t oppose the idea of “don’t run the ABI check unless the branch has a .abi-compliance-history file”, it would just need some minor code removals and adjustments. We can put a note in the compliance check result for new STABLE branches - "no .abi-compliance-history file found", but keep the client status green?
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-30T15:30:44Z
On Wed, Oct 29, 2025 at 10:49:24PM -0400, Tom Lane wrote: > No; my proposal was "don't run the ABI check unless the branch has > a .abi-compliance-history file". master would normally not contain > such a file, thus no check. As I was just discussing with Bruce, > we could put one there for awhile if we wanted to run ABI checks in > advance of forking a stable branch. > > The reason I'm so allergic to having any of these decisions made on > the buildfarm-client side is years of herding cats^W^W trying to get > buildfarm owners to update their script versions and/or config files. > It's close to hopeless. Thus, your proposal a message or three back > to add another BF client config setting to control this sounds like > the worst of all possible worlds. If we needed a change in the > setting, getting the farm to converge to that would take months if not > years. The idea that we could transiently enable checks between beta1 > and branch fork on the basis of that approach is downright risible. > On the other hand, if the decisions are driven purely by what is in > our git tree, a change is the work of moments. I agree with Tom. The lack of an .abi-compliance-history file should be taken to mean that we're not interested in maintaining ABI compatibility across commits for that branch, and the buildfarm check should be skipped. -- nathan
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Bruce Momjian <bruce@momjian.us> — 2025-10-30T16:49:23Z
On Wed, Oct 29, 2025 at 05:37:36PM -0400, Tom Lane wrote: > Bruce Momjian <bruce@momjian.us> writes: > > On Fri, Oct 17, 2025 at 07:07:36PM -0400, Tom Lane 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". > > > I assume we would want ABI breakage checks on master between Beta 1 and > > the time we branch for the new major release in July. > > In the past we've never really thought that ABI was more than mildly > solidified until around rc1. On the whole I'd rather wait until after > the branch before starting to check ABI, simply because I don't care > for the idea of adding .abi-compliance-history in the master branch > only to remove it again later. Having said that, it would be good > if we *could* choose to do that, so I still do not like having any > policy decisions about which branches to check hard-wired into the > buildfarm client. I guess I would like to be _notified_, in some way, of ABI breaks during that period. -- Bruce Momjian <bruce@momjian.us> https://momjian.us EDB https://enterprisedb.com Do not let urgent matters crowd out time for investment in the future.
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-30T17:03:23Z
On Oct 30, 2025, at 11:22, Mankirat Singh <mankiratsingh1315@gmail.com> wrote: > We can put a note in the compliance check result for new STABLE > branches - "no .abi-compliance-history file found", but keep the > client status green? Yes, I think that’s the thing to do. D
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Robert Haas <robertmhaas@gmail.com> — 2025-10-30T20:10:03Z
On Thu, Oct 30, 2025 at 8:42 AM David E. Wheeler <david@justatheory.com> wrote: > Might I suggest, however, that it also run for branches matching /_STABLE$/ even if there is no .abi-hstory-file? The whole point of this exercise was to increase the confidence in ABI stability in stable branches. Running it in such branches would help remind maintainers to add the file. I think it is better to do as Tom suggests -- i.e. have a buildfarm client that only cares about the presence of the file, and not at all about the name of the branch. That gives us the maximum amount of future flexibility for no real cost. Said differently, if somebody nukes the ABI history file from a back-branch, I think we should assume that represents a deliberate decision on the part of the project to stop caring about ABI compatibility in that particular back-branch, not an error that the build-farm needs to flag. Of course, if somebody wants to argue that such a decision was wrongly made, they're free to do so on pgsql-hackers, but we'd like the buildfarm to be green rather than red while that's being litigated. -- Robert Haas EDB: http://www.enterprisedb.com
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Peter Eisentraut <peter@eisentraut.org> — 2025-10-31T11:02:10Z
On 21.10.25 22:13, Tom Lane wrote: > 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.) What is the reason that this file is supposed to contain the history of relevant changes, rather than just the last one? If you want the history, you could look at the git log of the file itself, no?
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-10-31T13:57:14Z
On Oct 30, 2025, at 16:10, Robert Haas <robertmhaas@gmail.com> wrote: > Said differently, if somebody > nukes the ABI history file from a back-branch, I think we should > assume that represents a deliberate decision on the part of the > project to stop caring about ABI compatibility in that particular > back-branch, not an error that the build-farm needs to flag. I was thinking less of old branches than brand new ones. I’m on board with requiring an ABI history file, and that it’s part of the branch for release process to create one should minimize the chances that someone forgets long enough for an ABI change to go uncaught and released. Best, David
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Nathan Bossart <nathandbossart@gmail.com> — 2025-10-31T14:09:36Z
On Fri, Oct 31, 2025 at 12:02:10PM +0100, Peter Eisentraut wrote: > What is the reason that this file is supposed to contain the history of > relevant changes, rather than just the last one? > > If you want the history, you could look at the git log of the file itself, > no? I think either way would ultimately be fine. I might argue that keeping the full history in a file with detailed explanations is more accessible than requiring folks to run git log .abi-compliance-history (Plus, if someone doesn't bother to put details in the commit message, you then have to sleuth further to figure out what changed.) -- nathan
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
Tom Lane <tgl@sss.pgh.pa.us> — 2025-10-31T15:17:11Z
Nathan Bossart <nathandbossart@gmail.com> writes: > On Fri, Oct 31, 2025 at 12:02:10PM +0100, Peter Eisentraut wrote: >> What is the reason that this file is supposed to contain the history of >> relevant changes, rather than just the last one? >> If you want the history, you could look at the git log of the file itself, >> no? > I think either way would ultimately be fine. I might argue that keeping > the full history in a file with detailed explanations is more accessible > than requiring folks to run > git log .abi-compliance-history > (Plus, if someone doesn't bother to put details in the commit message, you > then have to sleuth further to figure out what changed.) Yeah, that last. I would expect the annotation text in .abi-compliance-history to be specific about the nature of the ABI break, whereas the log message for the commit that changed things might not bother with such details. As we move along with this effort, we might ultimately decide that this scheme is overdoing the amount of detail recorded. But to start with, I'd rather err on the side of recording more info not less. regards, tom lane
-
Re: abi-compliance-check failure due to recent changes to pg_{clear,restore}_{attribute,relation}_stats()
David E. Wheeler <david@justatheory.com> — 2025-11-01T15:49:50Z
On Oct 30, 2025, at 13:03, David E. Wheeler <david@justatheory.com> wrote: > On Oct 30, 2025, at 11:22, Mankirat Singh <mankiratsingh1315@gmail.com> wrote: > >> We can put a note in the compliance check result for new STABLE >> branches - "no .abi-compliance-history file found", but keep the >> client status green? > > Yes, I think that’s the thing to do. FYI, Mankirat made this change[0] and I’ve deployed it to baza. It will no only run for branches that contain the history file unless the animal owner configures specific baselines in the configuration file (mainly intended for testing). Best, David