Re: Restructured Shared Buffer Hash Table

On Thu, Aug 13, 2026 at 12:52 PM Alexandre Felipe <
o(dot)alexandre(dot)felipe(at)gmail(dot)com> wrote:

> > I don't really understand the race condition this is trying to address:
> >
> >> + /* Unlock buffer header after the entry is deleted to avoid a
> race condition:
> >> + * If unlocked prior, a concurrent GetVictimBuffer() could insert
> a new entry
> >> + * for the same buffer and overwrite the entry slot. Then, the
> BufTableDelete()
> >> + * would be unable to find the entry and would corrupt the
> hashtable. */
> >> + UnlockBufHdrExt(buf, buf_state,
> >> + 0,
> >> + BUF_FLAG_MASK | BUF_USAGECOUNT_MASK,
> >> + 0);
> >
> >How could there be a concurrent insertion while the buffer partition lock
> is
> >held?
>
>
> I am removing that part as there is a consensus among the big guys that
> we shouldn't hold a spin-lock while doing the BufferTableDelete.
>
> I think I could explain why this is necessary, but I want to see your
> argument.
> Was it safe before? if so, what is the property of dynhash that we lost on
> this
> patch.
>
> I could argue that BufferTableDelete is safe if we check for the bounds of
> .next and make sure there are no cycles in the chain. But again I will let
> you elaborate that argument.
>
> Regards,
> Alexandre
>

Apologies for the inadequate race condition description in the comment.
Without the spinlock change, a concurrent backend can overwrite the entry
slot that is supposed to be deleted imminently after the header lock is
released, corrupting the bucket chains.
Consider two concurrent backends b1 and b2 in the following scenario:

1. b1 executes within InvalidateVictimBuffer(), clearing the header for
buffer *x* and unlocking the header lock.
2. b2 calls GetVictimBuffer() which returns *x*, and b2 proceeds to
insert a new entry into entry slot *x* via BufTableInsert(). This
overwrites the original entry in that slot and breaks the bucket chain the
original entry was part of.
3. b1 continues with BufTableDelete() to delete the entry that is no
longer in slot *x* (since b2 overwrote it) and is unable to find it.

I do not believe simply following the .next indices and checking for cycles
is sufficient for two main reasons:

1. The entry *p* that used to chain to entry *x* through its .next still
also points to the new entry *x*. If the new entry *x* hashes to a
different bucket than entry *p *and the original entry *x*, following
entry *p* to the new entry *x* would jump buckets (which is incorrect)
and potentially bypass the partition locking paradigm, causing further
concurrency issues.
2. The entries that were once reached by following the .next of the
original entry *x* are now unreachable, causing false lookup misses.

This is not an issue with dynahash because it allocates new entries via the
freelist, which safely allows inserting a new entry while the stale one
waits to be deleted.
Since the patched table instead maintains that entry *x* describes buffer
*x*, we cannot have both the new entry and the stale one present
simultaneously; the old one must be fully deleted (with the bucket chains
adjusted appropriately) before the new one is inserted.
As such, the patched code is unsafe without the lock change, though I admit
that holding the lock while calling BufTableDelete is not ideal and other
solutions should be explored.

Best,
Dhruv Aron