Subject: Re: Potential ABI breakage in upcoming minor releases

From: Tom Lane <[email-masked]>
Date: Fri, 15 Nov 2024 10:09:54 -0500

Aleksander Alekseev <[email-masked]> writes:
> Hi Macro,
>> The problem here is that because TimescaleDB compiled against 17.0
>> assumes a struct size of 376 (on my laptop) while PostgreSQL allocated
>> the array with a struct size of 384, so the pointer math no longer
>> holds and the whichrel value becomes nonsense. (1736263376 for
>> whatever reason)

> Thanks for reporting. Yes, the code assumed fixed
> sizeof(ResultRelInfo) within a given PG major release branch which
> turned out not to be the case. We will investigate whether it can be
> easily fixed on TimescaleDB side.

Yeah, the array-stride problem seems extremely hard to work around,
because whichever size it is, you can't get code compiled with the
other size to work.  I believe ResultRelInfo is the only node type
we use arrays of, so this was a particularly unfortunate place
to break ABI, but there it is.

I'm starting to lean to the opinion that we need a re-wrap.
Given that padding holes exist, the code changes shouldn't
be big.

			regards, tom lane

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 09:52:23 -0800

> to break ABI, but there it is.

I see ModifyTableState.resultRelInfo; are there other known ResultRelInfo
arrays?  That does add firebird_fdw to the list of PGXN modules requiring
rebuilds:

$ grep -rE 'resultRelInfo(\[|.* \+ )' | tee /tmp/3 | sed 's/-[^:]*/:/'|sort -u
firebird_fdw::                  resultRelation = mtstate->resultRelInfo[0].ri_RangeTableIndex;
firebird_fdw::          resultRelInfo > mtstate->resultRelInfo + mtstate->mt_whichplan)

> I'm starting to lean to the opinion that we need a re-wrap.

Perhaps.  Even if we do rewrap for some reason, it's not a given that
restoring the old struct size is net beneficial.  If we restore the old struct
size in v16.6, those who rebuild for v16.5 would need to rebuild again.
Hearing about other ResultRelInfo arrays will help clarify that decision.

Either way, users of timescaledb should rebuild timescaledb for every future
PostgreSQL minor release.

---

From: Pavan Deolasee <[email-masked]>
Date: Fri, 15 Nov 2024 23:39:24 +0530

> Hearing about other ResultRelInfo arrays will help clarify that decision.

Looking more carefully at the usage of `ResultRelInfo` in the PGD code, I
think we might also be impacted by it. At one place, we loop through the
`es_result_relations` array and a size mismatch there will cause problems.
Interestingly, in v14 and above, we read from `es_opened_result_relations`,
which is a List, so it should be safe. I will try some tests on v13 to see
if they result in crashes. But it seems quite likely by reading the code.

Thanks,
Pavan

---

From: "David E. Wheeler" <[email-masked]>
Date: Fri, 15 Nov 2024 13:24:35 -0500

> PostgreSQL minor release.

Ugh, was really hoping to get to a place where we could avoid requiring rebuilds of any extension for every minor release. :-( We even added ABI guidance to the docs[1] about this.

Best,

David

[1]: https://commitfest.postgresql.org/48/5080/

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 10:35:29 -0800

> if they result in crashes. But it seems quite likely by reading the code.

I agree.  I'm investigating the scope of damage from es_result_relations.

---

From: Pavan Deolasee <[email-masked]>
Date: Sat, 16 Nov 2024 00:10:06 +0530

> if they result in crashes. But it seems quite likely by reading the code.
Ah, the addition of a member to `ResultRelInfo` did not happen in v12 and
v13, even though the commit was backpatched all the way to v12. Maybe we
(PGD) got twice lucky :-) There could be other extensions which might be
looping through `es_result_relations` though and get impacted.

Thanks,
Pavan

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 11:03:41 -0800

On Sat, Nov 16, 2024 at 12:10:06AM +0530, Pavan Deolasee wrote:
> On Fri, Nov 15, 2024 at 11:39 PM Pavan Deolasee <[email-masked]> wrote:

True.

> looping through `es_result_relations` though and get impacted.

Like you say, trouble with es_result_relations would be a v12/v13 phenomenon,
and v12/v13 ABI didn't change.  If the v12/v13 ABI had changed here, that
would have moved pg_pathman from the "rebuild if using asserts" category to
the "rebuild unconditionally" category, due to this code:

pg_pathman::                       estate->es_num_result_relations * sizeof(ResultRelInfo));
pg_pathman::    estate->es_result_relations[estate->es_num_result_relations] = *rri;      
pg_pathman::    if (result_rels_allocated <= estate->es_num_result_relations)
pg_pathman::    return estate->es_num_result_relations++;                                 

