Обсуждение: ABI Compliance Checker GSoC Project

Hackers,

I’d like to introduce Mankirat Singh, a Google Summer of Code student that Pavlo Golub and I are mentoring this year.
He’sstarted work on his project, an ABI Compliance Checker. The plan is to work out the patterns, integrate it into the
BuildFarm, and get it sending regular reports by early September. He’s building on the foundation laid by Peter
Geogheganback in 2023 [1]. He’s written up his plan in his blog [2]. 

Since the work naturally gets into what’s considered a public API and what’s not, we feel that hackers is the best
placeto ask questions about bits to include and exclude, as well as other questions related to configuration settings,
etc.,so please watch for those. I hope you all will help Mankirat to quickly learn what’s what, figure out how it goes
together,and make it a reality! 

In the meantime, please give him a warm welcome!

Best,

David

PS: If one of you fine people can grant permission for Mankirat’s blog to be added to Planet Postgres, we’d super
appreciateit. 

[1]: https://postgr.es/m/CAH2-Wzm-W6hSn71sUkz0Rem=qDEU7TnFmc7_jG2DjrLFef_WKQ@mail.gmail.com
[2]: https://blog.mankiratsingh.com/

On 2025-Jun-03, Mankirat Singh wrote:

> 'struct ReadStream at read_stream.c:109:1' changed:
>   type size hasn't changed
>   1 data member insertion:
>     'int16 io_combine_limit', at offset 2 (in bytes) at read_stream.c:112:1
>   there are data member changes:
>     'int16 ios_in_progress' offset changed from 2 to 4 (in bytes) (by +2
> bytes)
>     'int16 queue_size' offset changed from 4 to 6 (in bytes) (by +2 bytes)
>     'int16 max_pinned_buffers' offset changed from 6 to 8 (in bytes) (by +2
> bytes)
>     'int16 pinned_buffers' offset changed from 8 to 10 (in bytes) (by +2
> bytes)
>     'int16 distance' offset changed from 10 to 12 (in bytes) (by +2 bytes)
>     'bool advice_enabled' offset changed from 12 to 14 (in bytes) (by +2
> bytes)

Well, this is an ABI difference all right, and I don't think it's the
job of the tool to determine that this ABI difference is okay.
Ultimately that's for a human to determine, and they must either add an
suppression to the Postgres source code somehow, or modify or revert the
commit so that the ABI change disappears.

I'm surprised to hear that libabigail has no way for us to declare that
an ABI diff should be ignored.  Suppressions were part of the original
plan, and I fear that if they aren't possible, then this whole thing may
not work for us.

> 'struct WalSndCtlData at walsender_private.h:91:1' changed:
>   type size hasn't changed
>   there are data member changes:
>     name of 'WalSndCtlData::sync_standbys_defined' changed to
> 'WalSndCtlData::sync_standbys_status' at walsender_private.h:110:1

Also a diff worth reporting, and suppressing from the report as
appropriate.

The only kinds of ABI changes that should be silenced by default are

1) addition of struct members that cause no change to the offsets of other
   members in the struct (i.e. a new member that uses space that was
   previously padding space)

2) addition of struct members at the end of structs, changing the struct
   size, but only for structs that aren't used as array elements (the
   problem being that the size of the "stride" when scanning the array
   would mismatch).  Not sure how easy it is to detect this case.

I'm also wondering what platforms/architectures are you thinking on
running this on.  For instance, thinking about padding space in structs,
what is padding space in x86_64 may not be so in 32bit x86 ...


> I don't have much idea about the PostgreSQL internal code, but I really
> wanted to know how we can "classify" what exactly is a false positive for
> this report and should not be reported by the final ABI compliance checking
> tool?

I think in a first cut of this tooling, we should consider all ABI
changes as potentially problematic.

> I checked the suppression file for libabigail tools, but it seems not to
> work for the PostgreSQL case, as we need to specify each and every symbol
> to be ignored in the file, which is kinda not possible.

Please elaborate.  Can you not write a suppression file that says
"ignore offset changes for ios_in_progress in ReadStream", for example?

> And these symbols to be suppressed don't follow any common Regex as
> well, and the suppression file doesn't support including whole files
> or folders.

Ummm, are you saying that it complains about changes to unexported
symbols also?

> Secondly, we can't use the -fvisibility=hidden flag while compiling
> either, as it results in a failed compilation with an error.

I didn't understand what you meant here.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"All rings of power are equal,
But some rings of power are more equal than others."
                                 (George Orwell's The Lord of the Rings)

On Tue, 3 Jun 2025 at 20:49, Álvaro Herrera <alvherre@kurilemu.de> wrote:
>
> I don't think it's the
> job of the tool to determine that this ABI difference is okay.
> Ultimately that's for a human to determine,

 Yes, but it would be better if we could automate that thing to some
extent, along with the development of the ABI compliance checker.

> and they must either add an
> suppression to the Postgres source code somehow, or modify or revert the
> commit so that the ABI change disappears.
That's exactly what I aim for with this project, by just automatically
run the comparing the latest commit with the previous using abidiff
and if some ABI instability found, then immediately report it to the
commiter or some mailing list so that maintainers don't have to do
that manually everytime - That's the reason for asking about false
positives and how to suppress them.


> Also a diff worth reporting, and suppressing from the report as
> appropriate.
Okay - It is a change in name, I assume that's the reason it's important.


> The only kinds of ABI changes that should be silenced by default are
>
> 1) addition of struct members that cause no change to the offsets of other
>    members in the struct (i.e. a new member that uses space that was
>    previously padding space)
>
> 2) addition of struct members at the end of structs, changing the struct
>    size, but only for structs that aren't used as array elements (the
>    problem being that the size of the "stride" when scanning the array
>    would mismatch).  Not sure how easy it is to detect this case.

These sound very similar to 17.1 minor release ABI instability.
abidiff seems to do most of the work in this case:

$ abidiff ./install/with_debug/REL_17_0/bin/postgres
./install/with_debug/REL_17_1/bin/postgres --leaf-changes-only
--no-added-syms --show-bytes
Leaf changes summary: 1 artifact changed
Changed leaf types summary: 1 leaf type changed
Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added
function (11 filtered out)
Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable

'struct ResultRelInfo at execnodes.h:450:1' changed:
  type size changed from 376 to 384 (in bytes)
  1 data member insertion:
    'bool ri_needLockTagTuple', at offset 376 (in bytes) at execnodes.h:597:1


> I'm also wondering what platforms/architectures are you thinking on
> running this on.  For instance, thinking about padding space in structs,
> what is padding space in x86_64 may not be so in 32bit x86 ...

The reporting system is planned to be developed as an extension to the
postgresql build farm, so the abidiff reports across architectures
from various animals could be received, processed and reported
accordingly.


> I think in a first cut of this tooling, we should consider all ABI
> changes as potentially problematic.

Certainly, will do that only until I understand how to identify and
implement some techniques for the removal of false positives.


> Please elaborate.  Can you not write a suppression file that says
> "ignore offset changes for ios_in_progress in ReadStream", for example?

 I can do that, and that's what's causing the problem. According to
the documentation for these suppression files[1], we have to mention
the particular symbol name we need to suppress like "ReadStream" or
something particular like "ignore offset changes for ios_in_progress
in ReadStream between member1 and member 2" which is humanly very hard
to do as for sure there will be 100s of symbols in postgres like this
which needs to be ignored in that case.


> > And these symbols to be suppressed don't follow any common Regex as
> > well, and the suppression file doesn't support including whole files
> > or folders.
> Ummm, are you saying that it complains about changes to unexported
> symbols also?
>
>
> > Secondly, we can't use the -fvisibility=hidden flag while compiling
> > either, as it results in a failed compilation with an error.
>
> I didn't understand what you meant here.

This led me to another doubt, which might make things clear, that is,
any symbol which abidiff gives in output is important to report?
because my initial thought was that symbols exported in the binary
created when we build postgres also contain some symbols which need to
be suppressed because they are some postgres internal functions[2] -
is that true?
If it's not true, then things seem to be much sorted than before.

Note - The -fvisibility=hidden flag I mentioned was on a similar note
that when we compile postgres with this flag, it should generate
postgres binary by removing internal symbols, but instead it gives a
compilation error.

Regards,
Mankirat

[1] - https://sourceware.org/libabigail/manual/suppression-specifications.html#suppr-spec-label
[2] - https://www.postgresql.org/message-id/CAH2-Wzm-W6hSn71sUkz0Rem%3DqDEU7TnFmc7_jG2DjrLFef_WKQ%40mail.gmail.com

On 2025-Jun-03, Mankirat Singh wrote:

> On Tue, 3 Jun 2025 at 20:49, Álvaro Herrera <alvherre@kurilemu.de> wrote:

> > Please elaborate.  Can you not write a suppression file that says
> > "ignore offset changes for ios_in_progress in ReadStream", for example?
> 
> I can do that, and that's what's causing the problem. According to
> the documentation for these suppression files[1], we have to mention
> the particular symbol name we need to suppress like "ReadStream" or
> something particular like "ignore offset changes for ios_in_progress
> in ReadStream between member1 and member 2" which is humanly very hard
> to do as for sure there will be 100s of symbols in postgres like this
> which needs to be ignored in that case.

Well, now that I grep the source for ReadStream, I realize that the
struct is defined in a .c file, not in any .h files, so you're right
that the tooling needs a way to understand that changes to this symbol
must not raise any alarms; and that way must not involve a manually
written suppression file.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Officer Krupke, what are we to do?
Gee, officer Krupke, Krup you! (West Side Story, "Gee, Officer Krupke")

On 2025-Jun-04, Mankirat Singh wrote:

> On Tue, 3 Jun 2025 at 23:50, David E. Wheeler <david@justatheory.com> wrote:
> > >> Ummm, are you saying that it complains about changes to unexported
> > >> symbols also?
> >
> > This is a good question.
> No, it doesn’t complain about unexported symbols.

You mentioned ReadStream, but that's not exported.

> > What’s the error? Maybe we can fix it.
>
> As per my knowledge Postgres internal code lacks visibility annotations on
> its symbols, which causes compilation errors when fvisibility flag is used.

You're being way too vague with your responses.  Please copy & paste
command lines used and the error messages you get.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Las mujeres son como hondas:  mientras más resistencia tienen,
 más lejos puedes llegar con ellas"  (Jonas Nightingale, Leap of Faith)

