Re: Is this a problem in GenericXLogFinish()?

On Wed, Nov 1, 2023 at 12:54 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Mon, Oct 30, 2023 at 03:54:39PM +0530, Amit Kapila wrote:
> > On Sat, Oct 28, 2023 at 4:30 PM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> >> On Sat, Oct 28, 2023 at 03:45:13PM +0530, Amit Kapila wrote:
> >> > Yes, we need it to exclude any concurrent in-progress scans that could
> >> > return incorrect tuples during bucket squeeze operation.
> >>
> >> Thanks. So I assume that we should just set REGBUF_NO_CHANGE when the
> >> primary and write buffers are the same and there are no tuples to
> >> move. Say with something like the attached?
> >
> > What if the primary and write buffer are not the same but ntups is
> > zero? Won't we hit the assertion again in that case?
>
> The code tells that it should be able to handle such a case,
>

It should be possible when we encounter some page in between the chain
that has all dead items and write buffer points to some page after the
primary bucket page and before the read buffer page.

> so this
> would mean to set REGBUF_NO_CHANGE only when we have no tuples to
> move:
> - XLogRegisterBuffer(1, wbuf, REGBUF_STANDARD);
> + /*
> + * A cleanup lock is still required on the write buffer even
> + * if there are no tuples to move, so it needs to be registered
> + * in this case.
> + */
> + wbuf_flags = REGBUF_STANDARD;
> + if (xlrec.ntups == 0)
> + wbuf_flags |= REGBUF_NO_CHANGE;
> + XLogRegisterBuffer(1, wbuf, wbuf_flags);
>

Why register wbuf at all if there are no tuples to add and it is not
the same as bucketbuf? Also, I think this won't be correct if prevbuf
and wrtbuf are the same and also we have no tuples to add to wbuf. I
have attached a naive and crude way to achieve it. This needs more
work both in terms of trying to find a better way to change the code
or ensure this won't break any existing case. I have just run the
existing tests. Such a fix certainly required more testing.

--
With Regards,
Amit Kapila.