Since the v12/v13 ABI didn't change, pg_pathman remains in the "rebuild if
using asserts" category.

---

From: Tom Lane <[email-masked]>
Date: Fri, 15 Nov 2024 15:29:21 -0500

> size in v16.6, those who rebuild for v16.5 would need to rebuild again.

I think what we should say is "sorry, 16.5 is broken for use with
these extensions, use another minor version".  If we don't undo the
struct size change then 16.5 is effectively a major version update for
affected extensions: they cannot build a binary release that works
with both older and newer minor releases.  That's a packaging
disaster, especially if it impacts more than timescale.  The more
so if more than one release branch is affected.

> PostgreSQL minor release.

We really don't want that either.  I recall that somebody (Peter E?)
had been looking into tools for automatically checking ABI
compatibility in stable branches.  My takeaway from this mess is
that we need to move the priority of that project way up.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Fri, 15 Nov 2024 16:13:16 -0500

After some quick checking with "git diff", I can confirm that
there is no ABI break for ResultRelInfo in v12 or v13.  To fix
it in v14-v17, we could do this:

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 418c81f4be..17b0ec5138 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -484,6 +484,9 @@ typedef struct ResultRelInfo
 	/* Have the projection and the slots above been initialized? */
 	bool		ri_projectNewInfoValid;
 
+	/* updates do LockTuple() before oldtup read; see README.tuplock */
+	bool		ri_needLockTagTuple;
+
 	/* triggers to be fired, if any */
 	TriggerDesc *ri_TrigDesc;
 
@@ -592,9 +595,6 @@ typedef struct ResultRelInfo
 	 * one of its ancestors; see ExecCrossPartitionUpdateForeignKey().
 	 */
 	List	   *ri_ancestorResultRels;
-
-	/* updates do LockTuple() before oldtup read; see README.tuplock */
-	bool		ri_needLockTagTuple;
 } ResultRelInfo;
 
 /* ----------------

(The next-to-last field varies across branches, but this general
idea will work.)  In other words, put ri_needLockTagTuple in the
same place it is in HEAD.  In other words, our current guidelines
for preserving ABI compatibility actually *created* this disaster,
because the HEAD change was fine from an ABI standpoint but what
was done in back branches was not.  So we do need to rethink how
that's worded.

			regards, tom lane

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 14:27:49 -0800

> so if more than one release branch is affected.

Currently, we have Christoph Berg writing "I'd say the ship has sailed, a new
release would now break things the other way round." and you writing in favor
of undoing.  It think it boils down to whether you want N people to recompile
twice or M>N people to recompile once, where we don't know N or M except that
M > N.  Fortunately, the N are probably fairly well represented in this
thread.  So to all: please speak up by 2024-11-16T17:00+0000 if you think it's
the wrong choice to bring back the v16.4 ABI and tell people to rebuild
extensions built against v16.5 (likewise for corresponding versions of
v14-v17).  Currently, the plan of record is to do that.

---

From: Tom Lane <[email-masked]>
Date: Fri, 15 Nov 2024 17:37:01 -0500

> v14-v17).  Currently, the plan of record is to do that.

Well, no, what I propose is for some number of people to not recompile
extensions at all.  Only the sort of early adopters who install a new
PG release on day one will have run into this, so I anticipate that
that number will be much larger than the number who have already done
a rebuild.

			regards, tom lane

---

From: Noah Misch <[email-masked]>
Date: Fri, 15 Nov 2024 15:07:08 -0800

> that number will be much larger than the number who have already done

If you're saying that undoing the ABI break would allow the M builders to
rebuild zero times, I agree.  In other words, here are the number of rebuilds
by builder group:

If we undo, group N: 2 rebuilds
If we undo, group M: 0 rebuilds
If we don't undo, group N: 1 rebuild
If we don't undo, group M: 1 rebuild

I think you're predicting that M is much larger than N.  That may be so.  A
builder will be in group N if they publish or consume 2024-11-14 builds for
any amount of time.  A builder is in group M if they skip the 2024-11-14
release entirely.  Note that people getting binaries from someone else don't
increase the size of N or M; builders of binaries (both packagers and
self-packaging users) are the populations to count here.

In this thread, Christoph Berg and Gregory Smith have asserted membership in
that group N.  We can indeed advise builders to wait in group M instead of
joining group N, but N is already nonempty.  Also, some builders and users of
their builds can't afford to wait.  That's why I described it the way I did.
I'm no longer arguing against the undo, but I'm trying to explain the
consequences rigorously.

---

From: "David E. Wheeler" <[email-masked]>
Date: Fri, 15 Nov 2024 19:00:02 -0500

On Nov 15, 2024, at 16:13, Tom Lane <[email-masked]> wrote:

> In other words, our current guidelines

What bit is mis-worded? The guidance Peter committed[1] says that “PostgreSQL makes an effort to avoid server
ABI breaks in minor releases.” It sounds to me like that effort wasn’t held up in back-branches, the sources for minor releases.

But maybe you had some other guidance in mind?

D

[1]: https://github.com/postgres/postgres/commit/e54a42a

---

From: Tom Lane <[email-masked]>
Date: Fri, 15 Nov 2024 19:30:56 -0500

> ABI breaks in minor releases.”

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.

The other bit of documentation we probably need is some annotation in
struct ResultRelInfo saying "do not change the sizeof() this struct
in back branches, period".

			regards, tom lane

---

From: Christoph Berg <[email-masked]>
Date: Sat, 16 Nov 2024 15:20:05 +0100

Re: Noah Misch
> I'm no longer arguing against the undo, but I'm trying to explain the

I'll just have to explain what happened here to the Debian security
team who just released the 16.5 update. We can do 16.6 on top of that.

Christoph

---

From: "David E. Wheeler" <[email-masked]>
Date: Sat, 16 Nov 2024 11:04:40 -0500

On Nov 15, 2024, at 19:30, Tom Lane <[email-masked]> wrote:

> in back branches, period”.

This sounds like complementary documentation for committers; totally agree the guidance should be written down somewhere.

D

---

From: Tom Lane <[email-masked]>
Date: Sat, 16 Nov 2024 13:00:44 -0500

> team who just released the 16.5 update. We can do 16.6 on top of that.

Fixes are pushed, and we'll wrap new releases next week on the
usual release schedule.

			regards, tom lane

---

From: Tom Lane <[email-masked]>
Date: Mon, 25 Nov 2024 20:56:50 -0500

[ getting back to the document-ABI-breakage-rules-better topic ... ]

> 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.

* Add new enum entries at the end of the enum's list, so that existing
  entries don't change value.

* These rules only apply to released branches.  In master, or in a new
  branch that has not yet reached RC status, it's better to place new
  fields and enum values in natural positions.  Changing function
  signatures is fine too, unless there are so many callers that
  a compatibility wrapper seems advisable.

> in back branches, period".

Something like

    /*
     * DO NOT change sizeof(ResultRelInfo) in a released branch.  This
     * generally makes it unsafe to add fields here in a released branch.
     */