On Jun 4, 2025, at 09:43, Álvaro Herrera <alvherre@kurilemu.de> wrote:

> You mentioned ReadStream, but that's not exported.

I this not an export at line 67?

```
❯ rg ReadStream src/include/storage/read_stream.h

50: * the ReadStreamBlockNumberCB callback to abide by the restrictions of AIO
66:struct ReadStream;
67:typedef struct ReadStream ReadStream;
70:typedef struct BlockRangeReadStreamPrivate
74:} BlockRangeReadStreamPrivate;
77:typedef BlockNumber (*ReadStreamBlockNumberCB) (ReadStream *stream,
81:extern BlockNumber block_range_read_stream_cb(ReadStream *stream,
84:extern ReadStream *read_stream_begin_relation(int flags,
88:   ReadStreamBlockNumberCB callback,
91:extern Buffer read_stream_next_buffer(ReadStream *stream, void **per_buffer_data);
92:extern BlockNumber read_stream_next_block(ReadStream *stream,
94:extern ReadStream *read_stream_begin_smgr_relation(int flags,
99:   ReadStreamBlockNumberCB callback,
102:extern void read_stream_reset(ReadStream *stream);
103:extern void read_stream_end(ReadStream *stream);
```

Best,

David

On Wed, 4 Jun 2025 at 19:13, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> > On Tue, 3 Jun 2025 at 23:50, David E. Wheeler <david@justatheory.com> wrote:
> > > What’s the error? Maybe we can fix it.
> >
> > As per my knowledge Postgres internal code lacks visibility annotations on
> > its symbols, which causes compilation errors when fvisibility flag is used.
>
> You're being way too vague with your responses.  Please copy & paste
> command lines used and the error messages you get.
Really sorry for that.

Here's the workflow I tried to compile
$ ./configure CFLAGS="-Og -g -fvisibility=hidden"
--prefix=/home/mankirat/install/REL_17_4
$ make -j$(nproc)
........
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1154:
undefined reference to `PQserverVersion'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1156:
undefined reference to `appendPQExpBufferChar'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1155:
undefined reference to `appendPQExpBufferStr'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1164:
undefined reference to `appendPQExpBufferStr'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1165:
undefined reference to `appendPQExpBuffer'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1169:
undefined reference to `termPQExpBuffer'
/usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1170:
undefined reference to `termPQExpBuffer'
collect2: error: ld returned 1 exit status
make[3]: *** [Makefile:51: pg_restore] Error 1
make[3]: Leaving directory
'/home/mankirat/Desktop/OSS/abicc/try2/postgres/src/bin/pg_dump'
make[2]: *** [Makefile:45: all-pg_dump-recurse] Error 2
make[2]: Leaving directory
'/home/mankirat/Desktop/OSS/abicc/try2/postgres/src/bin'
make[1]: *** [Makefile:42: all-bin-recurse] Error 2
make[1]: Leaving directory '/home/mankirat/Desktop/OSS/abicc/try2/postgres/src'
make: *** [GNUmakefile:11: all-src-recurse] Error 2

$ make install
.........
/usr/bin/install: cannot stat './dynloader.h': No such file or directory
make[2]: *** [Makefile:50: install] Error 1
make[2]: Leaving directory '/home/mankirat/postgres/src/include'
make[1]: *** [Makefile:42: install-include-recurse] Error 2
make[1]: Leaving directory '/home/mankirat/postgres/src'
make: *** [GNUmakefile:11: install-src-recurse] Error 2


I get this error when I try using the -fvisibilty=hidden flag

Regards,
Mankirat

Hi,

On 2025-06-04 11:15:10 -0400, David E. Wheeler wrote:
> On Jun 4, 2025, at 09:43, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> 
> > You mentioned ReadStream, but that's not exported.
> 
> I this not an export at line 67?
> 
> ```
> ❯ rg ReadStream src/include/storage/read_stream.h
> 
> 50: * the ReadStreamBlockNumberCB callback to abide by the restrictions of AIO
> 66:struct ReadStream;
> 67:typedef struct ReadStream ReadStream;

No. It just makes the *name* of the struct visible. The type's definition is
in the .c file and therefore not visible outside of read_stream.c.

Greetings,

Andres Freund

On Jun 4, 2025, at 12:10, Andres Freund <andres@anarazel.de> wrote:

> No. It just makes the *name* of the struct visible. The type's definition is
> in the .c file and therefore not visible outside of read_stream.c.

Right, got it, thanks.

David

On 2025-Jun-04, Mankirat Singh wrote:

