Re: Diagnostic comment in LogicalIncreaseXminForSlot

On Mon, Jul 12, 2021 at 8:39 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:

> On Mon, Jul 5, 2021 at 12:54 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> >
> > On Fri, May 21, 2021 at 6:00 PM Ashutosh Bapat
> > <ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
> > >
> > > It's there in CF. I am fine with PG-15. It will be good to patch the
> back-branches to have this extra diagnostic information available.
> >
> > The patch looks to me.
> >
>
> {
> slot->candidate_catalog_xmin = xmin;
> slot->candidate_xmin_lsn = current_lsn;
> + elog(DEBUG1, "got new catalog_xmin %u at %X/%X", xmin,
> + LSN_FORMAT_ARGS(current_lsn));
> }
> SpinLockRelease(&slot->mutex);
>
> Today, again looking at this patch, I don't think doing elog inside
> spinlock is a good idea. We can either release spinlock before it or
> use some variable to remember that we need to write such an elog and
> do it after releasing the lock. What do you think?

The elog will be effective only under DEBUG1, otherwise it will be almost a
NOOP. I am wondering whether it's worth adding a bool assignment and move
the elog out only for DEBUG1. Anyway, will defer it to you.

> I have noticed that
> a nearby function LogicalIncreaseRestartDecodingForSlot() logs similar
> information after releasing spinlock, so it is better to follow the
> same here as well.
>

Now that you mention it, the code their looks rather suspicious :)
We acquire the spinlock at the beginning of the function but do not release
it if (restart_lsn <= slot->data.restart_lsn) or if (current_lsn <=
slot->data.confirmed_flush). I might be missing something there. But it
looks unrelated.

--
--
Best Wishes,
Ashutosh