public inbox for [email protected]  

help / color / mirror / Atom feed

From: Matthias van de Meent <[email protected]>
To: Tom Lane <[email protected]>
Cc: David E. Wheeler <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Aleksander Alekseev <[email protected]>
Cc: pgsql-hackers <[email protected]>
Cc: Marco Slot <[email protected]>
Cc: Alvaro Herrera <[email protected]>
Cc: Christoph Berg <[email protected]>
Cc: Pavan Deolasee <[email protected]>
Subject: Re: Potential ABI breakage in upcoming minor releases
Date: Tue, 26 Nov 2024 11:18:06 +0100
Message-ID: <CAEze2WgjQZJFnKwjRX9fUX0syW=qQbU+Y_4mkK9EN-oQB2D9_g@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<CAJ7c6TPcUJq_Bwo69f6ysRA1UbmSh_hOOEzJ2EvM0cW=3VEh5w@mail.gmail.com>
	<CAFMSG9Hy-2-8tfmmKy7noCbA3Yn1UY2SFUwYj7wHODTO8gDm8w@mail.gmail.com>
	<CAJ7c6TMCc85YoiSKex5Sztz6bEmV-0YqL7FY=_MxxcyoMGTDfQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

On Tue, 26 Nov 2024 at 02:57, Tom Lane <[email protected]> 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_backpatchi...
>
> 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)

view thread (3+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Potential ABI breakage in upcoming minor releases
  In-Reply-To: <CAEze2WgjQZJFnKwjRX9fUX0syW=qQbU+Y_4mkK9EN-oQB2D9_g@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox