From 44bfa575f5f28d1a8093e02b4a7993c1368cbeb5 Mon Sep 17 00:00:00 2001
From: Peter Geoghegan <pg@bowt.ie>
Date: Sat, 8 Apr 2023 18:59:09 -0700
Subject: [PATCH v1] Fix WAL description of posting list updates.

---
 src/include/access/rmgrdesc_utils.h          |  41 ++++++-
 src/backend/access/rmgrdesc/heapdesc.c       |   2 +-
 src/backend/access/rmgrdesc/nbtdesc.c        | 121 ++++++++++---------
 src/backend/access/rmgrdesc/rmgrdesc_utils.c |  43 +------
 doc/src/sgml/pgwalinspect.sgml               |  34 +++---
 5 files changed, 123 insertions(+), 118 deletions(-)

diff --git a/src/include/access/rmgrdesc_utils.h b/src/include/access/rmgrdesc_utils.h
index 13552d6f3..16916e3e8 100644
--- a/src/include/access/rmgrdesc_utils.h
+++ b/src/include/access/rmgrdesc_utils.h
@@ -12,12 +12,49 @@
 #ifndef RMGRDESC_UTILS_H_
 #define RMGRDESC_UTILS_H_
 
+/*
+ * Guidelines for rmgr descriptor routine authors:
+ *
+ * The goal of these guidelines is to avoid gratuitous inconsistencies across
+ * each rmgr, and to allow users to parse desc output strings without too much
+ * difficulty.  This is not an API specification or an interchange format.
+ * (Only heapam and nbtree desc routines follow these guidelines at present,
+ * in any case.)
+ *
+ * Record descriptions are similar to JSON style key/value objects.  However,
+ * there is no explicit "string" type/string escaping.  Top-level { } brackets
+ * should be omitted.  For example:
+ *
+ * snapshotConflictHorizon: 0, flags: 0x03
+ *
+ * Record descriptions may contain variable-length arrays.  For example:
+ *
+ * nunused: 5, unused: [1, 2, 3, 4, 5]
+ *
+ * Nested objects are supported via { } brackets.  They generally appear
+ * inside variable-length arrays.  For example:
+ *
+ * ndeleted: 0, nupdated: 1, deleted: [], updated: [{ off: 45, nptids: 1, ptids: [0] }]
+ *
+ * Try to output things in an order that faithfully represents the order of
+ * things in the physical WAL record struct.  It's a good idea if the number
+ * of items in the array appears before the array.
+ *
+ * It's okay for individual WAL record types to invent their own conventions.
+ * For example, heapam's PRUNE records output the follow representation of
+ * redirects:
+ *
+ * ... redirected: [39->46], ...
+ *
+ * Arguably the PRUNE desc routine should be using object notation instead.
+ * This ad-hoc representation of redirects has the advantage of being terse in
+ * a context where that might matter a lot.
+ */
 extern void array_desc(StringInfo buf, void *array, size_t elem_size, int count,
 					   void (*elem_desc) (StringInfo buf, void *elem, void *data),
 					   void *data);
 extern void offset_elem_desc(StringInfo buf, void *offset, void *data);
 extern void redirect_elem_desc(StringInfo buf, void *offset, void *data);
-extern void relid_desc(StringInfo buf, void *relid, void *data);
-extern void uint16_elem_desc(StringInfo buf, void *value, void *data);
+extern void oid_elem_desc(StringInfo buf, void *relid, void *data);
 
 #endif							/* RMGRDESC_UTILS_H */
