Re: Add isCatalogRel in rmgrdesc

Hi,

On 12/19/23 9:00 AM, Masahiko Sawada wrote:
> 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

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.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com