Hi,
On 2022-10-13 17:46:25 -0700, Peter Geoghegan wrote:
> On Fri, Sep 30, 2022 at 12:05 PM Andres Freund <andres@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