at the end of struct ResultRelInfo's definition.

None of this is a substitute for installing some kind of ABI-checking
infrastructure; but that project doesn't seem to be moving fast.

			regards, tom lane

---

From: Bertrand Drouvot <[email-masked]>
Date: Tue, 26 Nov 2024 05:16:05 +0000

Hi,

On Mon, Nov 25, 2024 at 08:56:50PM -0500, Tom Lane wrote:
>   deciding what is "padding".)

What about providing a decision table to help considering for 32-bit, something
like (proposed in [1])?

64-bit hole size | use on 32-bit?
-----------------|---------------
<=3 bytes        | safe to use
4 bytes          | don't use
5-7 bytes        | use first (hole_size - 4) bytes only

[1]: https://www.postgresql.org/message-id/flat/ZzcR%2BoQmUOIm6RVF%40ip-10-97-1-34.eu-west-3.compute.internal#08182ae6a6719632acf52fe4d90e9778

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: "David E. Wheeler" <[email-masked]>
Date: Tue, 26 Nov 2024 08:48:05 -0500

> infrastructure; but that project doesn't seem to be moving fast.

These sound great, very useful. I wonder if the guideline docs Peter added should link to these items (and back).

D

---

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

Bertrand Drouvot <[email-masked]> writes:
>> ...  (But consider both 32-bit and 64-bit cases when

> 64-bit hole size | use on 32-bit?

Mumble ... that seems too simplistic to me.  Admittedly,
I can't offhand think of an inter-platform variation that
would move field offsets by something other than a multiple
of 4 bytes, so maybe it's correct.

In any case, I think the purpose of these notes is just to remind
people of issues to think about, not to provide complete solution
recipes, so I'd rather not go into that much detail.

			regards, tom lane

---

From: Noah Misch <[email-masked]>
Date: Fri, 29 Nov 2024 12:39:37 -0800

> https://wiki.postgresql.org/wiki/Committing_checklist#Maintaining_ABI_compatibility_while_backpatching

I'd say the source tree's <sect2 id="xfunc-api-abi-stability-guidance">, which
David Wheeler mentioned, is authoritative.  It currently matches
postgr.es/c/e54a42a.  Let's update both or change that wiki heading to point
to the authoritative doc section.

>   calls a new function with more/different arguments.

Sounds fine.

>   extensions to index into arrays of ResultRelInfos.

We should be able to avoid ResultRelInfo length changes, since there are other
executor structs to choose from.

I think this thread implies an open question about assert builds.  Whenever we
add to the end of a struct that extensions heap-allocate (makeNode() or
otherwise), assert builds will get "write past chunk end" warnings
(postgr.es/m/[email-masked]).  Should we say that (a) assert
builds need to rebuild with every minor release, that (b) PostgreSQL will
treat this like it treats non-assert rebuild requirements, or something else?
If (a), we committers also need to confirm that each lengthening of an
extension-allocated struct doesn't change the resulting palloc chunk size.

Corresponding doc paragraphs:

      <para>
       When a change <emphasis>is</emphasis> required,
       <productname>PostgreSQL</productname> will choose the least invasive
       change possible, for example by squeezing a new field into padding
       space or appending it to the end of a struct. These sorts of changes
       should not impact extensions unless they use very unusual code
       patterns.
      </para>

      <para>
       In rare cases, however, even such non-invasive changes may be
       impractical or impossible. In such an event, the change will be
       carefully managed, taking the requirements of extensions into account.
       Such changes will also be documented in the release notes (<xref
       linkend="release"/>).
      </para>

I think those paragraphs aren't practical for packager needs, or they leave
too much undefined.  There's no available algorithm for classifying extensions
as "unusual".  timescaledb is objectively unusual, being the one extension
known to break in response to a ResultRelInfo size change in v17.  That
observation requires knowing what will change, so it wouldn't have helped a
packager classify in advance.  For non-unusual extensions, the second
paragraph implies packagers should check release notes on Monday, being ready
to rebuild by Thursday.  Assuming nobody wants to create a rebuild procedure
in 3 days, the packager will have a procedure already.  If they have a rebuild
procedure, they're better off rebuilding every time.  That way, they don't
need to scrutinize the release notes under time pressure, and they're
protected even if hackers overlook the incompatibility.  Those are the
incentives our docs create today, right or wrong.

I gather we want packagers to feel comfortable not rebuilding every time.  One
way to achieve that could be text like:

  If a change is suspected to require extension rebuilds in well-known
  extensions, a release at least 3 months before the breaking release will
  include "<specific phrase>" in its release notes.

Then packagers who are taking the risk of not rebuilding every time will have
3 months to prepare, not the 3 days we're currently giving.  The point about
"well-known extensions" is based on my practice of grepping PGXN.  That would
not have found timescaledb.  Should we name PGXN explicitly, or should we be
vague like that draft?  I'd be comfortable naming any number of repositories
that make it equally easy to bulk-download the whole repository.

How else might we make it safe for packagers to skip rebuilding extensions for
the majority of minor releases?

>   a compatibility wrapper seems advisable.

Those two sound fine.

> at the end of struct ResultRelInfo's definition.

I agree with adding a comment to the end of the struct.  Thanks for writing
all this.  I'd also have that comment refer to ModifyTableState.resultRelInfo
specifically.  That way, if the ModifyTable implementation changes to remove
this hazard, it's easier to trace the ability to remove this comment.

> infrastructure; but that project doesn't seem to be moving fast.

Right, they're complementary.  The documentation is about what ABI checker
complaints we choose to ignore.  An extension buildfarm is another
complementary idea that comes up occasionally.

---

From: "David E. Wheeler" <[email-masked]>
Date: Sun, 1 Dec 2024 11:13:21 -0500

> that make it equally easy to bulk-download the whole repository.

I’m wondering how we can help more projects make releases on PGXN, if for now other reason than to enable these kinds of checks without much additional effort. PGXN releases don’t require a ton of work, requiring just a META.json file in a zip file, and there are GitHub workflows to automate such releases. Anyone interested please hit me up directly for details. I even make pull requests like this one for pg_partman[1].

Best,

David

[1]: https://github.com/pgpartman/pg_partman/pull/671/files

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 4 Dec 2024 20:29:57 +0100

On 29.11.24 21:39, Noah Misch wrote:
>> https://wiki.postgresql.org/wiki/ 
>> Committing_checklist#Maintaining_ABI_compatibility_while_backpatching

I don't know if this proposing to merge the text in the wiki into the docs?

The text in the docs is describing to extension authors what they can 
expect.  The text in the wiki is advice for server developers how to 
attempt to satisfy those expectations.  So I think those can be separate 
pieces of text.

---

From: Tom Lane <[email-masked]>
Date: Wed, 04 Dec 2024 15:28:36 -0500

> attempt to satisfy those expectations.  So I think those can be separate 

Yes, that was my feeling as well.  The text on the wiki contemplates
that we'd merge it into the main docs whenever it seems stable enough,
but I'm not sure we're there yet (this discussion suggests not ;-)).
In any case it feels like it should be a different chunk of text than
xfunc-api-abi-stability-guidance, which is aimed at extension
developers not core developers.

			regards, tom lane