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.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 9f7856fa007a2bec2c4c579e4b0730a476950a47 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@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
