Hi, On 2022-10-13 17:46:25 -0700, Peter Geoghegan wrote: > On Fri, Sep 30, 2022 at 12:05 PM Andres Freund <and...@anarazel.de> wrote: > > My problem with this approach is that the whole cleanup lock is hugely > > misleading as-is. > > While nbtree VACUUM does use cleanup locks, they don't protect the > index structure itself -- it actually functions as an interlock > against concurrent TID recycling, which might otherwise confuse > in-flight index scans. That's why we need cleanup locks for VACUUM, > but not for index deletions, even though the physical modifications > that are performed to physical leaf pages are identical (the WAL > records are almost identical). Clearly the use of cleanup locks is not > really about protecting the leaf page itself -- it's about using the > physical leaf page as a proxy for the heap TIDs contained therein. A > very narrow protocol with a very specific purpose. > > More generally, cleanup locks exist to protect transient references > that point into a heap page. References held by one backend only. A > TID, or a HeapTuple C pointer, or something similar. Cleanup locks are > not intended to protect a physical data structure in the heap, either > -- just a reference/pointer that points to the structure. There are > implications for the physical page structure itself, of course, but > that seems secondary. The guarantees are often limited to "never allow > the backend holding the pin to become utterly confused". > > I am skeptical of the idea of using cleanup locks for anything more > ambitious than this. Especially in index AM code. It seems > uncomfortably close to "a buffer lock, but somehow also not a buffer > lock".
My point here is a lot more mundane. The code essentially does _hash_pageinit(), overwriting the whole page, and *then* conditionally acquires a cleanup lock. It simply is bogus code. Greetings, Andres Freund