> Here's the workflow I tried to compile
> $ ./configure CFLAGS="-Og -g -fvisibility=hidden"
> --prefix=/home/mankirat/install/REL_17_4
> $ make -j$(nproc)
> ........
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1154:
> undefined reference to `PQserverVersion'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1156:
> undefined reference to `appendPQExpBufferChar'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1155:
> undefined reference to `appendPQExpBufferStr'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1164:
> undefined reference to `appendPQExpBufferStr'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1165:
> undefined reference to `appendPQExpBuffer'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1169:
> undefined reference to `termPQExpBuffer'
> /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1170:
> undefined reference to `termPQExpBuffer'

Ah yeah, that doesn't work.  What confused me is that we do use
-fvisibility=hidden in our builds already, just not in this way; what we
do is put it in the CFLAGS specifically for "modules".  By adding it to
the overall CFLAGS I think you're breaking things for the linker
somehow, though I don't understand exactly how or why.  Anyway, it
doesn't look to me like adding -fvisibility=hidden to CFLAGS is a
viable solution, though maybe it is possible to get the build to play
nice.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Syntax error: function hell() needs an argument.
Please choose what hell you want to involve.

Hello Mankirat,

Were you able to make any progress on this?

Thanks,

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Porque francamente, si para saber manejarse a uno mismo hubiera que
rendir examen... ¿Quién es el machito que tendría carnet?"  (Mafalda)

Hi,

On 2025-06-05 19:05:28 +0200, Álvaro Herrera wrote:
> On 2025-Jun-04, Mankirat Singh wrote:
>
> > Here's the workflow I tried to compile
> > $ ./configure CFLAGS="-Og -g -fvisibility=hidden"
> > --prefix=/home/mankirat/install/REL_17_4
> > $ make -j$(nproc)
> > ........
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1154:
> > undefined reference to `PQserverVersion'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1156:
> > undefined reference to `appendPQExpBufferChar'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1155:
> > undefined reference to `appendPQExpBufferStr'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1164:
> > undefined reference to `appendPQExpBufferStr'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1165:
> > undefined reference to `appendPQExpBuffer'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1169:
> > undefined reference to `termPQExpBuffer'
> > /usr/bin/ld: /home/mankirat/postgres/src/fe_utils/string_utils.c:1170:
> > undefined reference to `termPQExpBuffer'
>
> Ah yeah, that doesn't work.  What confused me is that we do use
> -fvisibility=hidden in our builds already, just not in this way; what we
> do is put it in the CFLAGS specifically for "modules".  By adding it to
> the overall CFLAGS I think you're breaking things for the linker
> somehow, though I don't understand exactly how or why.

Two mechanisms, I think:

For some libraries, like libpq, we rely on export files for symbol
visibility. Just adding -fvisibility=hidden will remove the symbols from
visibility before the linker gets to them, basically making libpq not export
any symbols.

Extensions rely on linking against a vast number of symbols in the main
binary. Just adding -fvisibility=hidden will hide those symbols. I think it's
somewhat platform defined how -fvisibility=hidden will interact with
-Wl,--export-dynamic.


For "modules" it doesn't cause problems because we only rely on a small set of
functions to be visible on postgres, namely _PG_init(), the module magic and
functions exported with PG_FUNCTION_INFO_V1(). For those we all have added
explicit PGDLLEXPORT annotations, which "counteracts" the -fvisibility=hidden.

This is beneficial for extensions because on ELF platforms, all exported
platforms in shared libraries can be intercepted by the main executable,
requiring all function calls between exported functions within an extension to
go through the GOT, slowing things down noticeably.


> Anyway, it doesn't look to me like adding -fvisibility=hidden to CFLAGS is a
> viable solution, though maybe it is possible to get the build to play nice.

It'd be nice if we could get there, but it'd require annotating *all*
intentionally exported functions in the backend with PGDLLIMPORT (rather than
just doing that for variables).  Then we could make some symbols
*intentionally* not exported, which can improve the code generation (allowing
more compiler and linker optimizations).

Greetings,

Andres Freund

On Jul 9, 2025, at 11:33, Andres Freund <andres@anarazel.de> wrote:

> It'd be nice if we could get there, but it'd require annotating *all*
> intentionally exported functions in the backend with PGDLLIMPORT (rather than
> just doing that for variables).  Then we could make some symbols
> *intentionally* not exported, which can improve the code generation (allowing
> more compiler and linker optimizations).

How much effort would that require? Is it simply annotating all such exported functions? Or is the challenge
identifyingthem? 

Best,

David

Hello Hackers,
I have been working on this project with David since last month.
I've written some blogs about the progress, the most recent one is here [1]
Here's the draft pull request for anyone interested in reviewing the code: [2]

On Wed, 9 Jul 2025 at 17:57, Álvaro Herrera <alvherre@kurilemu.de> wrote:
> Were you able to make any progress on this?
For now, my goal was to make it work without handling false positives.
I plan to address those later, but I want to understand them now so I
can move faster when the time comes.

On Wed, 9 Jul 2025 at 21:03, Andres Freund <andres@anarazel.de> wrote:
> This is beneficial for extensions because on ELF platforms, all exported
> platforms in shared libraries can be intercepted by the main executable,
> requiring all function calls between exported functions within an extension to
> go through the GOT, slowing things down noticeably.
>
>
> > Anyway, it doesn't look to me like adding -fvisibility=hidden to CFLAGS is a
> > viable solution, though maybe it is possible to get the build to play nice.
>
> It'd be nice if we could get there, but it'd require annotating *all*
> intentionally exported functions in the backend with PGDLLIMPORT (rather than
> just doing that for variables).  Then we could make some symbols
> *intentionally* not exported, which can improve the code generation (allowing
> more compiler and linker optimizations).
Thanks for the explanation, Andres!
Had the same questions as David asked.

Regards,
Mankirat

[1] - https://blog.mankiratsingh.com/posts/worklog-3to11-abi-complicance-reporting/
[2] - https://github.com/PGBuildFarm/client-code/pull/38

