Re: hash_xlog_split_allocate_page: failed to acquire cleanup lock

On Wed, Aug 17, 2022 at 6:27 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,
>
> On 2022-08-16 19:55:18 -0400, Tom Lane wrote:
> > Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > > What sort of random things would someone do with pageinspect functions
> > > that would hold buffer pins on one buffer while locking another one?
> > > The functions in hashfuncs.c don't even seem like they would access
> > > multiple buffers in total, let alone at overlapping times. And I don't
> > > think that a query pageinspect could realistically be suspended while
> > > holding a buffer pin either. If you wrapped it in a cursor it'd be
> > > suspended before or after accessing any given buffer, not right in the
> > > middle of that operation.
> >
> > pin != access. Unless things have changed really drastically since
> > I last looked, a seqscan will sit on a buffer pin throughout the
> > series of fetches from a single page.
>
> That's still the case. But for heap that shouldn't be a problem, because we'll
> never try to take a cleanup lock while holding another page locked (nor even
> another heap page pinned, I think).
>
>
> I find it *highly* suspect that hash needs to acquire a cleanup lock while
> holding another buffer locked. The recovery aspect alone makes that seem quite
> unwise. Even if there's possibly no deadlock here for some reason or another.
>
>
> Looking at the non-recovery code makes me even more suspicious:
>
> /*
> * Physically allocate the new bucket's primary page. We want to do this
> * before changing the metapage's mapping info, in case we can't get the
> * disk space. Ideally, we don't need to check for cleanup lock on new
> * bucket as no other backend could find this bucket unless meta page is
> * updated. However, it is good to be consistent with old bucket locking.
> */
> buf_nblkno = _hash_getnewbuf(rel, start_nblkno, MAIN_FORKNUM);
> if (!IsBufferCleanupOK(buf_nblkno))
> {
> _hash_relbuf(rel, buf_oblkno);
> _hash_relbuf(rel, buf_nblkno);
> goto fail;
> }
>
>
> _hash_getnewbuf() calls _hash_pageinit() which calls PageInit(), which
> memset(0)s the whole page. What does it even mean to check whether you
> effectively have a cleanup lock after you zeroed out the page?
>
> Reading the README and the comment above makes me wonder if this whole cleanup
> lock business here is just cargo culting and could be dropped?
>

I think it is okay to not acquire a clean-up lock on the new bucket
page both in recovery and non-recovery paths. It is primarily required
on the old bucket page to avoid concurrent scans/inserts. As mentioned
in the comments and as per my memory serves, it is mainly for keeping
it consistent with old bucket locking.

--
With Regards,
Amit Kapila.