Horton

Thread

Commits

  1. Show isCatalogRel in several rmgr descriptions.

  1. Add isCatalogRel in rmgrdesc

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-12-12T08:23:46Z

    Hi hackers,

6af1793954 added a new field namely "isCatalogRel" in some WAL records
to help detecting row removal conflict during logical decoding from standby.

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

Looking forward to your feedback,

Regards,

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

  2. Re: Add isCatalogRel in rmgrdesc

    Michael Paquier <michael@paquier.xyz> — 2023-12-12T09:15:01Z

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

Agreed that this is helpful.  One would likely guess if you are
dealing with a catalog relation depending on its relfilenode, but that
does not take into account user_catalog_table that can be set as a
reloption, impacting the value of isCatalogRel stored in the records.
--
Michael

  3. Re: Add isCatalogRel in rmgrdesc

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-12-12T09:41:22Z

    Hi,

On 12/12/23 10:15 AM, Michael Paquier 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).
> 
> Agreed that this is helpful.

Thanks for looking at it!

>  One would likely guess if you are
> dealing with a catalog relation depending on its relfilenode, but that
> does not take into account user_catalog_table that can be set as a
> reloption, impacting the value of isCatalogRel stored in the records.

Exactly and not mentioning the other checks in RelationIsAccessibleInLogicalDecoding()
like the wal_level >= logical one.

Regards,

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

  4. Re: Add isCatalogRel in rmgrdesc

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-12-19T08:00:09Z

    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

-   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

  5. Re: Add isCatalogRel in rmgrdesc

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-12-19T09:27:39Z

    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

  6. Re: Add isCatalogRel in rmgrdesc

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-12-20T01:43:30Z

    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

  7. Re: Add isCatalogRel in rmgrdesc

    Michael Paquier <michael@paquier.xyz> — 2023-12-21T00:04:38Z

    On Wed, Dec 20, 2023 at 10:43:30AM +0900, Masahiko Sawada wrote:
> Thank you for updating the patch. The v2 patch looks good to me. I'll
> push it, barring any objections.

This is capturing the eight records where the flag exists, so it looks
OK seen from here.  

As you said, there may be a point in reducing the output in the most
common case and not show the flag when !isCatalogRel, but I cannot get
excited about that either because that would require one to do more
cross-checks with the core code when looking at WAL dumps.
--
Michael

  8. Re: Add isCatalogRel in rmgrdesc

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-12-21T01:13:16Z

    On Thu, Dec 21, 2023 at 9:04 AM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Dec 20, 2023 at 10:43:30AM +0900, Masahiko Sawada wrote:
> > Thank you for updating the patch. The v2 patch looks good to me. I'll
> > push it, barring any objections.
>
> This is capturing the eight records where the flag exists, so it looks
> OK seen from here.
>
> As you said, there may be a point in reducing the output in the most
> common case and not show the flag when !isCatalogRel, but I cannot get
> excited about that either because that would require one to do more
> cross-checks with the core code when looking at WAL dumps.

Thank you for the comments. Agreed.

I've just pushed, bf6260b39.

Regards,

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

  9. Re: Add isCatalogRel in rmgrdesc

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-12-21T01:18:22Z

    On Thu, Dec 21, 2023 at 10:13 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Thu, Dec 21, 2023 at 9:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Dec 20, 2023 at 10:43:30AM +0900, Masahiko Sawada wrote:
> > > Thank you for updating the patch. The v2 patch looks good to me. I'll
> > > push it, barring any objections.
> >
> > This is capturing the eight records where the flag exists, so it looks
> > OK seen from here.
> >
> > As you said, there may be a point in reducing the output in the most
> > common case and not show the flag when !isCatalogRel, but I cannot get
> > excited about that either because that would require one to do more
> > cross-checks with the core code when looking at WAL dumps.
>
> Thank you for the comments. Agreed.
>
> I've just pushed, bf6260b39.
>

FYI, in the commitfest app, there seems to be two duplicated entries
for this item:

https://commitfest.postgresql.org/46/4694/
https://commitfest.postgresql.org/46/4695/

I've marked the latter one as committed, and will remove the former.

Regards,

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

  10. Re: Add isCatalogRel in rmgrdesc

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2023-12-21T05:56:12Z

    Hi,

On Thu, Dec 21, 2023 at 10:13:16AM +0900, Masahiko Sawada wrote:
> On Thu, Dec 21, 2023 at 9:04 AM Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Wed, Dec 20, 2023 at 10:43:30AM +0900, Masahiko Sawada wrote:
> > > Thank you for updating the patch. The v2 patch looks good to me. I'll
> > > push it, barring any objections.
> >
> > This is capturing the eight records where the flag exists, so it looks
> > OK seen from here.
> >
> > As you said, there may be a point in reducing the output in the most
> > common case and not show the flag when !isCatalogRel, but I cannot get
> > excited about that either because that would require one to do more
> > cross-checks with the core code when looking at WAL dumps.
> 
> Thank you for the comments. Agreed.
> 
> I've just pushed, bf6260b39.
> 

Thanks!

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