diff --git a/src/backend/access/rmgrdesc/heapdesc.c b/src/backend/access/rmgrdesc/heapdesc.c
index 6dfd7d7d1..3bd083875 100644
--- a/src/backend/access/rmgrdesc/heapdesc.c
+++ b/src/backend/access/rmgrdesc/heapdesc.c
@@ -127,7 +127,7 @@ heap_desc(StringInfo buf, XLogReaderState *record)
 		appendStringInfo(buf, ", nrelids: %u", xlrec->nrelids);
 		appendStringInfoString(buf, ", relids:");
 		array_desc(buf, xlrec->relids, sizeof(Oid), xlrec->nrelids,
-				   &relid_desc, NULL);
+				   &oid_elem_desc, NULL);
 	}
 	else if (info == XLOG_HEAP_CONFIRM)
 	{
diff --git a/src/backend/access/rmgrdesc/nbtdesc.c b/src/backend/access/rmgrdesc/nbtdesc.c
index 6bc043265..6edb02a0b 100644
--- a/src/backend/access/rmgrdesc/nbtdesc.c
+++ b/src/backend/access/rmgrdesc/nbtdesc.c
@@ -17,59 +17,8 @@
 #include "access/nbtxlog.h"
 #include "access/rmgrdesc_utils.h"
 
-static void btree_del_desc(StringInfo buf, char *block_data, uint16 ndeleted,
-						   uint16 nupdated);
-static void btree_update_elem_desc(StringInfo buf, void *update, void *data);
-
-static void
-btree_del_desc(StringInfo buf, char *block_data, uint16 ndeleted,
-			   uint16 nupdated)
-{
-	OffsetNumber *updatedoffsets;
-	xl_btree_update *updates;
-	OffsetNumber *data = (OffsetNumber *) block_data;
-
-	appendStringInfoString(buf, ", deleted:");
-	array_desc(buf, data, sizeof(OffsetNumber), ndeleted,
-			   &offset_elem_desc, NULL);
-
-	appendStringInfoString(buf, ", updated:");
-	array_desc(buf, data, sizeof(OffsetNumber), nupdated,
-			   &offset_elem_desc, NULL);
-
-	if (nupdated <= 0)
-		return;
-
-	updatedoffsets = (OffsetNumber *)
-		((char *) data + ndeleted * sizeof(OffsetNumber));
-	updates = (xl_btree_update *) ((char *) updatedoffsets +
-								   nupdated *
-								   sizeof(OffsetNumber));
-
-	appendStringInfoString(buf, ", updates:");
-	array_desc(buf, updates, sizeof(xl_btree_update),
-			   nupdated, &btree_update_elem_desc,
-			   &updatedoffsets);
-}
-
-static void
-btree_update_elem_desc(StringInfo buf, void *update, void *data)
-{
-	xl_btree_update *new_update = (xl_btree_update *) update;
-	OffsetNumber *updated_offset = *((OffsetNumber **) data);
-
-	appendStringInfo(buf, "{ updated offset: %u, ndeleted tids: %u",
-					 *updated_offset, new_update->ndeletedtids);
-
-	appendStringInfoString(buf, ", deleted tids:");
-
-	array_desc(buf, (char *) new_update + SizeOfBtreeUpdate, sizeof(uint16),
-			   new_update->ndeletedtids, &uint16_elem_desc, NULL);
-
-	updated_offset++;
-
-	appendStringInfo(buf, " }");
-}
+static void delvacuum_desc(StringInfo buf, char *block_data,
+						   uint16 ndeleted, uint16 nupdated);
 
 void
 btree_desc(StringInfo buf, XLogReaderState *record)
@@ -114,9 +63,8 @@ btree_desc(StringInfo buf, XLogReaderState *record)
 								 xlrec->ndeleted, xlrec->nupdated);
 
 				if (!XLogRecHasBlockImage(record, 0))
-					btree_del_desc(buf, XLogRecGetBlockData(record, 0, NULL),
+					delvacuum_desc(buf, XLogRecGetBlockData(record, 0, NULL),
 								   xlrec->ndeleted, xlrec->nupdated);
-
 				break;
 			}
 		case XLOG_BTREE_DELETE:
@@ -128,16 +76,15 @@ btree_desc(StringInfo buf, XLogReaderState *record)
 								 xlrec->ndeleted, xlrec->nupdated);
 
 				if (!XLogRecHasBlockImage(record, 0))
