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@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.

Regards,

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