Re: hash_xlog_split_allocate_page: failed to acquire cleanup lock

Hi,

On 2022-10-17 10:43:16 -0400, Robert Haas wrote:
> On Fri, Oct 14, 2022 at 2:21 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > On 2022-10-14 10:40:11 +0530, Amit Kapila wrote:
> > > On Fri, Oct 14, 2022 at 2:25 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > Fair point. How about something like: "XXX Do we really need to check
> > > for cleanup lock on the new bucket? Here, we initialize the page, so
> > > ideally we don't need to perform any operation that requires such a
> > > check."?.
> >
> > This still seems to omit that the code is quite broken.
>
> I don't think it's the job of a commit which is trying to fix a
> certain bug to document all the other bugs it isn't fixing.

That's true in general, but the case of fixing a bug in one place but not in
another nearby is a different story.

> If that were the standard, we'd never be able to commit any bug fixes

<eyeroll/>

> which is exactly what's happening on this thread.

What's been happening from my POV is that Amit and you didn't even acknowledge
the broken cleanup lock logic for weeks. I don't mind a reasoned decision to
not care about the non-recovery case. But until now I've not seen that, but I
also have a hard time keeping up with email, so I might have missed it.

> > > Feel free to suggest something better.
> >
> > How about something like:
> >
> > XXX: This code is wrong, we're overwriting the buffer before "acquiring" the
> > cleanup lock. Currently this is not known to have bad consequences because
> > XYZ and the fix seems a bit too risky for the backbranches.
>
> I think here you're talking about the code that runs in
> normal-running, not recovery, in _hash_expandtable, where we call
> _hash_getnewbuf and then IsBufferCleanupOK.

Yes.

> That code indeed seems stupid, because as you say, there's no point in
> calling _hash_getnewbuf() and thus overwriting the buffer and then only
> afterwards checking IsBufferCleanupOK. By then the die is cast. But at the
> same time, I think that it's not wrong in any way that matters to the best
> of our current knowledge. That new buffer that we just went and got might be
> pinned by somebody else, but they can't be doing anything interesting with
> it, because we wouldn't be allocating it as a new page if it were already in
> use for anything, and only one process is allowed to be doing such an
> allocation at a time. That both means that we can likely remove the cleanup
> lock acquisition, but it also means that if we don't, there is no
> correctness problem here, strictly speaking.

If that's the case cool - I just don't know the locking protocol of hash
indexes well enough to judge this.

> So I would suggest that if we feel we absolutely must put a comment
> here, we could make it say something like "XXX. It doesn't make sense
> to call _hash_getnewbuf() first, zeroing the buffer, and then only
> afterwards check whether we have a cleanup lock. However, since no
> scan can be accessing the new buffer yet, any concurrent accesses will
> just be from processes like the bgwriter or checkpointer which don't
> care about its contents, so it doesn't really matter."

WFM. I'd probably lean to just fixing in the backbranches instead, but as long
as we make a conscious decision...

Greetings,

Andres Freund