Обсуждение: Add isCatalogRel in rmgrdesc
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
Вложения
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
Вложения
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
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
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
Вложения
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
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
Вложения
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
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
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