On Jul 12, 2025, at 09:03, Mankirat Singh <mankiratsingh1315@gmail.com> wrote:

> I've written some blogs about the progress, the most recent one is here [1]

For those who didn’t click through, that post has a screenshot of an ABI diff report in the build farm server that I
thinkis worth seeing: 



Mankirat and I have been discussing whether the project needs to diff every commit since the last time it was run (or
fromthe root commit of the branch) or just the most recent. I’m leaning toward the latter, as it’s more like how other
stuffin the build farm client works. But it also seems useful to report on individual commits (as in this one committed
byTom). Thoughts? 

Best,

David

"David E. Wheeler" <david@justatheory.com> writes:
> For those who didn’t click through, that post has a screenshot of an
> ABI diff report in the build farm server that I think is worth
> seeing:

Nitpick: I think something is backwards about the labeling.  AFAICS
the described ABI change was made by 53cd0b71e not its predecessor
9dcc76414.  It does look like a useful bit of information once
correctly attributed, though.

> Mankirat and I have been discussing whether the project needs to
> diff every commit since the last time it was run (or from the root
> commit of the branch) or just the most recent. I’m leaning toward
> the latter, as it’s more like how other stuff in the build farm
> client works. But it also seems useful to report on individual
> commits (as in this one committed by Tom). Thoughts?

What we are going to want to know is if there are any ABI breaks in a
stable branch since the last release point.  Once something turns up
in that view, it'd be good to be able to drill down to exactly which
commit(s) caused the changes.  But nobody is going to want to leaf
through dozens or hundreds of per-commit reports, most of which should
be totally uninteresting for this purpose (in a back branch anyway).
Also, assuming we take action to undo the break, the cancelling-out
commits are no longer interesting, but we want to ensure that indeed
the ABI break is gone.  So in my mind, ABI diff between last release
and branch tip is going to be the normal thing to want to see.

            regards, tom lane

On Sun, 13 Jul 2025 at 05:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Nitpick: I think something is backwards about the labeling.  AFAICS
> the described ABI change was made by 53cd0b71e not its predecessor
> 9dcc76414.  It does look like a useful bit of information once
> correctly attributed, though.
Thanks for pointing that out, I'll fix it!

> What we are going to want to know is if there are any ABI breaks in a
> stable branch since the last release point.  Once something turns up
> in that view, it'd be good to be able to drill down to exactly which
> commit(s) caused the changes.  But nobody is going to want to leaf
> through dozens or hundreds of per-commit reports, most of which should
> be totally uninteresting for this purpose (in a back branch anyway).
> Also, assuming we take action to undo the break, the cancelling-out
> commits are no longer interesting, but we want to ensure that indeed
> the ABI break is gone.  So in my mind, ABI diff between last release
> and branch tip is going to be the normal thing to want to see.
Got your point.
Here, I had a proposal: In case an ABI break is found, then loop
through the commits after the last run to find the offending commit.
We can also give the animal owner the option to decide whether they
want to use their own machine to perform this search or not. Let me
know your thoughts on this.

Regards,
Mankirat

On Jul 13, 2025, at 14:34, Mankirat Singh <mankiratsingh1315@gmail.com> wrote:

> Here, I had a proposal: In case an ABI break is found, then loop
> through the commits after the last run to find the offending commit.
> We can also give the animal owner the option to decide whether they
> want to use their own machine to perform this search or not. Let me
> know your thoughts on this.

Presumably you could work your way backwards until the ABI break disappears. That should minimize the number of commits
youhave to compile and test. 

Best,

David

On Mon, 14 Jul 2025 at 00:07, David E. Wheeler <david@justatheory.com> wrote:
> Presumably you could work your way backwards until the ABI break disappears. That should minimize the number of
commitsyou have to compile and test.
 
Makes sense. Thanks for the suggestion!

Regards,
Mankirat

Mankirat Singh <mankiratsingh1315@gmail.com> writes:
> Here, I had a proposal: In case an ABI break is found, then loop
> through the commits after the last run to find the offending commit.
> We can also give the animal owner the option to decide whether they
> want to use their own machine to perform this search or not. Let me
> know your thoughts on this.

On reflection, assuming that this is being run by one or more
buildfarm animals, there are not likely to be so many commits
between runs that it'll be hard to assign blame after a breakage.
So I'm not convinced that we need to build the logic you describe.

What we do need to think about perhaps is how we identify
the reference commit to compare ABI with.  In stable branches
it might be sufficient to say "the reference is the last
tagged commit in this branch", since we only tag known-good
release points.  I'm less sure that that rule works for a
branch that's currently in beta, since it's quite likely that
there will be ABI breaks that we are willing to accept.
(I guess that that scenario could happen for stable branches
too, though rarely.)  If that happens, it'd be nice to be able
to silence the complaints sooner than the next release point.

One way to solve that without additional mechanism is to say
"a tag is a tag; if you want to deem some commit good just
tag it with a tag that doesn't look like a release tag".
This feels a little icky somehow but I guess it could work.
We'd probably want some documented convention for how to
name such non-release-point ABI-blessing tags.

Alternatively, does git have any kind of metadata that isn't
a tag but acts sort of like one?  The fact that you cannot
move or remove a tag once pushed could come back to bite us
if we start using them for this purpose.

