Subject: Re: Potential ABI breakage in upcoming minor releases

From: Matthias van de Meent <[email-masked]>
Date: Tue, 26 Nov 2024 11:18:06 +0100

On Tue, 26 Nov 2024 at 02:57, Tom Lane <[email-masked]> wrote:
>
> [ getting back to the document-ABI-breakage-rules-better topic ... ]
>
> I wrote:
> > That text says exactly nothing about what specific code changes to
> > make or not make.  I'm not sure offhand where (or if) we have this
> > documented, but there's an idea that adding fields at the end of
> > a struct is safer ABI-wise than putting them in the middle.  Which
> > is true if you can't squeeze them into padding space.  Here, that
> > could have been done and probably should have.
>
> I remembered where that's documented:
>
> https://wiki.postgresql.org/wiki/Committing_checklist#Maintaining_ABI_compatibility_while_backpatching
>
> The current text there is:
>
>
> * You can only really change the signature of a function with local
>   linkage, perhaps with a few rare exceptions.
>
> * You cannot modify any struct definition in src/include/*. If any new
>   members must be added to a struct, put them at the end in
>   backbranches. It's okay to have a different struct layout in
>   master. Even then, extensions that allocate the struct can break via
>   a dependency on its size.
>
> * Move new enum values to the end.
>
>
> I propose rewriting and expanding that:
>
>
> * Don't change the signature of non-static functions.  One common
>   workaround is to change the existing function into a wrapper that
>   calls a new function with more/different arguments.
>
> * Don't change the contents of globally-visible structs, specifically
>   not the offsets of existing fields.  If you must add a new field,
>   the very best way is to put it into existing alignment padding
>   between fields.  (But consider both 32-bit and 64-bit cases when
>   deciding what is "padding".)  If that's not possible, it may be okay
>   to add the field at the end of the struct; but you have to worry
>   about whether any extensions depend on the size of the struct.
>   This will not work well if extensions allocate new instances of the
>   struct.  One particular gotcha is that it's not okay to change
>   sizeof(ResultRelInfo), as there are executor APIs that require
>   extensions to index into arrays of ResultRelInfos.

I would also add something like the following, to protect against
corruption and deserialization errors of the catalog pg_node_tree
fields, because right now the generated readfuncs.c code ignores field
names, assumes ABI field order, and is generally quite sensitive to
binary changes, which can cause corruption and/or errors during
readback of those nodes:

* If you update the contents of Nodes which are stored on disk as
pg_node_tree, you also have to make sure that the read function for
that node type is able to read both the old and the new serialized
format.

A reference to readfuncs.c/readfuncs.c, and/or the usage of
pg_node_tree for (among others) views, index/default expressions,
constraints, and partition bounds would maybe be useful as well.

> None of this is a substitute for installing some kind of ABI-checking
> infrastructure;

Agreed.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

---

From: Tom Lane <[email-masked]>
Date: Tue, 26 Nov 2024 13:24:35 -0500

> that node type is able to read both the old and the new serialized

No, actually that policy is "You cannot change the contents of
Node structs at all if they could appear in stored views, rules,
default expressions, etc".  We don't do catversion bumps in back
branches, which means that catalog contents have to be backwards
as well as forwards compatible across minor releases.

That's not really something that belongs under the heading of
ABI breaks, I think.  It's about "the on-disk representation of
catalogs is frozen within a release series", which extends to
more things than Nodes.  Still, maybe a parenthetical remark
wouldn't hurt.  Perhaps:

  (Keep in mind also that Node structs can't be changed at all
  if they might appear in stored views, rules, etc.)

			regards, tom lane