On Thu, Aug 13, 2026 at 12:52 PM Alexandre Felipe < [email protected]> 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