Another idea could be an in-tree file, different in each branch,
that records the hash of the commit we presently want to compare to.
This would require a small amount of additional manual effort to
maintain, but maybe it's the most flexible way.  Notably, we could
say that if the file isn't present in a given branch then we don't
want any ABI checking, whereas with the tag approach I think we'd
need some hard-coded rule about which branches to ignore.

Sorry about the thinking-out-loud character of this message.
Of the ideas I've thought of, the in-tree file is appealing
to me the most, but maybe somebody else has a better idea.

            regards, tom lane

On Jul 13, 2025, at 15:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> On reflection, assuming that this is being run by one or more
> buildfarm animals, there are not likely to be so many commits
> between runs that it'll be hard to assign blame after a breakage.
> So I'm not convinced that we need to build the logic you describe.

Fair. Better to start simple and add something like this later if it’s a PITA without it.

> Alternatively, does git have any kind of metadata that isn't
> a tag but acts sort of like one?  The fact that you cannot
> move or remove a tag once pushed could come back to bite us
> if we start using them for this purpose.

Notes?

  https://git-scm.com/docs/git-notes

Best,

David

"David E. Wheeler" <david@justatheory.com> writes:
> On Jul 13, 2025, at 15:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alternatively, does git have any kind of metadata that isn't
>> a tag but acts sort of like one?

> Notes?
>   https://git-scm.com/docs/git-notes

Hmm, maybe.  I recall discussion about starting to use notes
to amend commit messages, and I'm not sure if this usage would
conflict with that.  At the very least we'd need to write a
specification for what a note that triggers ABI-check behavior
looks like.

I'm still liking the in-tree-file idea better.  For one thing,
it's not clear how expensive it'd be to search the git metadata
for this other stuff.

            regards, tom lane

On Jul 13, 2025, at 15:43, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I'm still liking the in-tree-file idea better.  For one thing,
> it's not clear how expensive it'd be to search the git metadata
> for this other stuff.

How bout we keep it a simple as possible to start with: make it either the last tag in the branch or else the first
commitin the branch (the branch commit). Except for master, in which case perhaps since the last time a branch was
created?

Best,

David

"David E. Wheeler" <david@justatheory.com> writes:
> How bout we keep it a simple as possible to start with: make it either the last tag in the branch or else the first
commitin the branch (the branch commit). Except for master, in which case perhaps since the last time a branch was
created?

I do not think we either need or want ABI checking on master.
I'm not even convinced we want it on a currently-beta branch;
having to update the reference point after each ABI-breaking
commit seems like mostly make-work until we approach RC stage.
I like the idea of an in-tree file partly because the decision
about when to create it in a branch could be made dynamically.

If we do want to drive this off tags, I'd suggest a rule like
"Use the most recent tag of the form REL_m_n or REL_m_RCn;
if there is none in this branch, don't perform ABI checking".
But I still wonder how expensive it'll be to determine that.

            regards, tom lane

On Mon, 14 Jul 2025 at 00:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Another idea could be an in-tree file, different in each branch,
> that records the hash of the commit we presently want to compare to.
> This would require a small amount of additional manual effort to
> maintain, but maybe it's the most flexible way.  Notably, we could
> say that if the file isn't present in a given branch then we don't
> want any ABI checking, whereas with the tag approach I think we'd
> need some hard-coded rule about which branches to ignore.

In-tree file seems fair as buildfarm clients already use them for operations.
Additionally, the abidiff tool provides its own way to suppress
specific symbols in the reports [1], which, as you mentioned, would
rarely be required for stable branches. Beta branches could be handled
similarly, and this will also not require us to maintain the record of
commits for individual branches; a single suppression file might work
for all branches. Hence, we can directly use the most recent tag as
discussed earlier.

Regards,
Mankirat

[1] - https://sourceware.org/libabigail/manual/suppression-specifications.html

On Aug 26, 2025, at 17:43, Mankirat Singh <mankiratsingh1315@gmail.com> wrote:

> If ABI differences are found, you’ll see "log files for step abi-compliance-check:"; if not, you’ll see "no abi diffs
foundin this run" (example [3]). 

I’m quite happy to have a working build farm animal making these reports. Yesterday I had baza compare changes since
the.1 release of each branch. Here’s the REL_17_STABLE report[1] Mankirat referenced. I’ve since removed the base tag
configuration,so today’s v17 build[2] has no diff. 

If anyone else would like to try it on their animals prior to its review, merge and release, just copy
ABICompCheck.pm[3]into PGBuild/Modules and add `ABICompCheck` to the `modules` list in `build-farm.conf`. See
Mankirat’sbranch for additional configuration[4]. 

>  I do have some questions related to the module, that, should we include any other binaries (besides postgres, ecpg,
andlibpq.so) in the default list? 

These seem like reasonable defaults to me. Anyone care about ABI changes in other binaries?

> I plan to work on false positives by adding abidiff suppression files per branch (possibly managed in a community
repo).I’d love to hear ideas on this. 

ISTM that starting with full reports for now ought to be fine, to see where the cow paths develop and where issues
annoyand fix those. 

Also, what rules should be put in place to make a build “fail” based on what has changed? Right now, I believe, the
resultsare simply reported without a pass/fail. Would a failure status be useful, given some level of change? 

> Although this is my last week under the official google summer of code project timeline, I do plan to stay involved
inthe Postgres community and work towards core database development in the future. 

Thank you, Mankirat, I think this will be very useful!

Best,

David

[1]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-08-26%2000%3A12%3A11&stg=abi-compliance-check
[2]
https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-08-27%2013%3A25%3A11&stg=abi-compliance-check
[3] https://github.com/MankiratSingh1315/pg-bf-client-code/blob/abi-comp-check/PGBuild/Modules/ABICompCheck.pm
[4] https://github.com/MankiratSingh1315/pg-bf-client-code/blob/abi-comp-check/build-farm.conf.sample#L419-L440

On Aug 31, 2025, at 10:17, Mankirat Singh <mankiratsingh1315@gmail.com> wrote:

> I have updated the module to report a failure status whenever any ABI change is detected without specific conditions.
Forexample, the image attached from my local server gave a failure status on the ABICompCheck step when comparing
REL_17_1with the head commit of REL_17_STABLE, as ABI changes were found. 

Thank you! I updated baza with the latest a couple days ago.

> Also, I think we can also have a configuration option for animal owners to toggle ABI change status on or off,
thoughts?

Mabye? Might be worth waiting to see how much of an issue it is. If there is a failure a then a fix, it should turn
greenagain. It might not be necessary. 

What do you think, Hackers?

Best,

David

On Sep 1, 2025, at 09:12, David E. Wheeler <david@justatheory.com> wrote:

>> Also, I think we can also have a configuration option for animal owners to toggle ABI change status on or off,
thoughts?
>
> Mabye? Might be worth waiting to see how much of an issue it is. If there is a failure a then a fix, it should turn
greenagain. It might not be necessary. 
>
> What do you think, Hackers?

I had baza configured to test ABI changes since the .1 tags for each of the maintenance branches for the past few days
togive a feel for what those failures look like. It was easy to configure: 

    tag_for_branch => {
        REL_17_STABLE => 'REL_17_1',
        REL_16_STABLE => 'REL_16_1',
        REL_15_STABLE => 'REL_15_1',
        REL_14_STABLE => 'REL_14_1',
        REL_13_STABLE => 'REL_13_1',
    }

You can see the results from today here:

REL_18_RC1 -> f256a7b

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-08%2012%3A42%3A42&stg=abi-compliance-check

REL_17_1 -> 3e6dfcf

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-08%2012%3A28%3A55&stg=abi-compliance-check

REL_16_1 -> 12f5768

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-08%2012%3A14%3A10&stg=abi-compliance-check

REL_15_1 -> 1852ec5

https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=baza&dt=2025-09-08%2012%3A00%3A02&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

The RC1 change surprised me a little; here’s the log:

> Leaf changes summary: 1 artifact changed
> Changed leaf types summary: 0 leaf type changed
> Removed/Changed/Added functions summary: 0 Removed, 1 Changed, 0 Added function
> Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
>
> 1 function with some sub-type change:
>
> [C] 'function void CheckValidResultRel(ResultRelInfo*, CmdType, List*)' has some sub-type changes:
> parameter 4 of type 'List*' was added
> parameter 3 of type 'List*' changed:
> entity changed from 'List*' to 'typedef OnConflictAction'
> type size changed from 8 to 4 (in bytes)
> type alignment changed from 0 to 4

Presumably this is expected, but it looks like it might be an issue if it weren’t a pre-release change, yes?

In any event, I’ve restored the default configuration so that tomorrow’s builds will start comparing from the latest
tagin each branch, which should return all but REL_18_STABLE to passing again. 

Anyone else interested in trying out the compliance checker on their build farm animals? It works only on Linux for
now,I believe. 

Best,

David

On 08.09.25 17:17, David E. Wheeler wrote:
> The RC1 change surprised me a little; here’s the log:
> 
>> Leaf changes summary: 1 artifact changed
>> Changed leaf types summary: 0 leaf type changed
>> Removed/Changed/Added functions summary: 0 Removed, 1 Changed, 0 Added function
>> Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable
>>
>> 1 function with some sub-type change:
>>
>> [C] 'function void CheckValidResultRel(ResultRelInfo*, CmdType, List*)' has some sub-type changes:
>> parameter 4 of type 'List*' was added
>> parameter 3 of type 'List*' changed:
>> entity changed from 'List*' to 'typedef OnConflictAction'
>> type size changed from 8 to 4 (in bytes)
>> type alignment changed from 0 to 4
> Presumably this is expected, but it looks like it might be an issue if it weren’t a pre-release change, yes?

This was a change that was intentionally backpatched in a different way 
in order to preserve ABI compatibility.  Compare commits 344662848ac on 
REL_18_STABLE and 0b934d3994f on REL_17_STABLE.  So everything is in 
order. :)

On Sep 12, 2025, at 10:37, Peter Eisentraut <peter@eisentraut.org> wrote:

> This was a change that was intentionally backpatched in a different way in order to preserve ABI compatibility.
Comparecommits 344662848ac on REL_18_STABLE and 0b934d3994f on REL_17_STABLE.  So everything is in order. :) 

Excellent! But an example like this presumably helps make Tom’s case that each branch could have a file that suggests
whichcommit to use as the base for comparison, so that in this example it could be set to 344662848ac and the failures
wouldgo away. As it is, they will persist until a new tag is added or one overrides the base in the build farm client
config.

