public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v3] Rework redundant loop in subtrans.c
26+ messages / 8 participants
[nested] [flat]
* [PATCH v3] Rework redundant loop in subtrans.c
@ 2024-03-04 10:49 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw)
---
src/backend/access/transam/subtrans.c | 29 +++++++--------------------
1 file changed, 7 insertions(+), 22 deletions(-)
diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c
index dc9566fb51..50bb1d8cfc 100644
--- a/src/backend/access/transam/subtrans.c
+++ b/src/backend/access/transam/subtrans.c
@@ -311,7 +311,7 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
FullTransactionId nextXid;
int64 startPage;
int64 endPage;
- LWLock *prevlock;
+ LWLock *prevlock = NULL;
LWLock *lock;
/*
@@ -324,42 +324,27 @@ StartupSUBTRANS(TransactionId oldestActiveXID)
nextXid = TransamVariables->nextXid;
endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid));
- prevlock = SimpleLruGetBankLock(SubTransCtl, startPage);
- LWLockAcquire(prevlock, LW_EXCLUSIVE);
- while (startPage != endPage)
+ for (;;)
{
lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release
- * the lock on the old bank and acquire on the new bank.
- */
if (prevlock != lock)
{
- LWLockRelease(prevlock);
+ if (prevlock)
+ LWLockRelease(prevlock);
LWLockAcquire(lock, LW_EXCLUSIVE);
prevlock = lock;
}
(void) ZeroSUBTRANSPage(startPage);
+ if (startPage == endPage)
+ break;
+
startPage++;
/* must account for wraparound */
if (startPage > TransactionIdToPage(MaxTransactionId))
startPage = 0;
}
- lock = SimpleLruGetBankLock(SubTransCtl, startPage);
-
- /*
- * Check if we need to acquire the lock on the new bank then release the
- * lock on the old bank and acquire on the new bank.
- */
- if (prevlock != lock)
- {
- LWLockRelease(prevlock);
- LWLockAcquire(lock, LW_EXCLUSIVE);
- }
- (void) ZeroSUBTRANSPage(startPage);
LWLockRelease(lock);
}
--
2.39.2
--r4q4lfzvecohuykp--
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
@ 2024-11-15 15:09 Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2024-11-15 15:09 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; +Cc: pgsql-hackers; Marco Slot <[email protected]>; Noah Misch <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Aleksander Alekseev <[email protected]> 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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-15 17:52 ` Noah Misch <[email protected]>
2024-11-15 18:09 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
2024-11-15 18:24 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
0 siblings, 3 replies; 26+ messages in thread
From: Noah Misch @ 2024-11-15 17:52 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Fri, Nov 15, 2024 at 10:09:54AM -0500, Tom Lane wrote:
> Aleksander Alekseev <[email protected]> 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 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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-11-15 18:09 ` Pavan Deolasee <[email protected]>
2024-11-15 18:35 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:40 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
2 siblings, 2 replies; 26+ messages in thread
From: Pavan Deolasee @ 2024-11-15 18:09 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>
On Fri, Nov 15, 2024 at 11:22 PM Noah Misch <[email protected]> wrote:
>
>
> > 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.
>
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:09 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
@ 2024-11-15 18:35 ` Noah Misch <[email protected]>
1 sibling, 0 replies; 26+ messages in thread
From: Noah Misch @ 2024-11-15 18:35 UTC (permalink / raw)
To: Pavan Deolasee <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>
On Fri, Nov 15, 2024 at 11:39:24PM +0530, Pavan Deolasee wrote:
> On Fri, Nov 15, 2024 at 11:22 PM Noah Misch <[email protected]> wrote:
> > > 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.
>
> 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.
I agree. I'm investigating the scope of damage from es_result_relations.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:09 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
@ 2024-11-15 18:40 ` Pavan Deolasee <[email protected]>
2024-11-15 19:03 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Pavan Deolasee @ 2024-11-15 18:40 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>
On Fri, Nov 15, 2024 at 11:39 PM Pavan Deolasee <[email protected]>
wrote:
>
> 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.
>
>
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:09 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
2024-11-15 18:40 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
@ 2024-11-15 19:03 ` Noah Misch <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Noah Misch @ 2024-11-15 19:03 UTC (permalink / raw)
To: Pavan Deolasee <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>
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 protected]> wrote:
> > 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.
> >
> 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
True.
> (PGD) got twice lucky :-) There could be other extensions which might be
> 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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-11-15 18:24 ` David E. Wheeler <[email protected]>
2 siblings, 0 replies; 26+ messages in thread
From: David E. Wheeler @ 2024-11-15 18:24 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Nov 15, 2024, at 12:52, Noah Misch <[email protected]> wrote:
> Either way, users of timescaledb should rebuild timescaledb for every future
> 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/
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-11-15 20:29 ` Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2 siblings, 2 replies; 26+ messages in thread
From: Tom Lane @ 2024-11-15 20:29 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Noah Misch <[email protected]> writes:
> On Fri, Nov 15, 2024 at 10:09:54AM -0500, Tom Lane wrote:
>> 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.
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.
> Either way, users of timescaledb should rebuild timescaledb for every future
> 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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-15 21:13 ` Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Tom Lane @ 2024-11-15 21:13 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-16 00:00 ` David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: David E. Wheeler @ 2024-11-16 00:00 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Nov 15, 2024, at 16:13, Tom Lane <[email protected]> wrote:
> 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.
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
@ 2024-11-16 00:30 ` Tom Lane <[email protected]>
2024-11-16 16:04 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
0 siblings, 2 replies; 26+ messages in thread
From: Tom Lane @ 2024-11-16 00:30 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
"David E. Wheeler" <[email protected]> writes:
> On Nov 15, 2024, at 16:13, Tom Lane <[email protected]> wrote:
>> 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.
> What bit is mis-worded? The guidance Peter committed[1] says that “PostgreSQL makes an effort to avoid server
> 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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-16 16:04 ` David E. Wheeler <[email protected]>
1 sibling, 0 replies; 26+ messages in thread
From: David E. Wheeler @ 2024-11-16 16:04 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Nov 15, 2024, at 19:30, Tom Lane <[email protected]> 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.
>
> 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”.
This sounds like complementary documentation for committers; totally agree the guidance should be written down somewhere.
D
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-26 01:56 ` Tom Lane <[email protected]>
2024-11-26 05:16 ` Re: Potential ABI breakage in upcoming minor releases Bertrand Drouvot <[email protected]>
2024-11-26 13:48 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-29 20:39 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
1 sibling, 3 replies; 26+ messages in thread
From: Tom Lane @ 2024-11-26 01:56 UTC (permalink / raw)
To: David E. Wheeler <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
[ 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.
* 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.
> 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".
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-26 05:16 ` Bertrand Drouvot <[email protected]>
2024-11-26 18:17 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2 siblings, 1 reply; 26+ messages in thread
From: Bertrand Drouvot @ 2024-11-26 05:16 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Hi,
On Mon, Nov 25, 2024 at 08:56:50PM -0500, Tom Lane 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...
>
> I propose rewriting and expanding that:
>
> * 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".)
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.inte...
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 05:16 ` Re: Potential ABI breakage in upcoming minor releases Bertrand Drouvot <[email protected]>
@ 2024-11-26 18:17 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Tom Lane @ 2024-11-26 18:17 UTC (permalink / raw)
To: Bertrand Drouvot <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Bertrand Drouvot <[email protected]> writes:
> On Mon, Nov 25, 2024 at 08:56:50PM -0500, Tom Lane wrote:
>> ... (But consider both 32-bit and 64-bit cases when
>> 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
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-26 13:48 ` David E. Wheeler <[email protected]>
2 siblings, 0 replies; 26+ messages in thread
From: David E. Wheeler @ 2024-11-26 13:48 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Nov 25, 2024, at 20:56, Tom Lane <[email protected]> wrote:
> None of this is a substitute for installing some kind of ABI-checking
> 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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-29 20:39 ` Noah Misch <[email protected]>
2024-12-01 16:13 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-12-04 19:29 ` Re: Potential ABI breakage in upcoming minor releases Peter Eisentraut <[email protected]>
2 siblings, 2 replies; 26+ messages in thread
From: Noah Misch @ 2024-11-29 20:39 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Mon, Nov 25, 2024 at 08:56:50PM -0500, Tom Lane 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...
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.
> 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.
Sounds fine.
> * 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.
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 protected]). 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?
> * 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.
Those two sound fine.
> > 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".
>
> 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.
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.
> None of this is a substitute for installing some kind of ABI-checking
> 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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-29 20:39 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-12-01 16:13 ` David E. Wheeler <[email protected]>
1 sibling, 0 replies; 26+ messages in thread
From: David E. Wheeler @ 2024-12-01 16:13 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Nov 29, 2024, at 15:39, Noah Misch <[email protected]> wrote:
> 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.
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-29 20:39 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-12-04 19:29 ` Peter Eisentraut <[email protected]>
2024-12-04 20:28 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Peter Eisentraut @ 2024-12-04 19:29 UTC (permalink / raw)
To: Noah Misch <[email protected]>; Tom Lane <[email protected]>; +Cc: David E. Wheeler <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On 29.11.24 21:39, Noah Misch wrote:
>> I remembered where that's documented:
>>
>> 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.
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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-29 20:39 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-12-04 19:29 ` Re: Potential ABI breakage in upcoming minor releases Peter Eisentraut <[email protected]>
@ 2024-12-04 20:28 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Tom Lane @ 2024-12-04 20:28 UTC (permalink / raw)
To: Peter Eisentraut <[email protected]>; +Cc: Noah Misch <[email protected]>; David E. Wheeler <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Peter Eisentraut <[email protected]> writes:
> On 29.11.24 21:39, Noah Misch wrote:
>> 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.
> 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.
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-15 22:27 ` Noah Misch <[email protected]>
2024-11-15 22:37 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
1 sibling, 1 reply; 26+ messages in thread
From: Noah Misch @ 2024-11-15 22:27 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Fri, Nov 15, 2024 at 03:29:21PM -0500, Tom Lane wrote:
> Noah Misch <[email protected]> writes:
> > On Fri, Nov 15, 2024 at 10:09:54AM -0500, Tom Lane wrote:
> >> 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.
>
> 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.
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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-11-15 22:37 ` Tom Lane <[email protected]>
2024-11-15 23:07 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Tom Lane @ 2024-11-15 22:37 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
Noah Misch <[email protected]> writes:
> 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.
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 22:37 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
@ 2024-11-15 23:07 ` Noah Misch <[email protected]>
2024-11-16 14:20 ` Re: Potential ABI breakage in upcoming minor releases Christoph Berg <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Noah Misch @ 2024-11-15 23:07 UTC (permalink / raw)
To: Tom Lane <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Christoph Berg <[email protected]>; Pavan Deolasee <[email protected]>
On Fri, Nov 15, 2024 at 05:37:01PM -0500, Tom Lane wrote:
> Noah Misch <[email protected]> writes:
> > 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.
>
> 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.
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.
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 22:37 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 23:07 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
@ 2024-11-16 14:20 ` Christoph Berg <[email protected]>
2024-11-16 18:00 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
0 siblings, 1 reply; 26+ messages in thread
From: Christoph Berg @ 2024-11-16 14:20 UTC (permalink / raw)
To: Noah Misch <[email protected]>; +Cc: Tom Lane <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Pavan Deolasee <[email protected]>
Re: Noah Misch
> I'm no longer arguing against the undo, but I'm trying to explain the
> consequences rigorously.
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
^ permalink raw reply [nested|flat] 26+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 22:37 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 23:07 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-16 14:20 ` Re: Potential ABI breakage in upcoming minor releases Christoph Berg <[email protected]>
@ 2024-11-16 18:00 ` Tom Lane <[email protected]>
0 siblings, 0 replies; 26+ messages in thread
From: Tom Lane @ 2024-11-16 18:00 UTC (permalink / raw)
To: Christoph Berg <[email protected]>; +Cc: Noah Misch <[email protected]>; Aleksander Alekseev <[email protected]>; pgsql-hackers; Marco Slot <[email protected]>; Alvaro Herrera <[email protected]>; Pavan Deolasee <[email protected]>
Christoph Berg <[email protected]> writes:
> Re: Noah Misch
>> I'm no longer arguing against the undo, but I'm trying to explain the
>> consequences rigorously.
> 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.
Fixes are pushed, and we'll wrap new releases next week on the
usual release schedule.
regards, tom lane
^ permalink raw reply [nested|flat] 26+ messages in thread
end of thread, other threads:[~2024-12-04 20:28 UTC | newest]
Thread overview: 26+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]>
2024-11-15 15:09 Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 17:52 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:09 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
2024-11-15 18:35 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:40 ` Re: Potential ABI breakage in upcoming minor releases Pavan Deolasee <[email protected]>
2024-11-15 19:03 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 18:24 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-15 20:29 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 21:13 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 00:00 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-16 00:30 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-16 16:04 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-26 01:56 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 05:16 ` Re: Potential ABI breakage in upcoming minor releases Bertrand Drouvot <[email protected]>
2024-11-26 18:17 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-26 13:48 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-11-29 20:39 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-12-01 16:13 ` Re: Potential ABI breakage in upcoming minor releases David E. Wheeler <[email protected]>
2024-12-04 19:29 ` Re: Potential ABI breakage in upcoming minor releases Peter Eisentraut <[email protected]>
2024-12-04 20:28 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 22:27 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-15 22:37 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
2024-11-15 23:07 ` Re: Potential ABI breakage in upcoming minor releases Noah Misch <[email protected]>
2024-11-16 14:20 ` Re: Potential ABI breakage in upcoming minor releases Christoph Berg <[email protected]>
2024-11-16 18:00 ` Re: Potential ABI breakage in upcoming minor releases Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox