Remove custom redundant full page write description from GIN
Hi hackers!
While reading waldump output for the GIN index I noticed $subj.
Here is an example:
```
rmgr: Gin len (rec/tot): 53/ 113, tx: 788, lsn:
0/01D2B700, prev 0/01D2B590, desc: INSERT isdata: F isleaf: T (full
page image), blkref #0: rel 1663/16384/32857 blk 1 FPW
```
Notice we have "(full page image)" from gindesc and "FPW" from generic
waldump code
I suggest removing this custom FPW support.
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Remove-custom-full-page-write-decribption-from-GI.patchapplication/octet-stream; name=v1-0001-Remove-custom-full-page-write-decribption-from-GI.patchDownload
From 62eebcc826d54a96771fca0721c08fcb7c3c2cab Mon Sep 17 00:00:00 2001
From: reshke <reshke@qavm-273b4667.qemu>
Date: Mon, 15 Sep 2025 15:50:14 +0300
Subject: [PATCH v1] Remove custom full page write decribption from GIN
---
src/backend/access/rmgrdesc/gindesc.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 723ff9499c..229675775f 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -99,14 +99,7 @@ gin_desc(StringInfo buf, XLogReaderState *record)
appendStringInfo(buf, " children: %u/%u",
leftChildBlkno, rightChildBlkno);
}
- if (XLogRecHasBlockImage(record, 0))
- {
- if (XLogRecBlockImageApply(record, 0))
- appendStringInfoString(buf, " (full page image)");
- else
- appendStringInfoString(buf, " (full page image, for WAL verification)");
- }
- else
+ if (!XLogRecHasBlockImage(record, 0))
{
char *payload = XLogRecGetBlockData(record, 0, NULL);
@@ -144,14 +137,7 @@ gin_desc(StringInfo buf, XLogReaderState *record)
break;
case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
{
- if (XLogRecHasBlockImage(record, 0))
- {
- if (XLogRecBlockImageApply(record, 0))
- appendStringInfoString(buf, " (full page image)");
- else
- appendStringInfoString(buf, " (full page image, for WAL verification)");
- }
- else
+ if (!XLogRecHasBlockImage(record, 0))
{
ginxlogVacuumDataLeafPage *xlrec =
(ginxlogVacuumDataLeafPage *) XLogRecGetBlockData(record, 0, NULL);
--
2.43.0
On 15 Sep 2025, at 17:56, Kirill Reshke <reshkekirill@gmail.com> wrote:
I suggest removing this custom FPW support.
I agree that extra message adds no value. Generic FPW message has the same "for verification" details too.
I've checked if there are any other similar cases, but found non FPW indications in other resource managers.
Maybe a litter comment about why we don't describe anything in presence of FPW would be good. But nearby code is not very verbose...
Best regards, Andrey Borodin.
On Mon, 15 Sept 2025 at 18:27, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 15 Sep 2025, at 17:56, Kirill Reshke <reshkekirill@gmail.com> wrote:
I suggest removing this custom FPW support.
I agree that extra message adds no value. Generic FPW message has the same "for verification" details too.
I've checked if there are any other similar cases, but found non FPW indications in other resource managers.
Thanks for review
Maybe a litter comment about why we don't describe anything in presence of FPW would be good. But nearby code is not very verbose...
I dunno. Looks like after removing code, this comment about FPW will
be just out of place. We already have documentation about pg_waldump
in general, don't we?
By the way, I have spotted a few new places which can be enhanced in
GIN redo & waldump.
PFA V2 series.
0001:
Basically same as v1-0001
0002:
During my work, I have to modify GIN pg_waldump to get more
information about some wal records. In v2-0002 there are two
modifications in XLOG_GIN_UPDATE_META_PAGE and
XLOG_GIN_INSERT_LISTPAGE, which has added the most value (for me). I
also used to display meta-page info, but I discarded this custom info
display logic, as it adds little value(is it?)
0003:
Small nitpick. I copy-pasted this code from pg core to my cpp project
and the compiler noticed the `payload` variable was not used after the
last modification. I find this true.
0004:
Remove the RelFileLocator field of ginxlogUpdateMeta walrecord. It is
not used in relay logic.
0005:
Small nitpicky patch to document ginxlogInsertListPage's backup blocks.
0006:
CREATE_PTREE always includes page contents in its walrecord on HEAD
(correct me if i'm wrong, but this is what I see in source and in
by-hand testing). In this patch I completely removes
XLOG_GIN_CREATE_PTREE custom logic and replace it with a general FPW
mechanism.
This patch reduces wal record size in case wal_compression is enabled.
Before 0006:
```
reshke@yezzey-cbdb-bench:~$ /home/reshke/pg19/bin/bin/pg_waldump -f
-r GIN db/pg_wal/000000010000000000000005 | grep create_p -i
rmgr: Gin len (rec/tot): 8117/ 8117, tx: 831, lsn:
0/05AF9B18, prev 0/05AF9A98, desc: CREATE_PTREE size: 8064, blkref #0:
rel 1663/16384/16541 blk 517
rmgr: Gin len (rec/tot): 8117/ 8117, tx: 831, lsn:
0/05B13078, prev 0/05B13030, desc: CREATE_PTREE size: 8064, blkref #0:
rel 1663/16384/16541 blk 525
rmgr: Gin len (rec/tot): 8117/ 8117, tx: 831, lsn:
0/05B2C5D8, prev 0/05B2C590, desc: CREATE_PTREE size: 8064, blkref #0:
rel 1663/16384/16541 blk 533
rmgr: Gin len (rec/tot): 8117/ 8117, tx: 831, lsn:
0/05B45B20, prev 0/05B45AD8, desc: CREATE_PTREE size: 8064, blkref #0:
rel 1663/16384/16541 blk 541
```
After 0006:
```
reshke@yezzey-cbdb-bench:~$ /home/reshke/pg19/bin/bin/pg_waldump -f
-r GIN db/pg_wal/000000010000000000000004 | grep CREATE
rmgr: Gin len (rec/tot): 51/ 410, tx: 795, lsn:
0/041FE560, prev 0/041FE4E0, desc: CREATE_PTREE , blkref #0: rel
1663/16384/16527 blk 517 FPW
rmgr: Gin len (rec/tot): 51/ 410, tx: 795, lsn:
0/042000B8, prev 0/04200070, desc: CREATE_PTREE , blkref #0: rel
1663/16384/16527 blk 525 FPW
rmgr: Gin len (rec/tot): 51/ 410, tx: 795, lsn:
0/04201BF8, prev 0/04201BB0, desc: CREATE_PTREE , blkref #0: rel
1663/16384/16527 blk 533 FPW
rmgr: Gin len (rec/tot): 51/ 410, tx: 795, lsn:
0/04203750, prev 0/04203708, desc: CREATE_PTREE , blkref #0: rel
1663/16384/16527 blk 541 FPW
```
Size reduced 8117-> 410 bytes.
WDYT?
--
Best regards,
Kirill Reshke
Attachments:
v2-0002-Better-gin-pg_waldump.patchapplication/octet-stream; name=v2-0002-Better-gin-pg_waldump.patchDownload
From b4c5d6a81d29f097196901ff36bda103dd534a6d Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Mon, 22 Sep 2025 12:47:17 +0000
Subject: [PATCH v2 2/6] Better gin pg_waldump
---
src/backend/access/rmgrdesc/gindesc.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 229675775ff..9d5c944151b 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -150,10 +150,19 @@ gin_desc(StringInfo buf, XLogReaderState *record)
/* no further information */
break;
case XLOG_GIN_UPDATE_META_PAGE:
- /* no further information */
+ {
+ int32 ntuples;
+ ntuples = ((ginxlogUpdateMeta *) rec)->ntuples;
+ appendStringInfo(buf, "ntuples: %d", ntuples);
+ if (ntuples == 0)
+ appendStringInfo(buf, " prevTail: %d newRightLink: %d",
+ ((ginxlogUpdateMeta *) rec)->prevTail,
+ ((ginxlogUpdateMeta *) rec)->newRightlink);
+ }
break;
case XLOG_GIN_INSERT_LISTPAGE:
- /* no further information */
+ appendStringInfo(buf, "ntuples: %d",
+ ((ginxlogInsertListPage *) rec)->ntuples);
break;
case XLOG_GIN_DELETE_LISTPAGE:
appendStringInfo(buf, "ndeleted: %d",
--
2.43.0
v2-0004-Remove-locator.patchapplication/octet-stream; name=v2-0004-Remove-locator.patchDownload
From 0939cdf8710c62400b39df44c6b3abaa4ce41a48 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Fri, 26 Sep 2025 09:32:45 +0000
Subject: [PATCH v2 4/6] Remove locator
---
src/backend/access/gin/ginbtree.c | 1 -
src/backend/access/gin/ginfast.c | 1 -
src/backend/access/gin/ginutil.c | 1 -
src/include/access/ginxlog.h | 1 -
4 files changed, 4 deletions(-)
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 644d484ea53..d1470e362fc 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -476,7 +476,6 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
savedRightLink = GinPageGetOpaque(page)->rightlink;
/* Begin setting up WAL record */
- data.locator = btree->index->rd_locator;
data.flags = xlflags;
if (BufferIsValid(childbuf))
{
diff --git a/src/backend/access/gin/ginfast.c b/src/backend/access/gin/ginfast.c
index a6d88572cc2..446c3cc2994 100644
--- a/src/backend/access/gin/ginfast.c
+++ b/src/backend/access/gin/ginfast.c
@@ -235,7 +235,6 @@ ginHeapTupleFastInsert(GinState *ginstate, GinTupleCollector *collector)
needWal = RelationNeedsWAL(index);
- data.locator = index->rd_locator;
data.ntuples = 0;
data.newRightlink = data.prevTail = InvalidBlockNumber;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index 78f7b7a2495..00c2ede1d33 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -687,7 +687,6 @@ ginUpdateStats(Relation index, const GinStatsData *stats, bool is_build)
XLogRecPtr recptr;
ginxlogUpdateMeta data;
- data.locator = index->rd_locator;
data.ntuples = 0;
data.newRightlink = data.prevTail = InvalidBlockNumber;
memcpy(&data.metadata, metadata, sizeof(GinMetaPageData));
diff --git a/src/include/access/ginxlog.h b/src/include/access/ginxlog.h
index f8c671c2e0d..c3812e49230 100644
--- a/src/include/access/ginxlog.h
+++ b/src/include/access/ginxlog.h
@@ -167,7 +167,6 @@ typedef struct ginxlogDeletePage
*/
typedef struct ginxlogUpdateMeta
{
- RelFileLocator locator;
GinMetaPageData metadata;
BlockNumber prevTail;
BlockNumber newRightlink;
--
2.43.0
v2-0003-fix-oversight-of-631118fe1e8f.patchapplication/octet-stream; name=v2-0003-fix-oversight-of-631118fe1e8f.patchDownload
From 286fb879a5202f590247a90a6e6b9ee4e2d3a46a Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Fri, 26 Sep 2025 07:26:19 +0000
Subject: [PATCH v2 3/6] fix oversight of 631118fe1e8f
---
src/backend/access/gin/ginxlog.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index 4478e928204..fa293ee79d5 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -368,7 +368,6 @@ ginRedoInsert(XLogReaderState *record)
#endif
payload += sizeof(BlockIdData);
rightChildBlkno = BlockIdGetBlockNumber((BlockId) payload);
- payload += sizeof(BlockIdData);
ginRedoClearIncompleteSplit(record, 1);
}
--
2.43.0
v2-0001-Remove-custom-full-page-write-decribption-from-GI.patchapplication/octet-stream; name=v2-0001-Remove-custom-full-page-write-decribption-from-GI.patchDownload
From dce0947fcbfa49fa671b73cfda6ffc3546797df6 Mon Sep 17 00:00:00 2001
From: reshke <reshke@qavm-273b4667.qemu>
Date: Mon, 15 Sep 2025 15:50:14 +0300
Subject: [PATCH v2 1/6] Remove custom full page write decribption from GIN
---
src/backend/access/rmgrdesc/gindesc.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 723ff9499cf..229675775ff 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -99,14 +99,7 @@ gin_desc(StringInfo buf, XLogReaderState *record)
appendStringInfo(buf, " children: %u/%u",
leftChildBlkno, rightChildBlkno);
}
- if (XLogRecHasBlockImage(record, 0))
- {
- if (XLogRecBlockImageApply(record, 0))
- appendStringInfoString(buf, " (full page image)");
- else
- appendStringInfoString(buf, " (full page image, for WAL verification)");
- }
- else
+ if (!XLogRecHasBlockImage(record, 0))
{
char *payload = XLogRecGetBlockData(record, 0, NULL);
@@ -144,14 +137,7 @@ gin_desc(StringInfo buf, XLogReaderState *record)
break;
case XLOG_GIN_VACUUM_DATA_LEAF_PAGE:
{
- if (XLogRecHasBlockImage(record, 0))
- {
- if (XLogRecBlockImageApply(record, 0))
- appendStringInfoString(buf, " (full page image)");
- else
- appendStringInfoString(buf, " (full page image, for WAL verification)");
- }
- else
+ if (!XLogRecHasBlockImage(record, 0))
{
ginxlogVacuumDataLeafPage *xlrec =
(ginxlogVacuumDataLeafPage *) XLogRecGetBlockData(record, 0, NULL);
--
2.43.0
v2-0005-Add-comment-to-ginxlogInsertListPage.patchapplication/octet-stream; name=v2-0005-Add-comment-to-ginxlogInsertListPage.patchDownload
From c1d86fe0a0b9ec3c78b9e7e27daf2f8d112471c7 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Fri, 26 Sep 2025 11:22:04 +0000
Subject: [PATCH v2 5/6] Add comment to ginxlogInsertListPage
---
src/include/access/ginxlog.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/include/access/ginxlog.h b/src/include/access/ginxlog.h
index c3812e49230..56ba80b112c 100644
--- a/src/include/access/ginxlog.h
+++ b/src/include/access/ginxlog.h
@@ -178,6 +178,9 @@ typedef struct ginxlogUpdateMeta
#define XLOG_GIN_INSERT_LISTPAGE 0x70
+/*
+ * Backup Blk 0: list page with inserted tuples
+*/
typedef struct ginxlogInsertListPage
{
BlockNumber rightlink;
--
2.43.0
v2-0006-Remove-custom-logic-for-ginxlogCreatePostingTree-.patchapplication/octet-stream; name=v2-0006-Remove-custom-logic-for-ginxlogCreatePostingTree-.patchDownload
From 19a2bb88383228fc39ee860b990ec8ccc9363c89 Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Fri, 26 Sep 2025 09:31:40 +0000
Subject: [PATCH v2 6/6] Remove custom logic for ginxlogCreatePostingTree,
replace with FPW
---
src/backend/access/gin/gindatapage.c | 9 ++-------
src/backend/access/gin/ginxlog.c | 24 ++++--------------------
src/include/access/ginxlog.h | 5 -----
3 files changed, 6 insertions(+), 32 deletions(-)
diff --git a/src/backend/access/gin/gindatapage.c b/src/backend/access/gin/gindatapage.c
index 6c2c6194720..2b9254d2850 100644
--- a/src/backend/access/gin/gindatapage.c
+++ b/src/backend/access/gin/gindatapage.c
@@ -1839,18 +1839,13 @@ createPostingTree(Relation index, ItemPointerData *items, uint32 nitems,
if (RelationNeedsWAL(index) && !is_build)
{
XLogRecPtr recptr;
- ginxlogCreatePostingTree data;
-
- data.size = rootsize;
XLogBeginInsert();
- XLogRegisterData(&data, sizeof(ginxlogCreatePostingTree));
- XLogRegisterData(GinDataLeafPageGetPostingList(page),
- rootsize);
- XLogRegisterBuffer(0, buffer, REGBUF_WILL_INIT);
+ XLogRegisterBuffer(0, buffer, REGBUF_FORCE_IMAGE | REGBUF_STANDARD);
recptr = XLogInsert(RM_GIN_ID, XLOG_GIN_CREATE_PTREE);
+
PageSetLSN(page, recptr);
}
diff --git a/src/backend/access/gin/ginxlog.c b/src/backend/access/gin/ginxlog.c
index fa293ee79d5..a48557a62b1 100644
--- a/src/backend/access/gin/ginxlog.c
+++ b/src/backend/access/gin/ginxlog.c
@@ -43,28 +43,12 @@ ginRedoClearIncompleteSplit(XLogReaderState *record, uint8 block_id)
static void
ginRedoCreatePTree(XLogReaderState *record)
{
- XLogRecPtr lsn = record->EndRecPtr;
- ginxlogCreatePostingTree *data = (ginxlogCreatePostingTree *) XLogRecGetData(record);
- char *ptr;
- Buffer buffer;
- Page page;
-
- buffer = XLogInitBufferForRedo(record, 0);
- page = BufferGetPage(buffer);
-
- GinInitBuffer(buffer, GIN_DATA | GIN_LEAF | GIN_COMPRESSED);
+ Buffer buf;
- ptr = XLogRecGetData(record) + sizeof(ginxlogCreatePostingTree);
+ if (XLogReadBufferForRedo(record, 0, &buf) != BLK_RESTORED)
+ elog(ERROR, "GIN create posting tree record did not contain a full-page image");
- /* Place page data */
- memcpy(GinDataLeafPageGetPostingList(page), ptr, data->size);
-
- GinDataPageSetDataSize(page, data->size);
-
- PageSetLSN(page, lsn);
-
- MarkBufferDirty(buffer);
- UnlockReleaseBuffer(buffer);
+ UnlockReleaseBuffer(buf);
}
static void
diff --git a/src/include/access/ginxlog.h b/src/include/access/ginxlog.h
index 56ba80b112c..9c98ba1f264 100644
--- a/src/include/access/ginxlog.h
+++ b/src/include/access/ginxlog.h
@@ -18,11 +18,6 @@
#define XLOG_GIN_CREATE_PTREE 0x10
-typedef struct ginxlogCreatePostingTree
-{
- uint32 size;
- /* A compressed posting list follows */
-} ginxlogCreatePostingTree;
/*
* The format of the insertion record varies depending on the page type.
--
2.43.0
On 26 Sep 2025, at 16:39, Kirill Reshke <reshkekirill@gmail.com> wrote:
WDYT?
<v2-0002-Better-gin-pg_waldump.patch><v2-0004-Remove-locator.patch><v2-0003-fix-oversight-of-631118fe1e8f.patch><v2-0001-Remove-custom-full-page-write-decribption-from-GI.patch><v2-0005-Add-comment-to-ginxlogInsertListPage.patch><v2-0006-Remove-custom-logic-for-ginxlogCreatePostingTree-.patch>
Hi!
I think this thread should stick to patches 1 and 2. Which look good to me, but need few words in commit messages.
Patches 3, 4 and 5 deserve their own thread about GIN code beautification, but may be reviewed here. But perhaps, it's OK for them to live here too. They also look correct to me, but, again, lack appropriate commit messages.
Patch 6 is significant improvement and for sure must be discussed in another thread. The idea seems straightforward to me, but it's WAL logging, there might be some unforeseen consequences of any small change.
Best regards, Andrey Borodin.
On Tue, Sep 30, 2025 at 05:59:49PM +0500, Andrey Borodin wrote:
I think this thread should stick to patches 1 and 2. Which look good
to me, but need few words in commit messages.
With XLogRecGetBlockRefInfo() already doing the same job with much
more information about the block, 0001 makes sense.
+ int32 ntuples;
+ ntuples = ((ginxlogUpdateMeta *) rec)->ntuples;
+ appendStringInfo(buf, "ntuples: %d", ntuples);In 0002. We have a routine for this type of assignments:
XLogRecGetBlockData(). Let's use it here for clarity.
prevTail and newRightlink are block numbers, meaning %u not %d when
printed.
0001 and 0002 can just be merged together.
Patches 3, 4 and 5 deserve their own thread about GIN code
beautification, but may be reviewed here. But perhaps, it's OK for
them to live here too. They also look correct to me, but, again,
lack appropriate commit messages.
In 0003, "payload" is local to its block or code in ginRedoInsert(),
still it's true that the extra assignment is useless.
I don't think that I'm going to agree with 0004, this removes a
RelFileLocator that could be useful.
0005 looks OK.
Patch 6 is significant improvement and for sure must be discussed in
another thread. The idea seems straightforward to me, but it's WAL
logging, there might be some unforeseen consequences of any small
change.
Yep, not sure that I see the full benefit of what you are doing here.
A thread describing problems around the WAL descriptions is not suited
for what you are qualifying as an optimization. Same for 0004. 0003
is small enough that I would not bother with a different thread, now
that you've posted a patch..
--
Michael
On Wed, Oct 01, 2025 at 03:39:35PM +0900, Michael Paquier wrote:
Yep, not sure that I see the full benefit of what you are doing here.
A thread describing problems around the WAL descriptions is not suited
for what you are qualifying as an optimization. Same for 0004. 0003
is small enough that I would not bother with a different thread, now
that you've posted a patch..
Worth a note. I had some time and applied patches 0001, 0003 and
0005, as they are worth their own.
--
Michael
On Wed, Oct 01, 2025 at 03:39:35PM +0900, Michael Paquier wrote:
+ int32 ntuples; + ntuples = ((ginxlogUpdateMeta *) rec)->ntuples; + appendStringInfo(buf, "ntuples: %d", ntuples);In 0002. We have a routine for this type of assignments:
XLogRecGetBlockData(). Let's use it here for clarity.
This should be XLogRecGetData(), not XLogRecGetBlockData(), as the
data is assigned to the record. My apologies for the mistake.
--
Michael
On Tue, 7 Oct 2025 at 11:46, Michael Paquier <michael@paquier.xyz> wrote:
On Wed, Oct 01, 2025 at 03:39:35PM +0900, Michael Paquier wrote:
+ int32 ntuples; + ntuples = ((ginxlogUpdateMeta *) rec)->ntuples; + appendStringInfo(buf, "ntuples: %d", ntuples);In 0002. We have a routine for this type of assignments:
XLogRecGetBlockData(). Let's use it here for clarity.This should be XLogRecGetData(), not XLogRecGetBlockData(), as the
data is assigned to the record. My apologies for the mistake.
--
Michael
Yes, thank you.
Turns out we already use XLogRecGetData in assignment: char *rec =
XLogRecGetData(record). But I still did alter v2 patch a bit,
introducing new variable xlrec, to avoid clunky casts between char *
and ginxlogUpdateMeta.
FPA v3.
--
Best regards,
Kirill Reshke
Attachments:
v3-0001-Better-gin-pg_waldump.patchapplication/octet-stream; name=v3-0001-Better-gin-pg_waldump.patchDownload
From 99dd310fe8c53d3e400bff68e289c19def832feb Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Mon, 22 Sep 2025 12:47:17 +0000
Subject: [PATCH v3] Better gin pg_waldump
---
src/backend/access/rmgrdesc/gindesc.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 229675775ff..69994cb4593 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -150,10 +150,20 @@ gin_desc(StringInfo buf, XLogReaderState *record)
/* no further information */
break;
case XLOG_GIN_UPDATE_META_PAGE:
- /* no further information */
+ {
+ int32 ntuples;
+ ginxlogUpdateMeta *xlrec = (ginxlogUpdateMeta *) rec;
+ ntuples = xlrec->ntuples;
+ appendStringInfo(buf, "ntuples: %d", ntuples);
+ if (ntuples == 0)
+ appendStringInfo(buf, " prevTail: %u newRightLink: %u",
+ xlrec->prevTail,
+ xlrec->newRightlink);
+ }
break;
case XLOG_GIN_INSERT_LISTPAGE:
- /* no further information */
+ appendStringInfo(buf, "ntuples: %d",
+ ((ginxlogInsertListPage *) rec)->ntuples);
break;
case XLOG_GIN_DELETE_LISTPAGE:
appendStringInfo(buf, "ndeleted: %d",
--
2.43.0
On Tue, Oct 07, 2025 at 02:08:02PM +0500, Kirill Reshke wrote:
Turns out we already use XLogRecGetData in assignment: char *rec =
XLogRecGetData(record). But I still did alter v2 patch a bit,
introducing new variable xlrec, to avoid clunky casts between char *
and ginxlogUpdateMeta.
For UPDATE_META_PAGE, ntuples == 0 could also mean that we may show
a lot of invalid block numbers, as well, which feels a bit pointless.
I have switched that to check InvalidBlockNumber instead.
While reviewing the whole, I have noticed that rightlink was missing
for INSERT_LISTPAGE, as well as the right/left children pages for
SPLIT. I have added this information, applied the result. Thanks!
--
Michael
On Wed, 8 Oct 2025 at 10:14, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Oct 07, 2025 at 02:08:02PM +0500, Kirill Reshke wrote:
Turns out we already use XLogRecGetData in assignment: char *rec =
XLogRecGetData(record). But I still did alter v2 patch a bit,
introducing new variable xlrec, to avoid clunky casts between char *
and ginxlogUpdateMeta.For UPDATE_META_PAGE, ntuples == 0 could also mean that we may show
a lot of invalid block numbers, as well, which feels a bit pointless.
I have switched that to check InvalidBlockNumber instead.
Thank you for pushing!
While reviewing the whole, I have noticed that rightlink was missing
for INSERT_LISTPAGE, as well as the right/left children pages for
SPLIT. I have added this information, applied the result. Thanks!
After this change, I was wondering what is the purpose of
leftChildBlkno/rightChildBlkno fields of SPLIT record.
Turns out all of ginxlogSplit's fields except flags are of no use.
Should we remove them, reducing overall cognitive complexity of GIN
internals and reducing WAL footprint?
PFA patch for that. If this topic is too out-of-scope of this thread,
I can start another one, just let me know.
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Remove-all-fields-from-ginxlogSplit-except-flags.patchapplication/octet-stream; name=v1-0001-Remove-all-fields-from-ginxlogSplit-except-flags.patchDownload
From c98d5da9801e899c6fd2d1f1a61c87ed38f5d4ce Mon Sep 17 00:00:00 2001
From: reshke <reshke@double.cloud>
Date: Thu, 9 Oct 2025 12:27:37 +0000
Subject: [PATCH v1] Remove all fields from ginxlogSplit except flags
---
src/backend/access/gin/ginbtree.c | 10 ----------
src/backend/access/rmgrdesc/gindesc.c | 3 ---
src/include/access/ginxlog.h | 5 -----
3 files changed, 18 deletions(-)
diff --git a/src/backend/access/gin/ginbtree.c b/src/backend/access/gin/ginbtree.c
index 644d484ea53..0150de8d492 100644
--- a/src/backend/access/gin/ginbtree.c
+++ b/src/backend/access/gin/ginbtree.c
@@ -476,15 +476,7 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
savedRightLink = GinPageGetOpaque(page)->rightlink;
/* Begin setting up WAL record */
- data.locator = btree->index->rd_locator;
data.flags = xlflags;
- if (BufferIsValid(childbuf))
- {
- data.leftChildBlkno = BufferGetBlockNumber(childbuf);
- data.rightChildBlkno = GinPageGetOpaque(childpage)->rightlink;
- }
- else
- data.leftChildBlkno = data.rightChildBlkno = InvalidBlockNumber;
if (stack->parent == NULL)
{
@@ -503,7 +495,6 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
buildStats->nEntryPages++;
}
- data.rrlink = InvalidBlockNumber;
data.flags |= GIN_SPLIT_ROOT;
GinPageGetOpaque(newrpage)->rightlink = InvalidBlockNumber;
@@ -537,7 +528,6 @@ ginPlaceToPage(GinBtree btree, GinBtreeStack *stack,
else
{
/* splitting a non-root page */
- data.rrlink = savedRightLink;
GinPageGetOpaque(newrpage)->rightlink = savedRightLink;
GinPageGetOpaque(newlpage)->flags |= GIN_INCOMPLETE_SPLIT;
diff --git a/src/backend/access/rmgrdesc/gindesc.c b/src/backend/access/rmgrdesc/gindesc.c
index 075c4a0ae93..acbc04027c3 100644
--- a/src/backend/access/rmgrdesc/gindesc.c
+++ b/src/backend/access/rmgrdesc/gindesc.c
@@ -130,9 +130,6 @@ gin_desc(StringInfo buf, XLogReaderState *record)
appendStringInfo(buf, " isdata: %c isleaf: %c",
(xlrec->flags & GIN_INSERT_ISDATA) ? 'T' : 'F',
(xlrec->flags & GIN_INSERT_ISLEAF) ? 'T' : 'F');
- if (xlrec->leftChildBlkno != InvalidBlockNumber)
- appendStringInfo(buf, " children: %u/%u",
- xlrec->leftChildBlkno, xlrec->rightChildBlkno);
}
break;
case XLOG_GIN_VACUUM_PAGE:
diff --git a/src/include/access/ginxlog.h b/src/include/access/ginxlog.h
index 98760bf6ee4..b4bc6d8107f 100644
--- a/src/include/access/ginxlog.h
+++ b/src/include/access/ginxlog.h
@@ -110,11 +110,6 @@ typedef struct
typedef struct ginxlogSplit
{
- RelFileLocator locator;
- BlockNumber rrlink; /* right link, or root's blocknumber if root
- * split */
- BlockNumber leftChildBlkno; /* valid on a non-leaf split */
- BlockNumber rightChildBlkno;
uint16 flags; /* see below */
} ginxlogSplit;
--
2.43.0
On 9 Oct 2025, at 17:33, Kirill Reshke <reshkekirill@gmail.com> wrote:
Should we remove them, reducing overall cognitive complexity of GIN
internals and reducing WAL footprint?
The patch does not add a single line... that's impressive :)
Why not wipe ginxlogSplit entirely? Will the code be clearer with XLogRegisterData(&flags, sizeof(uint16))?
Best regards, Andrey Borodin.
Hi,
On Fri, Oct 10, 2025 at 10:00 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 9 Oct 2025, at 17:33, Kirill Reshke <reshkekirill@gmail.com> wrote:
Should we remove them, reducing overall cognitive complexity of GIN
internals and reducing WAL footprint?The patch does not add a single line... that's impressive :)
Why not wipe ginxlogSplit entirely? Will the code be clearer with XLogRegisterData(&flags, sizeof(uint16))?
Looks like we will not be able to process old split records after
this, as 'flags' field offset was changed. So probably these fields
are for backward compatibility. Does it make sense?
Best regards,
Arseniy Mukhin
On Tue, 14 Oct 2025, 01:24 Arseniy Mukhin, <arseniy.mukhin.dev@gmail.com>
wrote:
Hi,
On Fri, Oct 10, 2025 at 10:00 PM Andrey Borodin <x4mmm@yandex-team.ru>
wrote:On 9 Oct 2025, at 17:33, Kirill Reshke <reshkekirill@gmail.com> wrote:
Should we remove them, reducing overall cognitive complexity of GIN
internals and reducing WAL footprint?The patch does not add a single line... that's impressive :)
Why not wipe ginxlogSplit entirely? Will the code be clearer with
XLogRegisterData(&flags, sizeof(uint16))?
Looks like we will not be able to process old split records after
this, as 'flags' field offset was changed. So probably these fields
are for backward compatibility. Does it make sense?Best regards,
Arseniy Mukhin
Hi! We do not need to support anything WAL related in new major version,
since we do new initdb. There are couple of threads nearby that change WAL
record layout or even drop them entirely, and that OK.
Also, we have WAL magic number for this purpose
Show quoted text
On Mon, Oct 13, 2025 at 11:59 PM Kirill Reshke <reshkekirill@gmail.com> wrote:
On Tue, 14 Oct 2025, 01:24 Arseniy Mukhin, <arseniy.mukhin.dev@gmail.com> wrote:
Hi,
On Fri, Oct 10, 2025 at 10:00 PM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
On 9 Oct 2025, at 17:33, Kirill Reshke <reshkekirill@gmail.com> wrote:
Should we remove them, reducing overall cognitive complexity of GIN
internals and reducing WAL footprint?The patch does not add a single line... that's impressive :)
Why not wipe ginxlogSplit entirely? Will the code be clearer with XLogRegisterData(&flags, sizeof(uint16))?
Looks like we will not be able to process old split records after
this, as 'flags' field offset was changed. So probably these fields
are for backward compatibility. Does it make sense?Best regards,
Arseniy MukhinHi! We do not need to support anything WAL related in new major version, since we do new initdb. There are couple of threads nearby that change WAL record layout or even drop them entirely, and that OK.
Also, we have WAL magic number for this purpose
I see, sorry for the noise then and thanks for the explanation!
Best regards,
Arseniy Mukhin