Best,

David

On 12.09.25 16:52, David E. Wheeler wrote:
> On Sep 12, 2025, at 10:37, Peter Eisentraut <peter@eisentraut.org> wrote:
> 
>> This was a change that was intentionally backpatched in a different way in order to preserve ABI compatibility.
Comparecommits 344662848ac on REL_18_STABLE and 0b934d3994f on REL_17_STABLE.  So everything is in order. :)
 
> 
> Excellent! But an example like this presumably helps make Tom’s case that each branch could have a file that suggests
whichcommit to use as the base for comparison, so that in this example it could be set to 344662848ac and the failures
wouldgo away. As it is, they will persist until a new tag is added or one overrides the base in the build farm client
config.

I don't think we need any ABI checking until there is a dot-0 release, 
so I don't agree that a facility like that is needed.  Just compare 
against the previous release tag.

On Sep 12, 2025, at 11:37, Peter Eisentraut <peter@eisentraut.org> wrote:

> I don't think we need any ABI checking until there is a dot-0 release, so I don't agree that a facility like that is
needed.

Hrm. I wonder how best to filter out a branch without a dot-0 tag.

> Just compare against the previous release tag.

Yep, that’s what it’s doing.

Best,

David

Peter Eisentraut <peter@eisentraut.org> writes:
> On 12.09.25 16:52, David E. Wheeler wrote:
>> Excellent! But an example like this presumably helps make Tom’s case that each branch could have a file that
suggestswhich commit to use as the base for comparison, so that in this example it could be set to 344662848ac and the
failureswould go away. As it is, they will persist until a new tag is added or one overrides the base in the build farm
clientconfig. 

> I don't think we need any ABI checking until there is a dot-0 release,
> so I don't agree that a facility like that is needed.  Just compare
> against the previous release tag.

My concern is that when we get a report, we might decide to apply
some fix to remove the ABI delta, or we might decide it's intentional
and/or harmless and leave the code as-is.  In the latter case, the
ABI-checking BF animal(s) will be red until the next release is made,
which helps nobody, particularly because it will probably interfere
with noticing any ABI breaks in later commits.

So I think we do need a way to move a branch's baseline that is not
rigidly tied to making a release.  The only way we could possibly use
this testing methodology without that is if the ABI checker agrees
100.00% with our opinion of what is or is not a breaking change.
Which we already see isn't so.  A concrete example is the hack we
frequently use of sandwiching a new field into what had been padding
space.  I would be astonished if an ABI checker doesn't flag that,
but the checker's output doesn't make it not OK.

I'm not wedded to any particular way of implementing that, though
I'd prefer to avoid basing it on git tags given that they can't
readily be deleted or moved.

            regards, tom lane

On 2025-Sep-12, Tom Lane wrote:

> My concern is that when we get a report, we might decide to apply
> some fix to remove the ABI delta, or we might decide it's intentional
> and/or harmless and leave the code as-is.  In the latter case, the
> ABI-checking BF animal(s) will be red until the next release is made,
> which helps nobody, particularly because it will probably interfere
> with noticing any ABI breaks in later commits.

The solution I propose for this (which I have mentioned before) is to
allow for our source tree to carry exclusion files.  The ABI checker
would refrain from turning red for any breaks that match what's in those
files.  For instance I think in branch REL_18_STABLE we would add a
subdirectory like
/src/tools/abicheck-exclude/18.1
where we would store one exclusion file that suppresses reporting for
each ABI-breaking change between 18.0 and 18.1.  (Or maybe name the
subdir REL_18_1).  

So when we break ABI compatibility after having 18.0 out and before
releasing 18.1, the animal turns red and we'd either add a file there
or fix the ABI break -- both of which turn the animal back to green.

It's somewhat equivalent to valgrind.supp, except the files are
version-specific.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Once again, thank you and all of the developers for your hard work on
PostgreSQL.  This is by far the most pleasant management experience of
any database I've worked on."                             (Dan Harris)
http://archives.postgresql.org/pgsql-performance/2006-04/msg00247.php

=?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@kurilemu.de> writes:
> On 2025-Sep-12, Tom Lane wrote:
>> My concern is that when we get a report, we might decide to apply
>> some fix to remove the ABI delta, or we might decide it's intentional
>> and/or harmless and leave the code as-is.

> The solution I propose for this (which I have mentioned before) is to
> allow for our source tree to carry exclusion files.  The ABI checker
> would refrain from turning red for any breaks that match what's in those
> files.

That seems substantially more complicated than just moving which
commit is considered the baseline.  It might be less reliable too:
if the exclusions are written sloppily, they might hide problems
we'd rather find out about.  (I do not regard valgrind suppression
files as a model of good engineering...)

            regards, tom lane

On Sep 12, 2025, at 13:53, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> That seems substantially more complicated than just moving which
> commit is considered the baseline.  It might be less reliable too:
> if the exclusions are written sloppily, they might hide problems
> we'd rather find out about.  (I do not regard valgrind suppression
> files as a model of good engineering...)

It would certainly be easier to start with some marker for the baseline commit. We an always add some more complicated
stuffin the future if it turns out to be desirable. I know there’s some stuff built in to Abigail for that. But IMHO it
makessense to start with the simplest solution and see whether anything else is needed later. 

D