On Tue, Dec 19, 2023 at 6:27 PM Drouvot, Bertrand <bertranddrouvot...@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 <mich...@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