-					btree_del_desc(buf, XLogRecGetBlockData(record, 0, NULL),
+					delvacuum_desc(buf, XLogRecGetBlockData(record, 0, NULL),
 								   xlrec->ndeleted, xlrec->nupdated);
-
 				break;
 			}
 		case XLOG_BTREE_MARK_PAGE_HALFDEAD:
 			{
 				xl_btree_mark_page_halfdead *xlrec = (xl_btree_mark_page_halfdead *) rec;
 
-				appendStringInfo(buf, "topparent: %u; leaf: %u; left: %u; right: %u",
+				appendStringInfo(buf, "topparent: %u, leaf: %u, left: %u, right: %u",
 								 xlrec->topparent, xlrec->leafblk, xlrec->leftblk, xlrec->rightblk);
 				break;
 			}
@@ -146,11 +93,11 @@ btree_desc(StringInfo buf, XLogReaderState *record)
 			{
 				xl_btree_unlink_page *xlrec = (xl_btree_unlink_page *) rec;
 
-				appendStringInfo(buf, "left: %u; right: %u; level: %u; safexid: %u:%u; ",
+				appendStringInfo(buf, "left: %u, right: %u, level: %u, safexid: %u:%u, ",
 								 xlrec->leftsib, xlrec->rightsib, xlrec->level,
 								 EpochFromFullTransactionId(xlrec->safexid),
 								 XidFromFullTransactionId(xlrec->safexid));
-				appendStringInfo(buf, "leafleft: %u; leafright: %u; leaftopparent: %u",
+				appendStringInfo(buf, "leafleft: %u, leafright: %u, leaftopparent: %u",
 								 xlrec->leafleftsib, xlrec->leafrightsib,
 								 xlrec->leaftopparent);
 				break;
@@ -242,3 +189,57 @@ btree_identify(uint8 info)
 
 	return id;
 }
+
+static void
+delvacuum_desc(StringInfo buf, char *block_data,
+			   uint16 ndeleted, uint16 nupdated)
+{
+	OffsetNumber *deletedoffsets;
+	OffsetNumber *updatedoffsets;
+	xl_btree_update *updates;
+
+	/* Output deleted page offset number array */
+	appendStringInfoString(buf, ", deleted:");
+	deletedoffsets = (OffsetNumber *) block_data;
+	array_desc(buf, deletedoffsets, sizeof(OffsetNumber), ndeleted,
+			   &offset_elem_desc, NULL);
+
+	/*
+	 * Output updates as an array of "update objects", where each element
+	 * contains a page offset number from updated array.  (This is not the
+	 * most literal representation of the underlying physical data structure
+	 * that we could use.  Readability seems more important here.)
+	 */
+	appendStringInfoString(buf, ", updated: [");
+	updatedoffsets = (OffsetNumber *) (block_data + ndeleted *
+									   sizeof(OffsetNumber));
+	updates = (xl_btree_update *) ((char *) updatedoffsets +
+								   nupdated *
+								   sizeof(OffsetNumber));
+	for (int i = 0; i < nupdated; i++)
+	{
+		OffsetNumber off = updatedoffsets[i];
+
+		appendStringInfo(buf, "{ off: %u, nptids: %u, ptids: [",
+						 off, updates->ndeletedtids);
+		Assert(updates->ndeletedtids > 0);
+		for (int p = 0; p < updates->ndeletedtids; p++)
+		{
+			uint16 *ptid;
+
+			ptid = (uint16 *) ((char *) updates + SizeOfBtreeUpdate) + p;
+			appendStringInfo(buf, "%u", *ptid);
+
+			if (p < updates->ndeletedtids - 1)
+				appendStringInfoString(buf, ", ");
+		}
+		appendStringInfoString(buf, "] }");
+		if (i < nupdated - 1)
+			appendStringInfoString(buf, ", ");
+
+		updates = (xl_btree_update *)
+			((char *) updates + SizeOfBtreeUpdate +
+			 updates->ndeletedtids * sizeof(uint16));
+	}
+	appendStringInfoString(buf, "]");
+}
diff --git a/src/backend/access/rmgrdesc/rmgrdesc_utils.c b/src/backend/access/rmgrdesc/rmgrdesc_utils.c
index c95a41a25..90051f0df 100644
--- a/src/backend/access/rmgrdesc/rmgrdesc_utils.c
+++ b/src/backend/access/rmgrdesc/rmgrdesc_utils.c
@@ -16,31 +16,6 @@
 #include "access/rmgrdesc_utils.h"
 #include "storage/off.h"
 
-/*
- * Guidelines for formatting desc functions:
- *
- * member1_name: member1_value, member2_name: member2_value
- *
- * If the value is a list, please use:
- *
- * member3_name: [ member3_list_value1, member3_list_value2 ]
- *
- * The first item appended to the string should not be prepended by any spaces
- * or comma, however all subsequent appends to the string are responsible for
- * prepending themselves with a comma followed by a space.
- *
- * Arrays should have a space between the opening square bracket and first
- * element and between the last element and closing brace.
- *
- * Flags should be in ALL CAPS.
- *
- * For lists/arrays of items, the number of those items should be listed at
- * the beginning with all of the other numbers.
- *
- * List punctuation should still be included even if there are 0 items.
- *
- * Composite objects in a list should be surrounded with { }.
- */
 void
 array_desc(StringInfo buf, void *array, size_t elem_size, int count,
 		   void (*elem_desc) (StringInfo buf, void *elem, void *data),
@@ -51,16 +26,14 @@ array_desc(StringInfo buf, void *array, size_t elem_size, int count,
 		appendStringInfoString(buf, " []");
 		return;
 	}
-	appendStringInfo(buf, " [");
+	appendStringInfoString(buf, " [");
 	for (int i = 0; i < count; i++)
 	{
-		if (i > 0)
-			appendStringInfoString(buf, ",");
-		appendStringInfoString(buf, " ");
-
 		elem_desc(buf, (char *) array + elem_size * i, data);
+		if (i < count - 1)
+			appendStringInfoString(buf, ", ");
 	}
-	appendStringInfoString(buf, " ]");
+	appendStringInfoString(buf, "]");
 }
 
 void
@@ -78,13 +51,7 @@ redirect_elem_desc(StringInfo buf, void *offset, void *data)
 }
 
 void
-relid_desc(StringInfo buf, void *relid, void *data)
+oid_elem_desc(StringInfo buf, void *relid, void *data)
 {
 	appendStringInfo(buf, "%u", *(Oid *) relid);
 }
-
-void
-uint16_elem_desc(StringInfo buf, void *value, void *data)
-{
-	appendStringInfo(buf, "%u", *(uint16 *) value);
-}
diff --git a/doc/src/sgml/pgwalinspect.sgml b/doc/src/sgml/pgwalinspect.sgml
index b3712be00..a1a4422fb 100644
--- a/doc/src/sgml/pgwalinspect.sgml
+++ b/doc/src/sgml/pgwalinspect.sgml
@@ -71,19 +71,19 @@
       after the <replaceable>in_lsn</replaceable> argument.  For
       example:
 <screen>
-postgres=# SELECT * FROM pg_get_wal_record_info('0/E84F5E8');
--[ RECORD 1 ]----+--------------------------------------------------
-start_lsn        | 0/E84F5E8
-end_lsn          | 0/E84F620
-prev_lsn         | 0/E84F5A8
+postgres=# SELECT * FROM pg_get_wal_record_info('0/E419E28');
+-[ RECORD 1 ]----+-------------------------------------------------
+start_lsn        | 0/E419E28
+end_lsn          | 0/E419E68
+prev_lsn         | 0/E419D78
 xid              | 0
 resource_manager | Heap2
 record_type      | VACUUM
-record_length    | 50
+record_length    | 58
 main_data_length | 2
 fpi_length       | 0
-description      | nunused: 1, unused: [ 22 ]
-block_ref        | blkref #0: rel 1663/16389/20884 fork main blk 126
+description      | nunused: 5, unused: [1, 2, 3, 4, 5]
+block_ref        | blkref #0: rel 1663/16385/1249 fork main blk 364
 </screen>
      </para>
      <para>
@@ -144,18 +144,18 @@ block_ref        |
       references.  Returns one row per block reference per WAL record.
       For example:
 <screen>
-postgres=# SELECT * FROM pg_get_wal_block_info('0/10E9D80', '0/10E9DC0');
+postgres=# SELECT * FROM pg_get_wal_block_info('0/1230278', '0/12302B8');
 -[ RECORD 1 ]-----+-----------------------------------
-start_lsn         | 0/10E9D80
-end_lsn           | 0/10E9DC0
-prev_lsn          | 0/10E9860
+start_lsn         | 0/1230278
+end_lsn           | 0/12302B8
+prev_lsn          | 0/122FD40
 block_id          | 0
 reltablespace     | 1663
 reldatabase       | 1
-relfilenode       | 2690
+relfilenode       | 2658
 relforknumber     | 0
-relblocknumber    | 5
-xid               | 117
+relblocknumber    | 11
+xid               | 341
 resource_manager  | Btree
 record_type       | INSERT_LEAF
 record_length     | 64
@@ -163,8 +163,8 @@ main_data_length  | 2
 block_data_length | 16
 block_fpi_length  | 0
 block_fpi_info    |
-description       | off 14
-block_data        | \x00005400020010001407000000000000
+description       | off: 46
+block_data        | \x00002a00070010402630000070696400
 block_fpi_data    |
 </screen>
      </para>
-- 
2.40.0

