public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3] Rework redundant loop in subtrans.c 34+ messages / 10 participants [nested] [flat]
* [PATCH v3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw) --- src/backend/access/transam/subtrans.c | 28 +++++---------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c index dc9566fb51..1455cdf54a 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,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID) nextXid = TransamVariables->nextXid; endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid)); - prevlock = SimpleLruGetBankLock(SubTransCtl, startPage); - LWLockAcquire(prevlock, LW_EXCLUSIVE); - while (startPage != endPage) + do { 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; } @@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID) /* must account for wraparound */ if (startPage > TransactionIdToPage(MaxTransactionId)) startPage = 0; - } + } while (startPage != endPage); - 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 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0003-Simplify-coding-in-slru.c.patch" ^ permalink raw reply [nested|flat] 34+ messages in thread
* [PATCH v3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw) --- src/backend/access/transam/subtrans.c | 28 +++++---------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c index dc9566fb51..1455cdf54a 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,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID) nextXid = TransamVariables->nextXid; endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid)); - prevlock = SimpleLruGetBankLock(SubTransCtl, startPage); - LWLockAcquire(prevlock, LW_EXCLUSIVE); - while (startPage != endPage) + do { 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; } @@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID) /* must account for wraparound */ if (startPage > TransactionIdToPage(MaxTransactionId)) startPage = 0; - } + } while (startPage != endPage); - 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 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0003-Simplify-coding-in-slru.c.patch" ^ permalink raw reply [nested|flat] 34+ messages in thread
* [PATCH v3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* [PATCH v2 2/3] Rework redundant loop in subtrans.c @ 2024-03-04 10:49 Alvaro Herrera <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Alvaro Herrera @ 2024-03-04 10:49 UTC (permalink / raw) --- src/backend/access/transam/subtrans.c | 28 +++++---------------------- 1 file changed, 5 insertions(+), 23 deletions(-) diff --git a/src/backend/access/transam/subtrans.c b/src/backend/access/transam/subtrans.c index dc9566fb51..1455cdf54a 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,19 +324,13 @@ StartupSUBTRANS(TransactionId oldestActiveXID) nextXid = TransamVariables->nextXid; endPage = TransactionIdToPage(XidFromFullTransactionId(nextXid)); - prevlock = SimpleLruGetBankLock(SubTransCtl, startPage); - LWLockAcquire(prevlock, LW_EXCLUSIVE); - while (startPage != endPage) + do { 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; } @@ -346,20 +340,8 @@ StartupSUBTRANS(TransactionId oldestActiveXID) /* must account for wraparound */ if (startPage > TransactionIdToPage(MaxTransactionId)) startPage = 0; - } + } while (startPage != endPage); - 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 --gflnge3qfnp343ni Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v2-0003-Simplify-coding-in-slru.c.patch" ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 15:09 Tom Lane <[email protected]> 0 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 17:52 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 3 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 18:09 Pavan Deolasee <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 2 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 18:24 David E. Wheeler <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 18:35 Noah Misch <[email protected]> parent: Pavan Deolasee <[email protected]> 1 sibling, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 18:40 Pavan Deolasee <[email protected]> parent: Pavan Deolasee <[email protected]> 1 sibling, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 19:03 Noah Misch <[email protected]> parent: Pavan Deolasee <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 20:29 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 2 siblings, 2 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 21:13 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 22:27 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 22:37 Tom Lane <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-15 23:07 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-16 00:00 David E. Wheeler <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-16 00:30 Tom Lane <[email protected]> parent: David E. Wheeler <[email protected]> 0 siblings, 2 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-16 14:20 Christoph Berg <[email protected]> parent: Noah Misch <[email protected]> 0 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-16 16:04 David E. Wheeler <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-16 18:00 Tom Lane <[email protected]> parent: Christoph Berg <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-26 01:56 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 1 sibling, 3 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-26 05:16 Bertrand Drouvot <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-26 13:48 David E. Wheeler <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-26 18:17 Tom Lane <[email protected]> parent: Bertrand Drouvot <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-11-29 20:39 Noah Misch <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 2 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-12-01 16:13 David E. Wheeler <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 0 replies; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-12-04 19:29 Peter Eisentraut <[email protected]> parent: Noah Misch <[email protected]> 1 sibling, 1 reply; 34+ 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] 34+ messages in thread
* Re: Potential ABI breakage in upcoming minor releases @ 2024-12-04 20:28 Tom Lane <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 0 replies; 34+ 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] 34+ messages in thread
* [PATCH] Expose checkpoint reason to completion log messages. @ 2025-12-01 11:18 Soumya S Murali <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Soumya S Murali @ 2025-12-01 11:18 UTC (permalink / raw) To: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi all, This patch is an update after reworking the “checkpoint reason” changes as a standalone patch, separate from the pg_stat_checkpointer additions as suggested [1]. I applied the patch on a clean tree and verified that the logging changes work as expected under different workloads. I am attaching the observations and patch in support for this.This would improve clarity for performance debugging and help understand checkpoint behavior without parsing WAL logs manually. Below is one representative checkpoint log entry after a pgbench run and an explicit CHECKPOINT: 2025-12-01 15:33:30.121 IST [69178] LOG: checkpoint complete (immediate): wrote 3417 buffers (20.9%), wrote 3 SLRU buffers; 0 WAL file(s) added, 0 removed, 1 recycled; write=0.122 s, sync=0.022 s, total=0.166 s; sync files=9, longest=0.005 s, average=0.003 s; distance=31304 kB, estimate=489729 kB; lsn=0/65E92BC8, redo lsn=0/65E92B70 Regarding the pg_stat_checkpointer extensions [1], I understand the concerns that were raised and I will follow up with a separate full patch once I incorporate the remaining feedback. Thank you for the guidance. It has been very helpful. Looking forward to more further feedback. Regards, Soumya Reference [1] https://www.postgresql.org/message-id/flat/CAMtXxw_W6w2Q1QsCvMPnoq3xCMKzH28Zjk_EmL60oP%2BsCTkXOw%40m... Attachments: [text/x-patch] 0001-Expose-checkpoint-reason-to-completion-log-messages.patch (3.4K, ../../CAMtXxw9tPwV=NBv5S9GZXMSKPeKv5f9hRhSjZ8__oLsoS5jcuA@mail.gmail.com/3-0001-Expose-checkpoint-reason-to-completion-log-messages.patch) download | inline diff: From 4045d7366171a1e512429ac8feb217d8a1199362 Mon Sep 17 00:00:00 2001 From: Soumya <[email protected]> Date: Mon, 1 Dec 2025 16:17:35 +0530 Subject: [PATCH] Expose checkpoint reason to completion log messages Signed-off-by: Soumya <[email protected]> --- src/backend/access/transam/xlog.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-) diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c index 22d0a2e8c3..f699c79c71 100644 --- a/src/backend/access/transam/xlog.c +++ b/src/backend/access/transam/xlog.c @@ -6750,7 +6750,7 @@ LogCheckpointStart(int flags, bool restartpoint) * Log end of a checkpoint. */ static void -LogCheckpointEnd(bool restartpoint) +LogCheckpointEnd(bool restartpoint, int flags) { long write_msecs, sync_msecs, @@ -6758,6 +6758,16 @@ LogCheckpointEnd(bool restartpoint) longest_msecs, average_msecs; uint64 average_sync_time; + const char *ckpt_reason = "timed"; + /* Determine checkpoint reason */ + if (flags & CHECKPOINT_IS_SHUTDOWN) + ckpt_reason = "shutdown"; + else if (flags & CHECKPOINT_END_OF_RECOVERY) + ckpt_reason = "end-of-recovery"; + else if (flags & CHECKPOINT_FAST) + ckpt_reason = "immediate"; + else if (flags & CHECKPOINT_FORCE) + ckpt_reason = "forced"; CheckpointStats.ckpt_end_t = GetCurrentTimestamp(); @@ -6800,12 +6810,13 @@ LogCheckpointEnd(bool restartpoint) */ if (restartpoint) ereport(LOG, - (errmsg("restartpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("restartpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + ckpt_reason, CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -6824,12 +6835,13 @@ LogCheckpointEnd(bool restartpoint) LSN_FORMAT_ARGS(ControlFile->checkPointCopy.redo)))); else ereport(LOG, - (errmsg("checkpoint complete: wrote %d buffers (%.1f%%), " + (errmsg("checkpoint complete (%s): wrote %d buffers (%.1f%%), " "wrote %d SLRU buffers; %d WAL file(s) added, " "%d removed, %d recycled; write=%ld.%03d s, " "sync=%ld.%03d s, total=%ld.%03d s; sync files=%d, " "longest=%ld.%03d s, average=%ld.%03d s; distance=%d kB, " "estimate=%d kB; lsn=%X/%08X, redo lsn=%X/%08X", + ckpt_reason, CheckpointStats.ckpt_bufs_written, (double) CheckpointStats.ckpt_bufs_written * 100 / NBuffers, CheckpointStats.ckpt_slru_written, @@ -7418,7 +7430,7 @@ CreateCheckPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(false); + LogCheckpointEnd(false, flags); /* Reset the process title */ update_checkpoint_display(flags, false, true); @@ -7886,7 +7898,7 @@ CreateRestartPoint(int flags) TruncateSUBTRANS(GetOldestTransactionIdConsideredRunning()); /* Real work is done; log and update stats. */ - LogCheckpointEnd(true); + LogCheckpointEnd(true, flags); /* Reset the process title */ update_checkpoint_display(flags, true, true); -- 2.34.1 ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2025-12-18 00:19 Andres Freund <[email protected]> parent: Soumya S Murali <[email protected]> 0 siblings, 1 reply; 34+ messages in thread From: Andres Freund @ 2025-12-18 00:19 UTC (permalink / raw) To: Soumya S Murali <[email protected]>; +Cc: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi, On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote: > This patch is an update after reworking the “checkpoint reason” changes as > a standalone patch, separate from the pg_stat_checkpointer additions as > suggested [1]. This seems to not pass the tests on any platform: https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306 Looks like you need to update 019_replslot_limit.pl for the changed log format. I suggest running the tests before submitting. Greetings, Andres ^ permalink raw reply [nested|flat] 34+ messages in thread
* Re: [PATCH] Expose checkpoint reason to completion log messages. @ 2025-12-18 04:46 Soumya S Murali <[email protected]> parent: Andres Freund <[email protected]> 0 siblings, 0 replies; 34+ messages in thread From: Soumya S Murali @ 2025-12-18 04:46 UTC (permalink / raw) To: Andres Freund <[email protected]>; +Cc: Michael Banck <[email protected]>; PostgreSQL Hackers <[email protected]>; Álvaro Herrera <[email protected]>; [email protected]; [email protected] Hi Andres, On Thu, Dec 18, 2025 at 5:49 AM Andres Freund <[email protected]> wrote: > > Hi, > > On 2025-12-01 16:48:56 +0530, Soumya S Murali wrote: > > This patch is an update after reworking the “checkpoint reason” changes as > > a standalone patch, separate from the pg_stat_checkpointer additions as > > suggested [1]. > > This seems to not pass the tests on any platform: > https://cirrus-ci.com/github/postgresql-cfbot/postgresql/cf%2F6306 > > Looks like you need to update 019_replslot_limit.pl for the changed log > format. > > I suggest running the tests before submitting. > > Greetings, > > Andres Thank you for checking and for pointing this out. I will fix the expected log pattern, rerun the test, and will resend an updated patch once all tests get passed. Regards, Soumya ^ permalink raw reply [nested|flat] 34+ messages in thread
end of thread, other threads:[~2025-12-18 04:46 UTC | newest] Thread overview: 34+ 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-03-04 10:49 [PATCH v2 2/3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]> 2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]> 2024-03-04 10:49 [PATCH v2 2/3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]> 2024-03-04 10:49 [PATCH v3] Rework redundant loop in subtrans.c Alvaro Herrera <[email protected]> 2024-03-04 10:49 [PATCH v2 2/3] 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]> 2025-12-01 11:18 [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[email protected]> 2025-12-18 00:19 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Andres Freund <[email protected]> 2025-12-18 04:46 ` Re: [PATCH] Expose checkpoint reason to completion log messages. Soumya S Murali <[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