From 704336b3d2177c3695ebd49a36d26014c6362636 Mon Sep 17 00:00:00 2001 From: Alexandre Felipe Date: Sun, 16 Aug 2026 06:43:51 +0100 Subject: [PATCH-v2 3/3] Mutable links This patch implements mutable links to a buffer table link and search by Buffer instead of BufferTag. The previous patch was subject to concurrent reinsertion on a different hash partition breaking bucket chains. An initial version was calling BufTableDelete under a Buffer header lock, and reviewers deemed that inappropriate as spinlocks have no error recovery, and an error thrown while holding a spin-lock could make the buffer permanently unusable. Methods BufTableLinkByTag and BufTableLinkByBuffer identify the links in the hash using BufferTag or Buffer, respectively. The method BufTableUnlink(BufferTableLink *) enables removal of an entry form the hash in a few instruction. bufmgr buffer invalidation routines were updated to make sure the buffer is not reused before removal from BufTable is finished i.e. Lock(partition) { get link to BufTable entry Lock(Buffer Header) { abort if tag changed clear tag Unlink buffer from BufTable } } The victim buffer invalidation is protected by holding a pin to the buffer. This way the buffer remain protected after tag is cleared until the deletion from BufTable chain is finished. --- src/backend/storage/buffer/buf_table.c | 132 ++++++++++++------ src/backend/storage/buffer/bufmgr.c | 29 ++-- src/include/storage/buf_internals.h | 21 +++ .../modules/buftable_bench/buftable_bench.c | 14 ++ 4 files changed, 141 insertions(+), 55 deletions(-) diff --git a/src/backend/storage/buffer/buf_table.c b/src/backend/storage/buffer/buf_table.c index 7836d0dd664..508a4878a88 100644 --- a/src/backend/storage/buffer/buf_table.c +++ b/src/backend/storage/buffer/buf_table.c @@ -165,6 +165,34 @@ BufTableHashCode(BufferTag *tagPtr) return tag_hash(tagPtr, sizeof(BufferTag)); } +bool +BufTableLinkByTag(BufferTableLink * link, BufferTag *tagPtr, uint32 hashcode) +{ + link->head = &buckets[hashcode & (num_buckets - 1)].head; + link->buf_id = link->head; + while (*link->buf_id != BUF_TABLE_CHAIN_END) + { + if (BufferTagsEqual(&entries[*link->buf_id].tag, tagPtr)) + return true; + link->buf_id = &entries[*link->buf_id].next; + } + return false; +} + +bool +BufTableLinkByBuffer(BufferTableLink * link, Buffer buf, uint32 hashcode) +{ + link->head = &buckets[hashcode & (num_buckets - 1)].head; + link->buf_id = link->head; + while (*link->buf_id != BUF_TABLE_CHAIN_END) + { + if (*link->buf_id == buf) + return true; + link->buf_id = &entries[*link->buf_id].next; + } + return false; +} + /* * BufTableLookup * Lookup the given BufferTag; return buffer ID, or -1 if not found @@ -174,14 +202,10 @@ BufTableHashCode(BufferTag *tagPtr) int BufTableLookup(BufferTag *tagPtr, uint32 hashcode) { - int id = buckets[hashcode % num_buckets].head; + BufferTableLink link; - while (id != BUF_TABLE_CHAIN_END) - { - if (BufferTagsEqual(&entries[id].tag, tagPtr)) - return id; - id = entries[id].next; - } + if (BufTableLinkByTag(&link, tagPtr, hashcode)) + return *link.buf_id; return -1; } @@ -198,20 +222,14 @@ BufTableLookup(BufferTag *tagPtr, uint32 hashcode) int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id) { - int bucket_id = hashcode % num_buckets; - int head = buckets[bucket_id].head; - int id = head; + BufferTableLink link; Assert(buf_id >= 0 && buf_id < NBuffers); Assert(tagPtr->blockNum != P_NEW); /* invalid tag */ /* If the tag is already in the chain, surface the existing buf_id. */ - while (id != BUF_TABLE_CHAIN_END) - { - if (BufferTagsEqual(&entries[id].tag, tagPtr)) - return id; - id = entries[id].next; - } + if (BufTableLinkByTag(&link, tagPtr, hashcode)) + return *link.buf_id; /* * Not present. entry[buf_id] must be empty: bufmgr always deletes a @@ -221,16 +239,30 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id) /* * Link entry[buf_id] at the chain head, keeping the prior head as its - * successor. (Use the saved `head`, not `id`, which the loop above has - * advanced to BUF_TABLE_CHAIN_END.) + * successor. (Use the saved head, not buf_id on the link, which the walk + * has advanced to the chain terminator.) */ entries[buf_id].tag = *tagPtr; - entries[buf_id].next = head; - buckets[bucket_id].head = buf_id; - + entries[buf_id].next = *link.head; + *link.head = buf_id; return -1; } +/* + * BufTableUnlink + * Delete the hashtable entry using a BufferTableLink + */ +void +BufTableUnlink(BufferTableLink * link) +{ + int id = *link->buf_id; + + *link->buf_id = entries[id].next; + entries[id].tag.blockNum = P_NEW; + entries[id].next = BUF_TABLE_CHAIN_END; +} + + /* * BufTableDelete * Delete the hashtable entry for given tag (which must exist) @@ -240,32 +272,42 @@ BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id) void BufTableDelete(BufferTag *tagPtr, uint32 hashcode) { - int bucket_id = hashcode % num_buckets; - int prev = BUF_TABLE_CHAIN_END; - int id = buckets[bucket_id].head; + BufferTableLink link; - while (id != BUF_TABLE_CHAIN_END) + if (!BufTableLinkByTag(&link, tagPtr, hashcode)) { - if (BufferTagsEqual(&entries[id].tag, tagPtr)) - { - /* unlink from the chain */ - if (prev == BUF_TABLE_CHAIN_END) - buckets[bucket_id].head = entries[id].next; - else - entries[prev].next = entries[id].next; - /* mark the entry empty */ - entries[id].tag.blockNum = P_NEW; - entries[id].next = BUF_TABLE_CHAIN_END; - return; - } - prev = id; - id = entries[id].next; + /* + * Entry not in table. Callers never double-delete (deletion is gated + * by BM_TAG_VALID on the buffer header), so this indicates + * corruption. + */ + Assert(false); + elog(ERROR, "shared buffer hash table corrupted"); } + BufTableUnlink(&link); +} - /* - * Entry not in table. Callers never double-delete (deletion is gated by - * BM_TAG_VALID on the buffer header), so this indicates corruption. - */ - Assert(false); - elog(ERROR, "shared buffer hash table corrupted"); + +/* + * BufTableDeleteBuffer + * Delete the hashtable entry for given buffer (which must exist) + * + * Caller must hold exclusive lock on BufMappingLock for buffer's partition + */ +void +BufTableDeleteBuffer(Buffer buf, uint32 hashcode) +{ + BufferTableLink link; + + if (!BufTableLinkByBuffer(&link, buf, hashcode)) + { + /* + * Entry not in table. Callers never double-delete (deletion is gated + * by BM_TAG_VALID on the buffer header), so this indicates + * corruption. + */ + Assert(false); + elog(ERROR, "shared buffer hash table corrupted"); + } + BufTableUnlink(&link); } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 169829eb020..64c4b197bbe 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2374,10 +2374,13 @@ InvalidateBuffer(BufferDesc *buf) LWLock *oldPartitionLock; /* buffer partition lock for it */ uint32 oldFlags; uint64 buf_state; + Buffer buf_id; + BufferTableLink buf_link; + /* Save the original buffer tag before dropping the spinlock */ oldTag = buf->tag; - + buf_id = buf->buf_id; UnlockBufHdr(buf); /* @@ -2395,6 +2398,7 @@ retry: * association. */ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE); + BufTableLinkByBuffer(&buf_link, buf_id, oldHash); /* Re-lock the buffer header */ buf_state = LockBufHdr(buf); @@ -2441,16 +2445,17 @@ retry: oldFlags = buf_state & BUF_FLAG_MASK; ClearBufferTag(&buf->tag); - UnlockBufHdrExt(buf, buf_state, - 0, - BUF_FLAG_MASK | BUF_USAGECOUNT_MASK, - 0); /* * Remove the buffer from the lookup hashtable, if it was in there. */ if (oldFlags & BM_TAG_VALID) - BufTableDelete(&oldTag, oldHash); + BufTableUnlink(&buf_link); + + UnlockBufHdrExt(buf, buf_state, + 0, + BUF_FLAG_MASK | BUF_USAGECOUNT_MASK, + 0); /* * Done with mapping lock. @@ -2474,6 +2479,7 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr) uint32 hash; LWLock *partition_lock; BufferTag tag; + BufferTableLink buf_link; Assert(GetPrivateRefCount(BufferDescriptorGetBuffer(buf_hdr)) == 1); @@ -2484,6 +2490,7 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr) partition_lock = BufMappingPartitionLock(hash); LWLockAcquire(partition_lock, LW_EXCLUSIVE); + BufTableLinkByBuffer(&buf_link, buf_hdr->buf_id, hash); /* lock the buffer header */ buf_state = LockBufHdr(buf_hdr); @@ -2524,15 +2531,17 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr) * tag (see e.g. FlushDatabaseBuffers()). */ ClearBufferTag(&buf_hdr->tag); + + Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0); + + /* finally delete buffer from the buffer mapping table */ + BufTableUnlink(&buf_link); + UnlockBufHdrExt(buf_hdr, buf_state, 0, BUF_FLAG_MASK | BUF_USAGECOUNT_MASK, 0); - Assert(BUF_STATE_GET_REFCOUNT(buf_state) > 0); - - /* finally delete buffer from the buffer mapping table */ - BufTableDelete(&tag, hash); LWLockRelease(partition_lock); diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index e4ff5619b79..6093ee782ea 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -167,6 +167,17 @@ typedef struct buftag BlockNumber blockNum; /* blknum relative to begin of reln */ } BufferTag; + +/* + * A mutable chain link: buf_id points at the int that currently stores the + * entry index (either a bucket head or a predecessor's next field). + */ +typedef struct +{ + int *buf_id; /* link to to that leads to an existing buffer */ + int *head; /* bucket chain head, for insert-at-front */ +} BufferTableLink; + static inline RelFileNumber BufTagGetRelNumber(const BufferTag *tag) { @@ -599,6 +610,16 @@ extern uint32 BufTableHashCode(BufferTag *tagPtr); extern int BufTableLookup(BufferTag *tagPtr, uint32 hashcode); extern int BufTableInsert(BufferTag *tagPtr, uint32 hashcode, int buf_id); extern void BufTableDelete(BufferTag *tagPtr, uint32 hashcode); +extern void BufTableDeleteBuffer(Buffer buf, uint32 hashcode); + +/* Populate a BufferTableLink comparing a BufferTag */ +extern bool BufTableLinkByTag(BufferTableLink * link, BufferTag *tagPtr, uint32 hashcode); + +/* Populate a BufferTableLink comparing buffer id */ +extern bool BufTableLinkByBuffer(BufferTableLink * link, Buffer buf, uint32 hashcode); + +/* Quick removal of a buffer entry using a BufferTableLink */ +extern void BufTableUnlink(BufferTableLink * link); /* localbuf.c */ extern bool PinLocalBuffer(BufferDesc *buf_hdr, bool adjust_usagecount); diff --git a/src/test/modules/buftable_bench/buftable_bench.c b/src/test/modules/buftable_bench/buftable_bench.c index 500400ad589..e47f1a507a6 100644 --- a/src/test/modules/buftable_bench/buftable_bench.c +++ b/src/test/modules/buftable_bench/buftable_bench.c @@ -196,6 +196,20 @@ buftable_bench_probe(PG_FUNCTION_ARGS) } END_TIMING; + BEGIN_TIMING("del-buf", n) + { + int32 j = r_ord[i]; + BufTableDeleteBuffer(bufids[j], phash[j]); + } + END_TIMING + + BEGIN_TIMING("insert", n) + { + int32 j = w_ord[i]; + BufTableInsert(&ptag[j], phash[j], bufids[j]); + } + END_TIMING + BEGIN_TIMING("miss", n) { int32 j = r_ord[i]; -- 2.53.0