Re: Add isCatalogRel in rmgrdesc

On Tue, Dec 12, 2023 at 6:15 PM Michael Paquier <michael(at)paquier(dot)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

- 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? I prefer to always
print the field name like the current patch and hashdesc.c since it's
easier to parse it. But I'm fine with either way to show the field
value.

Regards,

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