public inbox for [email protected]help / color / mirror / Atom feed
[PATCH v3 12/17] Merge prune and freeze records 4+ messages / 3 participants [nested] [flat]
* [PATCH v3 12/17] Merge prune and freeze records @ 2024-01-07 22:55 Melanie Plageman <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Melanie Plageman @ 2024-01-07 22:55 UTC (permalink / raw) When there are both tuples to prune and freeze on a page, emit a single, combined prune record containing the offsets for pruning and the freeze plans and offsets for freezing. This will reduce the number of WAL records emitted. --- src/backend/access/heap/heapam.c | 42 ++++++++++++-- src/backend/access/heap/pruneheap.c | 85 +++++++++++++---------------- src/include/access/heapam_xlog.h | 20 +++++-- 3 files changed, 90 insertions(+), 57 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index a3691584c55..a8f35eba3c9 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -8803,24 +8803,28 @@ heap_xlog_prune(XLogReaderState *record) if (action == BLK_NEEDS_REDO) { Page page = (Page) BufferGetPage(buffer); - OffsetNumber *end; OffsetNumber *redirected; OffsetNumber *nowdead; OffsetNumber *nowunused; int nredirected; int ndead; int nunused; + int nplans; Size datalen; + xl_heap_freeze_plan *plans; + OffsetNumber *frz_offsets; + int curoff = 0; - redirected = (OffsetNumber *) XLogRecGetBlockData(record, 0, &datalen); - + nplans = xlrec->nplans; nredirected = xlrec->nredirected; ndead = xlrec->ndead; - end = (OffsetNumber *) ((char *) redirected + datalen); + nunused = xlrec->nunused; + + plans = (xl_heap_freeze_plan *) XLogRecGetBlockData(record, 0, &datalen); + redirected = (OffsetNumber *) &plans[nplans]; nowdead = redirected + (nredirected * 2); nowunused = nowdead + ndead; - nunused = (end - nowunused); - Assert(nunused >= 0); + frz_offsets = nowunused + nunused; /* Update all line pointers per the record, and repair fragmentation */ heap_page_prune_execute(buffer, @@ -8828,6 +8832,32 @@ heap_xlog_prune(XLogReaderState *record) nowdead, ndead, nowunused, nunused); + for (int p = 0; p < nplans; p++) + { + HeapTupleFreeze frz; + + /* + * Convert freeze plan representation from WAL record into + * per-tuple format used by heap_execute_freeze_tuple + */ + frz.xmax = plans[p].xmax; + frz.t_infomask2 = plans[p].t_infomask2; + frz.t_infomask = plans[p].t_infomask; + frz.frzflags = plans[p].frzflags; + frz.offset = InvalidOffsetNumber; /* unused, but be tidy */ + + for (int i = 0; i < plans[p].ntuples; i++) + { + OffsetNumber offset = frz_offsets[curoff++]; + ItemId lp; + HeapTupleHeader tuple; + + lp = PageGetItemId(page, offset); + tuple = (HeapTupleHeader) PageGetItem(page, lp); + heap_execute_freeze_tuple(tuple, &frz); + } + } + /* * Note: we don't worry about updating the page's prunability hints. * At worst this will cause an extra prune cycle to occur soon. diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index d77270ad0d6..994cf75c54e 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -619,6 +619,9 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, */ PageClearFull(page); + if (do_freeze) + heap_freeze_prepared_tuples(buffer, frozen, presult->nfrozen); + MarkBufferDirty(buffer); /* @@ -629,10 +632,37 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, xl_heap_prune xlrec; XLogRecPtr recptr; + xl_heap_freeze_plan plans[MaxHeapTuplesPerPage]; + OffsetNumber offsets[MaxHeapTuplesPerPage]; + xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation); - xlrec.snapshotConflictHorizon = prstate.snapshotConflictHorizon; xlrec.nredirected = prstate.nredirected; xlrec.ndead = prstate.ndead; + xlrec.nunused = prstate.nunused; + xlrec.nplans = 0; + + /* + * The snapshotConflictHorizon for the whole record should be the most + * conservative of all the horizons calculated for any of the possible + * modifications. If this record will prune tuples, any transactions + * on the standby older than the youngest xmax of the most recently + * removed tuple this record will prune will conflict. If this record + * will freeze tuples, any transactions on the standby with xids older + * than the youngest tuple this record will freeze will conflict. + */ + if (do_freeze) + xlrec.snapshotConflictHorizon = Max(prstate.snapshotConflictHorizon, + frz_conflict_horizon); + else + xlrec.snapshotConflictHorizon = prstate.snapshotConflictHorizon; + + /* + * Prepare deduplicated representation for use in WAL record + * Destructively sorts tuples array in-place. + */ + if (do_freeze) + xlrec.nplans = heap_log_freeze_plan(frozen, + presult->nfrozen, plans, offsets); XLogBeginInsert(); XLogRegisterData((char *) &xlrec, SizeOfHeapPrune); @@ -644,6 +674,10 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, * pretend that they are. When XLogInsert stores the whole buffer, * the offset arrays need not be stored too. */ + if (xlrec.nplans > 0) + XLogRegisterBufData(0, (char *) plans, + xlrec.nplans * sizeof(xl_heap_freeze_plan)); + if (prstate.nredirected > 0) XLogRegisterBufData(0, (char *) prstate.redirected, prstate.nredirected * @@ -657,56 +691,13 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, XLogRegisterBufData(0, (char *) prstate.nowunused, prstate.nunused * sizeof(OffsetNumber)); - recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_PRUNE); - - PageSetLSN(BufferGetPage(buffer), recptr); - } - - if (do_freeze) - { - Assert(presult->nfrozen > 0); - - heap_freeze_prepared_tuples(buffer, frozen, presult->nfrozen); - - MarkBufferDirty(buffer); - - /* Now WAL-log freezing if necessary */ - if (RelationNeedsWAL(relation)) - { - xl_heap_freeze_plan plans[MaxHeapTuplesPerPage]; - OffsetNumber offsets[MaxHeapTuplesPerPage]; - int nplans; - xl_heap_freeze_page xlrec; - XLogRecPtr recptr; - - /* - * Prepare deduplicated representation for use in WAL record - * Destructively sorts tuples array in-place. - */ - nplans = heap_log_freeze_plan(frozen, presult->nfrozen, plans, offsets); - - xlrec.snapshotConflictHorizon = frz_conflict_horizon; - xlrec.isCatalogRel = RelationIsAccessibleInLogicalDecoding(relation); - xlrec.nplans = nplans; - - XLogBeginInsert(); - XLogRegisterData((char *) &xlrec, SizeOfHeapFreezePage); - - /* - * The freeze plan array and offset array are not actually in the - * buffer, but pretend that they are. When XLogInsert stores the - * whole buffer, the arrays need not be stored too. - */ - XLogRegisterBuffer(0, buffer, REGBUF_STANDARD); - XLogRegisterBufData(0, (char *) plans, - nplans * sizeof(xl_heap_freeze_plan)); + if (xlrec.nplans > 0) XLogRegisterBufData(0, (char *) offsets, presult->nfrozen * sizeof(OffsetNumber)); - recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_FREEZE_PAGE); + recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_PRUNE); - PageSetLSN(page, recptr); - } + PageSetLSN(BufferGetPage(buffer), recptr); } END_CRIT_SECTION(); diff --git a/src/include/access/heapam_xlog.h b/src/include/access/heapam_xlog.h index 6488dad5e64..22f236bb52a 100644 --- a/src/include/access/heapam_xlog.h +++ b/src/include/access/heapam_xlog.h @@ -231,23 +231,35 @@ typedef struct xl_heap_update * during opportunistic pruning) * * The array of OffsetNumbers following the fixed part of the record contains: + * * for each freeze plan: the freeze plan * * for each redirected item: the item offset, then the offset redirected to * * for each now-dead item: the item offset * * for each now-unused item: the item offset - * The total number of OffsetNumbers is therefore 2*nredirected+ndead+nunused. - * Note that nunused is not explicitly stored, but may be found by reference - * to the total record length. + * * for each tuple frozen by the freeze plans: the offset of the item corresponding to that tuple + * The total number of OffsetNumbers is therefore + * (2*nredirected) + ndead + nunused + (sum[plan.ntuples for plan in plans]) * * Acquires a full cleanup lock. */ typedef struct xl_heap_prune { TransactionId snapshotConflictHorizon; + uint16 nplans; uint16 nredirected; uint16 ndead; + uint16 nunused; bool isCatalogRel; /* to handle recovery conflict during logical * decoding on standby */ - /* OFFSET NUMBERS are in the block reference 0 */ + /* + * OFFSET NUMBERS and freeze plans are in the block reference 0 in the + * following order: + * + * * xl_heap_freeze_plan plans[nplans]; + * * OffsetNumber redirected[2 * nredirected]; + * * OffsetNumber nowdead[ndead]; + * * OffsetNumber nowunused[nunused]; + * * OffsetNumber frz_offsets[...]; + */ } xl_heap_prune; #define SizeOfHeapPrune (offsetof(xl_heap_prune, isCatalogRel) + sizeof(bool)) -- 2.40.1 --racicctn4wry6xe5 Content-Type: text/x-diff; charset=us-ascii Content-Disposition: attachment; filename="v3-0013-Set-hastup-in-heap_page_prune.patch" ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BF mamba failure @ 2024-10-17 04:24 Michael Paquier <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michael Paquier @ 2024-10-17 04:24 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected] On Wed, Oct 16, 2024 at 10:11:08AM +0000, Bertrand Drouvot wrote: > Indeed, in pgstat_release_entry_ref(), we're doing: > > if (pg_atomic_fetch_sub_u32(&entry_ref->shared_entry->refcount, 1) == 1) > . > . > shent = dshash_find(pgStatLocal.shared_hash, > &entry_ref->shared_entry->key, > true); > > I wonder if we are not decrementing &entry_ref->shared_entry->refcount too early. > > I mean, wouldn't that make sense to decrement it after the dshash_find() call? > (to ensure a "proper" whole entry locking, done in dshash_find(), first) Making that a two-step process (first step to read the refcount, second step to decrement it after taking the exclusive lock through dshash_find) would make the logic more complicated what what we have now, without offering more protection, afaik, because you'd still need to worry about a race condition between the first and second steps. Making this a one-step only would incur more dshash_find() calls than we actually need, and I would not underestimate that under a heavily-concurrent pgstat_release_entry_ref() path taken. > Yeah, FWIW, we would be going from 32 bytes: > /* total size (bytes): 32 */ > > to 40 bytes (with the patch applied): > /* total size (bytes): 40 */ > > Due to the padding, that's a 8 bytes increase while we're adding "only" 4 bytes. I have spent some time digging into all the original pgstat threads dealing with the shmem implementation or dshash, and I'm not really seeing anybody stressing on the size of the individual hash entries. This stuff was already wasting space with padding originally, so perhaps we're stressing too much here? Would anybody like to comment? -- Michael Attachments: [application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BF mamba failure @ 2024-11-14 07:55 Michael Paquier <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 1 reply; 4+ messages in thread From: Michael Paquier @ 2024-11-14 07:55 UTC (permalink / raw) To: Bertrand Drouvot <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected] On Thu, Oct 17, 2024 at 01:24:50PM +0900, Michael Paquier wrote: >> Yeah, FWIW, we would be going from 32 bytes: >> /* total size (bytes): 32 */ >> >> to 40 bytes (with the patch applied): >> /* total size (bytes): 40 */ >> >> Due to the padding, that's a 8 bytes increase while we're adding "only" 4 bytes. > > I have spent some time digging into all the original pgstat threads > dealing with the shmem implementation or dshash, and I'm not really > seeing anybody stressing on the size of the individual hash entries. > This stuff was already wasting space with padding originally, so > perhaps we're stressing too much here? Would anybody like to comment? I've been going through this patch again, and the failures that could be seen because of such failures, like standbys failing in an unpredictible way is not cool, so I am planning to apply the attached down to 15 now that the release has colled down. At the end I am not really stressing about this addition in the structure for the sake of making the stats entries safe to handle. I did not like much the name "agecount" though, used to cross-check how many times an entry is reused in shared memory and in the local copy we keep in a process, so I've renamed it to "generation". -- Michael Attachments: [text/x-diff] v2-0001-Introduce-generation-counter-for-pgstats-entries.patch (5.0K, ../../[email protected]/2-v2-0001-Introduce-generation-counter-for-pgstats-entries.patch) download | inline diff: From abd8b567f3cd97ca4e0282ce7657f64807f8c008 Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Thu, 14 Nov 2024 16:53:58 +0900 Subject: [PATCH v2] Introduce "generation" counter for pgstats entries This fixes a set of race conditions with cumulative statistics where a shared stats entry could be dropped while it should still be valid when it gets reused. This can happen for the following case, causing the same hash key to be used: - replication slots that compute internally an index number. - other object types, with a wraparound involved. - As of PostgreSQL 18, custom pgstats kinds. The "generation" is tracked in the shared entries, initialized at 0 when an entry is created and incremented when the same entry is reused, to avoid concurrent turbulences on drop because of other backends still holding a reference to it. --- src/include/utils/pgstat_internal.h | 14 ++++++++ src/backend/utils/activity/pgstat_shmem.c | 40 ++++++++++++++++++++--- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/include/utils/pgstat_internal.h b/src/include/utils/pgstat_internal.h index 61b2e1f96b..45f5901d45 100644 --- a/src/include/utils/pgstat_internal.h +++ b/src/include/utils/pgstat_internal.h @@ -94,6 +94,14 @@ typedef struct PgStatShared_HashEntry */ pg_atomic_uint32 refcount; + /* + * Counter tracking the number of times the entry has been reused. + * + * Set to 0 when the entry is created, and incremented by one each time + * the shared entry is reinitialized with pgstat_reinit_entry(). + */ + pg_atomic_uint32 generation; + /* * Pointer to shared stats. The stats entry always starts with * PgStatShared_Common, embedded in a larger struct containing the @@ -133,6 +141,12 @@ typedef struct PgStat_EntryRef */ PgStatShared_Common *shared_stats; + /* + * Copy of PgStatShared_HashEntry->generation, keeping locally track of + * the shared stats entry "generation" retrieved (number of times reused). + */ + uint32 generation; + /* * Pending statistics data that will need to be flushed to shared memory * stats eventually. Each stats kind utilizing pending data defines what diff --git a/src/backend/utils/activity/pgstat_shmem.c b/src/backend/utils/activity/pgstat_shmem.c index c1b7ff76b1..c8ee0bd0b0 100644 --- a/src/backend/utils/activity/pgstat_shmem.c +++ b/src/backend/utils/activity/pgstat_shmem.c @@ -304,6 +304,11 @@ pgstat_init_entry(PgStat_Kind kind, * further if a longer lived reference is needed. */ pg_atomic_init_u32(&shhashent->refcount, 1); + + /* + * Initialize generation to 0, as freshly initialized. + */ + pg_atomic_init_u32(&shhashent->generation, 0); shhashent->dropped = false; chunk = dsa_allocate0(pgStatLocal.dsa, pgstat_get_kind_info(kind)->shared_size); @@ -327,6 +332,12 @@ pgstat_reinit_entry(PgStat_Kind kind, PgStatShared_HashEntry *shhashent) /* mark as not dropped anymore */ pg_atomic_fetch_add_u32(&shhashent->refcount, 1); + + /* + * Increment generation, to let any backend with local references know + * that what they point to is outdated. + */ + pg_atomic_fetch_add_u32(&shhashent->generation, 1); shhashent->dropped = false; /* reinitialize content */ @@ -367,6 +378,7 @@ pgstat_acquire_entry_ref(PgStat_EntryRef *entry_ref, entry_ref->shared_stats = shheader; entry_ref->shared_entry = shhashent; + entry_ref->generation = pg_atomic_read_u32(&shhashent->generation); } /* @@ -532,7 +544,8 @@ pgstat_get_entry_ref(PgStat_Kind kind, Oid dboid, uint64 objid, bool create, * case are replication slot stats, where a new slot can be * created with the same index just after dropping. But oid * wraparound can lead to other cases as well. We just reset the - * stats to their plain state. + * stats to their plain state, while incrementing its "generation" + * in the shared entry for any remaining local references. */ shheader = pgstat_reinit_entry(kind, shhashent); pgstat_acquire_entry_ref(entry_ref, shhashent, shheader); @@ -599,10 +612,27 @@ pgstat_release_entry_ref(PgStat_HashKey key, PgStat_EntryRef *entry_ref, if (!shent) elog(ERROR, "could not find just referenced shared stats entry"); - Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0); - Assert(entry_ref->shared_entry == shent); - - pgstat_free_entry(shent, NULL); + /* + * This entry may have been reinitialized while trying to release + * it, so double-check that it has not been reused while holding a + * lock on its shared entry. + */ + if (pg_atomic_read_u32(&entry_ref->shared_entry->generation) == + entry_ref->generation) + { + /* Same generation, so we're OK with the removal */ + Assert(pg_atomic_read_u32(&entry_ref->shared_entry->refcount) == 0); + Assert(entry_ref->shared_entry == shent); + pgstat_free_entry(shent, NULL); + } + else + { + /* + * Shared stats entry has been reinitialized, so do not drop + * its shared entry, only release its lock. + */ + dshash_release_lock(pgStatLocal.shared_hash, shent); + } } } -- 2.45.2 [application/pgp-signature] signature.asc (833B, ../../[email protected]/3-signature.asc) download ^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: BF mamba failure @ 2024-11-14 10:07 Bertrand Drouvot <[email protected]> parent: Michael Paquier <[email protected]> 0 siblings, 0 replies; 4+ messages in thread From: Bertrand Drouvot @ 2024-11-14 10:07 UTC (permalink / raw) To: Michael Paquier <[email protected]>; +Cc: Kouber Saparev <[email protected]>; [email protected] Hi, On Thu, Nov 14, 2024 at 04:55:23PM +0900, Michael Paquier wrote: > I've been going through this patch again, and the failures that could > be seen because of such failures, like standbys failing in an > unpredictible way is not cool, so I am planning to apply the attached > down to 15 now that the release has colled down. At the end I am not > really stressing about this addition in the structure for the sake of > making the stats entries safe to handle. I don't think the addition is a concern too. > I did not like much the name "agecount" though, used to cross-check > how many times an entry is reused in shared memory and in the local > copy we keep in a process, so I've renamed it to "generation". "generation" sounds more appropriate to me too. The approach to avoid the error sounds reasonable to me. Just 2 comments about the patch: === 1 Maybe use "generation" instead of generation in the comments (where it's not done, some comments do it already). === 2 We could think about adding a test. That should be doable with replication slots or custom pgstats kinds and probably injection points. But I'm not sure that's worth it, as the code in the patch looks "simple" enough. Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com ^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2024-11-14 10:07 UTC | newest] Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-01-07 22:55 [PATCH v3 12/17] Merge prune and freeze records Melanie Plageman <[email protected]> 2024-10-17 04:24 Re: BF mamba failure Michael Paquier <[email protected]> 2024-11-14 07:55 ` Re: BF mamba failure Michael Paquier <[email protected]> 2024-11-14 10:07 ` Re: BF mamba failure Bertrand Drouvot <[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