Re: Add isCatalogRel in rmgrdesc

On Tue, Dec 19, 2023 at 6:27 PM Drouvot, Bertrand
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On 12/19/23 9:00 AM, Masahiko Sawada wrote:
> > On Tue, Dec 12, 2023 at 6:15 PM Michael Paquier <michael@paquier.xyz> wrote:
> >>
> >> On Tue, Dec 12, 2023 at 09:23:46AM +0100, Drouvot, Bertrand wrote:
> >>> Please find attached a patch to add this field in the related rmgrdesc (i.e
> >>> all the ones that already provide the snapshotConflictHorizon except the one
> >>> related to xl_heap_visible: indeed a new bit was added in its flag field in 6af1793954
> >>> instead of adding the isCatalogRel bool).
> >>>
> >>> I think it's worth it, as this new field could help diagnose conflicts issues (if any).
> >
> > +1
>
> Thanks for looking at it!
>
> >
> > -   appendStringInfo(buf, "rel %u/%u/%u; blk %u; snapshotConflictHorizon %u:%u",
> > +   appendStringInfo(buf, "rel %u/%u/%u; blk %u;
> > snapshotConflictHorizon %u:%u, isCatalogRel %u",
> >                       xlrec->locator.spcOid, xlrec->locator.dbOid,
> >                       xlrec->locator.relNumber, xlrec->block,
> >                       EpochFromFullTransactionId(xlrec->snapshotConflictHorizon),
> > -                    XidFromFullTransactionId(xlrec->snapshotConflictHorizon));
> > +                    XidFromFullTransactionId(xlrec->snapshotConflictHorizon),
> > +                    xlrec->isCatalogRel);
> >
> > The patch prints isCatalogRel, a bool field, as %u. But other rmgrdesc
> > implementations seem to use different ways. For instance in spgdesc.c,
> > we print flag name if it's set: (newPage and postfixBlkSame are bool
> > fields):
> >
> >                  appendStringInfo(buf, "prefixoff: %u, postfixoff: %u",
> >                                   xlrec->offnumPrefix,
> >                                   xlrec->offnumPostfix);
> >                  if (xlrec->newPage)
> >                      appendStringInfoString(buf, " (newpage)");
> >                  if (xlrec->postfixBlkSame)
> >                      appendStringInfoString(buf, " (same)");
> >
> > whereas in hashdesc.c, we print either 'T' of 'F':
> >
> >                  appendStringInfo(buf, "clear_dead_marking %c, is_primary %c",
> >                                   xlrec->clear_dead_marking ? 'T' : 'F',
> >                                   xlrec->is_primary_bucket_page ? 'T' : 'F');
> >
> > Is it probably worth considering such formats?
>
> Good point, let's not add another format.
>
> > I prefer to always
> > print the field name like the current patch and hashdesc.c since it's
> > easier to parse it.
>
> I like this format too, so done that way in v2 attached.
>
> BTW, I noticed that sometimes the snapshotConflictHorizon is displayed
> as "snapshotConflictHorizon:" and sometimes as "snapshotConflictHorizon".
>
> So v2 is doing the same, means using "isCatalogRel:" if "snapshotConflictHorizon:"
> is being used or using "isCatalogRel" if not.

Agreed.

Thank you for updating the patch. The v2 patch looks good to me. I'll
push it, barring any objections.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com