Re: Diagnostic comment in LogicalIncreaseXminForSlot

On Mon, Jul 12, 2021 at 5:28 PM Ashutosh Bapat
<ashutosh(dot)bapat(at)enterprisedb(dot)com> wrote:
>
> 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.
>

If you don't like adding another variable then probably we can release
spinlock in each of if .. else loop. As mentioned previously, it
doesn't seem a good idea to use elog inside spinlock, so we either
need to find some way to avoid that or probably will drop this patch.
Do let me know what you or others prefer here?

--
With Regards,
Amit Kapila.