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
Attachments:
v1-0001-adding-isCatalogRel-to-rmgrdesc.patchtext/plain; charset=UTF-8; name=v1-0001-adding-isCatalogRel-to-rmgrdesc.patchDownload
From 564e689cf74e1b9893a28b5ecf6bdbc6fb549232 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 11 Dec 2023 21:10:30 +0000
Subject: [PATCH v1] adding isCatalogRel to rmgrdesc
---
src/backend/access/rmgrdesc/gistdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/hashdesc.c | 5 +++--
src/backend/access/rmgrdesc/heapdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/nbtdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/spgdesc.c | 5 +++--
5 files changed, 24 insertions(+), 16 deletions(-)
100.0% src/backend/access/rmgrdesc/
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 5dc6e1dcee..8008c041a6 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -26,18 +26,20 @@ out_gistxlogPageUpdate(StringInfo buf, gistxlogPageUpdate *xlrec)
static void
out_gistxlogPageReuse(StringInfo buf, gistxlogPageReuse *xlrec)
{
- 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);
}
static void
out_gistxlogDelete(StringInfo buf, gistxlogDelete *xlrec)
{
- appendStringInfo(buf, "delete: snapshotConflictHorizon %u, nitems: %u",
- xlrec->snapshotConflictHorizon, xlrec->ntodelete);
+ appendStringInfo(buf, "delete: snapshotConflictHorizon %u, nitems: %u, isCatalogRel %u",
+ xlrec->snapshotConflictHorizon, xlrec->ntodelete,
+ xlrec->isCatalogRel);
}
static void
diff --git a/src/backend/access/rmgrdesc/hashdesc.c b/src/backend/access/rmgrdesc/hashdesc.c
index b6810a9320..49855ce467 100644
--- a/src/backend/access/rmgrdesc/hashdesc.c
+++ b/src/backend/access/rmgrdesc/hashdesc.c
@@ -113,9 +113,10 @@ hash_desc(StringInfo buf, XLogReaderState *record)
{
xl_hash_vacuum_one_page *xlrec = (xl_hash_vacuum_one_page *) rec;
- appendStringInfo(buf, "ntuples %d, snapshotConflictHorizon %u",
+ appendStringInfo(buf, "ntuples %d, snapshotConflictHorizon %u, isCatalogRel %u",
xlrec->ntuples,
- xlrec->snapshotConflictHorizon);
+ xlrec->snapshotConflictHorizon,
+ xlrec->isCatalogRel);
break;
}
}
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index f382c0f623..3c17485b02 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -179,10 +179,11 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
{
xl_heap_prune *xlrec = (xl_heap_prune *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, nredirected: %u, ndead: %u",
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, nredirected: %u, ndead: %u, isCatalogRel %u",
xlrec->snapshotConflictHorizon,
xlrec->nredirected,
- xlrec->ndead);
+ xlrec->ndead,
+ xlrec->isCatalogRel);
if (XLogRecHasBlockData(record, 0))
{
@@ -238,8 +239,9 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
{
xl_heap_freeze_page *xlrec = (xl_heap_freeze_page *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, nplans: %u",
- xlrec->snapshotConflictHorizon, xlrec->nplans);
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, nplans: %u, isCatalogRel %u",
+ xlrec->snapshotConflictHorizon, xlrec->nplans,
+ xlrec->isCatalogRel);
if (XLogRecHasBlockData(record, 0))
{
diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c
index f3d725a274..5d87acbbfe 100644
--- a/src/backend/access/rmgrdesc/nbtdesc.c
+++ b/src/backend/access/rmgrdesc/nbtdesc.c
@@ -71,9 +71,10 @@ btree_desc(StringInfo buf, XLogReaderState *record)
{
xl_btree_delete *xlrec = (xl_btree_delete *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, ndeleted: %u, nupdated: %u",
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, ndeleted: %u, nupdated: %u, isCatalogRel %u",
xlrec->snapshotConflictHorizon,
- xlrec->ndeleted, xlrec->nupdated);
+ xlrec->ndeleted, xlrec->nupdated,
+ xlrec->isCatalogRel);
if (XLogRecHasBlockData(record, 0))
delvacuum_desc(buf, XLogRecGetBlockData(record, 0, NULL),
@@ -113,11 +114,12 @@ btree_desc(StringInfo buf, XLogReaderState *record)
{
xl_btree_reuse_page *xlrec = (xl_btree_reuse_page *) rec;
- appendStringInfo(buf, "rel: %u/%u/%u, snapshotConflictHorizon: %u:%u",
+ appendStringInfo(buf, "rel: %u/%u/%u, snapshotConflictHorizon: %u:%u, isCatalogRel %u",
xlrec->locator.spcOid, xlrec->locator.dbOid,
xlrec->locator.relNumber,
EpochFromFullTransactionId(xlrec->snapshotConflictHorizon),
- XidFromFullTransactionId(xlrec->snapshotConflictHorizon));
+ XidFromFullTransactionId(xlrec->snapshotConflictHorizon),
+ xlrec->isCatalogRel);
break;
}
case XLOG_BTREE_META_CLEANUP:
diff --git a/src/backend/access/rmgrdesc/spgdesc.c b/src/backend/access/rmgrdesc/spgdesc.c
index 87f62f0fb4..5f4eeaa240 100644
--- a/src/backend/access/rmgrdesc/spgdesc.c
+++ b/src/backend/access/rmgrdesc/spgdesc.c
@@ -118,10 +118,11 @@ spg_desc(StringInfo buf, XLogReaderState *record)
{
spgxlogVacuumRedirect *xlrec = (spgxlogVacuumRedirect *) rec;
- appendStringInfo(buf, "ntoplaceholder: %u, firstplaceholder: %u, snapshotConflictHorizon: %u",
+ appendStringInfo(buf, "ntoplaceholder: %u, firstplaceholder: %u, snapshotConflictHorizon: %u, isCatalogRel %u",
xlrec->nToPlaceholder,
xlrec->firstPlaceholder,
- xlrec->snapshotConflictHorizon);
+ xlrec->snapshotConflictHorizon,
+ xlrec->isCatalogRel);
}
break;
}
--
2.34.1
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
Attachments:
v2-0001-adding-isCatalogRel-to-rmgrdesc.patchtext/plain; charset=UTF-8; name=v2-0001-adding-isCatalogRel-to-rmgrdesc.patchDownload
From 9f7856fa007a2bec2c4c579e4b0730a476950a47 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Date: Mon, 11 Dec 2023 21:10:30 +0000
Subject: [PATCH v2] adding isCatalogRel to rmgrdesc
---
src/backend/access/rmgrdesc/gistdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/hashdesc.c | 5 +++--
src/backend/access/rmgrdesc/heapdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/nbtdesc.c | 10 ++++++----
src/backend/access/rmgrdesc/spgdesc.c | 5 +++--
5 files changed, 24 insertions(+), 16 deletions(-)
100.0% src/backend/access/rmgrdesc/
diff --git a/src/backend/access/rmgrdesc/gistdesc.c b/src/backend/access/rmgrdesc/gistdesc.c
index 5dc6e1dcee..651e645b85 100644
--- a/src/backend/access/rmgrdesc/gistdesc.c
+++ b/src/backend/access/rmgrdesc/gistdesc.c
@@ -26,18 +26,20 @@ out_gistxlogPageUpdate(StringInfo buf, gistxlogPageUpdate *xlrec)
static void
out_gistxlogPageReuse(StringInfo buf, gistxlogPageReuse *xlrec)
{
- appendStringInfo(buf, "rel %u/%u/%u; blk %u; snapshotConflictHorizon %u:%u",
+ appendStringInfo(buf, "rel %u/%u/%u; blk %u; snapshotConflictHorizon %u:%u, isCatalogRel %c",
xlrec->locator.spcOid, xlrec->locator.dbOid,
xlrec->locator.relNumber, xlrec->block,
EpochFromFullTransactionId(xlrec->snapshotConflictHorizon),
- XidFromFullTransactionId(xlrec->snapshotConflictHorizon));
+ XidFromFullTransactionId(xlrec->snapshotConflictHorizon),
+ xlrec->isCatalogRel ? 'T' : 'F');
}
static void
out_gistxlogDelete(StringInfo buf, gistxlogDelete *xlrec)
{
- appendStringInfo(buf, "delete: snapshotConflictHorizon %u, nitems: %u",
- xlrec->snapshotConflictHorizon, xlrec->ntodelete);
+ appendStringInfo(buf, "delete: snapshotConflictHorizon %u, nitems: %u, isCatalogRel %c",
+ xlrec->snapshotConflictHorizon, xlrec->ntodelete,
+ xlrec->isCatalogRel ? 'T' : 'F');
}
static void
diff --git a/src/backend/access/rmgrdesc/hashdesc.c b/src/backend/access/rmgrdesc/hashdesc.c
index b6810a9320..98c4b30b3e 100644
--- a/src/backend/access/rmgrdesc/hashdesc.c
+++ b/src/backend/access/rmgrdesc/hashdesc.c
@@ -113,9 +113,10 @@ hash_desc(StringInfo buf, XLogReaderState *record)
{
xl_hash_vacuum_one_page *xlrec = (xl_hash_vacuum_one_page *) rec;
- appendStringInfo(buf, "ntuples %d, snapshotConflictHorizon %u",
+ appendStringInfo(buf, "ntuples %d, snapshotConflictHorizon %u, isCatalogRel %c",
xlrec->ntuples,
- xlrec->snapshotConflictHorizon);
+ xlrec->snapshotConflictHorizon,
+ xlrec->isCatalogRel ? 'T' : 'F');
break;
}
}
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index f382c0f623..07abcb61b2 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -179,10 +179,11 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
{
xl_heap_prune *xlrec = (xl_heap_prune *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, nredirected: %u, ndead: %u",
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, nredirected: %u, ndead: %u, isCatalogRel: %c",
xlrec->snapshotConflictHorizon,
xlrec->nredirected,
- xlrec->ndead);
+ xlrec->ndead,
+ xlrec->isCatalogRel ? 'T' : 'F');
if (XLogRecHasBlockData(record, 0))
{
@@ -238,8 +239,9 @@ heap2_desc(StringInfo buf, XLogReaderState *record)
{
xl_heap_freeze_page *xlrec = (xl_heap_freeze_page *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, nplans: %u",
- xlrec->snapshotConflictHorizon, xlrec->nplans);
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, nplans: %u, isCatalogRel: %c",
+ xlrec->snapshotConflictHorizon, xlrec->nplans,
+ xlrec->isCatalogRel ? 'T' : 'F');
if (XLogRecHasBlockData(record, 0))
{
diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c
index f3d725a274..987b85549a 100644
--- a/src/backend/access/rmgrdesc/nbtdesc.c
+++ b/src/backend/access/rmgrdesc/nbtdesc.c
@@ -71,9 +71,10 @@ btree_desc(StringInfo buf, XLogReaderState *record)
{
xl_btree_delete *xlrec = (xl_btree_delete *) rec;
- appendStringInfo(buf, "snapshotConflictHorizon: %u, ndeleted: %u, nupdated: %u",
+ appendStringInfo(buf, "snapshotConflictHorizon: %u, ndeleted: %u, nupdated: %u, isCatalogRel: %c",
xlrec->snapshotConflictHorizon,
- xlrec->ndeleted, xlrec->nupdated);
+ xlrec->ndeleted, xlrec->nupdated,
+ xlrec->isCatalogRel ? 'T' : 'F');
if (XLogRecHasBlockData(record, 0))
delvacuum_desc(buf, XLogRecGetBlockData(record, 0, NULL),
@@ -113,11 +114,12 @@ btree_desc(StringInfo buf, XLogReaderState *record)
{
xl_btree_reuse_page *xlrec = (xl_btree_reuse_page *) rec;
- appendStringInfo(buf, "rel: %u/%u/%u, snapshotConflictHorizon: %u:%u",
+ appendStringInfo(buf, "rel: %u/%u/%u, snapshotConflictHorizon: %u:%u, isCatalogRel: %c",
xlrec->locator.spcOid, xlrec->locator.dbOid,
xlrec->locator.relNumber,
EpochFromFullTransactionId(xlrec->snapshotConflictHorizon),
- XidFromFullTransactionId(xlrec->snapshotConflictHorizon));
+ XidFromFullTransactionId(xlrec->snapshotConflictHorizon),
+ xlrec->isCatalogRel ? 'T' : 'F');
break;
}
case XLOG_BTREE_META_CLEANUP:
diff --git a/src/backend/access/rmgrdesc/spgdesc.c b/src/backend/access/rmgrdesc/spgdesc.c
index 87f62f0fb4..9fe906efc8 100644
--- a/src/backend/access/rmgrdesc/spgdesc.c
+++ b/src/backend/access/rmgrdesc/spgdesc.c
@@ -118,10 +118,11 @@ spg_desc(StringInfo buf, XLogReaderState *record)
{
spgxlogVacuumRedirect *xlrec = (spgxlogVacuumRedirect *) rec;
- appendStringInfo(buf, "ntoplaceholder: %u, firstplaceholder: %u, snapshotConflictHorizon: %u",
+ appendStringInfo(buf, "ntoplaceholder: %u, firstplaceholder: %u, snapshotConflictHorizon: %u, isCatalogRel: %c",
xlrec->nToPlaceholder,
xlrec->firstPlaceholder,
- xlrec->snapshotConflictHorizon);
+ xlrec->snapshotConflictHorizon,
+ xlrec->isCatalogRel ? 'T' : 'F');
}
break;
}
--
2.34.1
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