On Sat, Mar 18, 2023 at 1:06 AM Peter Geoghegan <p...@bowt.ie> wrote:
>
> On Fri, Mar 17, 2023 at 12:20 AM Bharath Rupireddy
> <bharath.rupireddyforpostg...@gmail.com> wrote:
> > +1 for pg_get_wal_block_info emitting per-record WAL info too along
> > with block info, attached v2 patch does that. IMO, usability wins the
> > race here.
>
> I think that this direction makes a lot of sense. Under this scheme,
> we still have pg_get_wal_records_info(), which is more or less an SQL
> interface to the information that pg_waldump presents by default.
> That's important when the record view of things is of primary
> importance. But now you also have a "block oriented" view of WAL
> presented by pg_get_wal_block_info(), which is useful when particular
> blocks are of primary interest. I think that I'll probably end up
> using both, while primarily using pg_get_wal_block_info() for more
> advanced analysis that focuses on what happened to particular blocks
> over time.

Hm.

> It makes sense to present pg_get_wal_block_info() immediately after
> pg_get_wal_records_info() in the documentation under this scheme,
> since they're closely related.

-1. I don't think we need that and even if we did, it's hard to
maintain that ordering in future. One who knows to use these functions
will anyway get to know how they're related.

> (Checks pg_walinspect once more...)
>
> Actually, I now see that block_ref won't be NULL for those records
> that have no block references at all -- it just outputs an empty
> string.

Yes, that's unnecessary.

> But wouldn't it be better if it actually output NULL?

+1 done so in the attached 0001 patch.

> Better
> for its own sake, but also better because doing so enables describing
> the relationship between the two functions with reference to
> block_ref. It seems particularly helpful to me to be able to say that
> pg_get_wal_block_info() doesn't show anything for precisely those WAL
> records whose block_ref is NULL according to
> pg_get_wal_records_info().

Hm.

Attaching v3 patch set - 0001 optimizations around block references,
0002 enables pg_get_wal_block_info() to emit per-record info. Any
thoughts?

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From 153d1d8ae7f03471aa74b685d20eb110ba7eda98 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 18 Mar 2023 04:01:46 +0000
Subject: [PATCH v3] Few optimizations around block references in pg_walinspect

---
 contrib/pg_walinspect/pg_walinspect.c | 29 +++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/contrib/pg_walinspect/pg_walinspect.c b/contrib/pg_walinspect/pg_walinspect.c
index 3b3215daf5..c3ed31db8f 100644
--- a/contrib/pg_walinspect/pg_walinspect.c
+++ b/contrib/pg_walinspect/pg_walinspect.c
@@ -199,9 +199,12 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
 	initStringInfo(&rec_desc);
 	desc.rm_desc(&rec_desc, record);
 
-	/* Block references. */
-	initStringInfo(&rec_blk_ref);
-	XLogRecGetBlockRefInfo(record, false, true, &rec_blk_ref, &fpi_len);
+	/* Get block references, if any. */
+	if (XLogRecHasAnyBlockRefs(record))
+	{
+		initStringInfo(&rec_blk_ref);
+		XLogRecGetBlockRefInfo(record, false, true, &rec_blk_ref, &fpi_len);
+	}
 
 	main_data_len = XLogRecGetDataLen(record);
 
@@ -215,7 +218,12 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
 	values[i++] = UInt32GetDatum(main_data_len);
 	values[i++] = UInt32GetDatum(fpi_len);
 	values[i++] = CStringGetTextDatum(rec_desc.data);
-	values[i++] = CStringGetTextDatum(rec_blk_ref.data);
+
+	/* Output block references, if any. */
+	if (XLogRecHasAnyBlockRefs(record))
+		values[i++] = CStringGetTextDatum(rec_blk_ref.data);
+	else
+		nulls[i++] = true;
 
 	Assert(i == ncols);
 }
@@ -377,6 +385,12 @@ pg_get_wal_block_info(PG_FUNCTION_ARGS)
 	while (ReadNextXLogRecord(xlogreader) &&
 		   xlogreader->EndRecPtr <= end_lsn)
 	{
+		CHECK_FOR_INTERRUPTS();
+
+		/* Get block references, if any, otherwise continue. */
+		if (!XLogRecHasAnyBlockRefs(xlogreader))
+			continue;
+
 		/* Use the tmp context so we can clean up after each tuple is done */
 		old_cxt = MemoryContextSwitchTo(tmp_cxt);
 
@@ -385,8 +399,6 @@ pg_get_wal_block_info(PG_FUNCTION_ARGS)
 		/* clean up and switch back */
 		MemoryContextSwitchTo(old_cxt);
 		MemoryContextReset(tmp_cxt);
-
-		CHECK_FOR_INTERRUPTS();
 	}
 
 	MemoryContextDelete(tmp_cxt);
@@ -483,8 +495,6 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
 #define PG_GET_WAL_RECORDS_INFO_COLS 11
 	XLogReaderState *xlogreader;
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
-	Datum		values[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
-	bool		nulls[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
 	MemoryContext old_cxt;
 	MemoryContext tmp_cxt;
 
@@ -501,6 +511,9 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
 	while (ReadNextXLogRecord(xlogreader) &&
 		   xlogreader->EndRecPtr <= end_lsn)
 	{
+		Datum		values[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
+		bool		nulls[PG_GET_WAL_RECORDS_INFO_COLS] = {0};
+
 		/* Use the tmp context so we can clean up after each tuple is done */
 		old_cxt = MemoryContextSwitchTo(tmp_cxt);
 
-- 
2.34.1

From dabc3c0a5625a45e0c179f1a0b1ede5187702198 Mon Sep 17 00:00:00 2001
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Sat, 18 Mar 2023 04:06:27 +0000
Subject: [PATCH v3] Emit WAL record info via pg_get_wal_block_info()

---
 .../pg_walinspect/pg_walinspect--1.0--1.1.sql | 10 +++-
 contrib/pg_walinspect/pg_walinspect.c         | 47 +++++++++++++------
 doc/src/sgml/pgwalinspect.sgml                | 38 +++++++++------
 3 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql b/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
index 586c3b4467..46fcaff3a1 100644
--- a/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
+++ b/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
@@ -12,7 +12,15 @@ DROP FUNCTION pg_get_wal_stats_till_end_of_wal(pg_lsn, boolean);
 --
 CREATE FUNCTION pg_get_wal_block_info(IN start_lsn pg_lsn,
 	IN end_lsn pg_lsn,
-	OUT lsn pg_lsn,
+    OUT start_lsn pg_lsn,
+    OUT end_lsn pg_lsn,
+    OUT prev_lsn pg_lsn,
+    OUT xid xid,
+    OUT resource_manager text,
+    OUT record_type text,
+    OUT record_length int4,
+    OUT main_data_length int4,
+    OUT description text,
 	OUT blockid int2,
 	OUT reltablespace oid,
 	OUT reldatabase oid,
diff --git a/contrib/pg_walinspect/pg_walinspect.c b/contrib/pg_walinspect/pg_walinspect.c
index c3ed31db8f..520d39a27f 100644
--- a/contrib/pg_walinspect/pg_walinspect.c
+++ b/contrib/pg_walinspect/pg_walinspect.c
@@ -43,7 +43,8 @@ static XLogRecPtr GetCurrentLSN(void);
 static XLogReaderState *InitXLogReaderState(XLogRecPtr lsn);
 static XLogRecord *ReadNextXLogRecord(XLogReaderState *xlogreader);
 static void GetWALRecordInfo(XLogReaderState *record, Datum *values,
-							 bool *nulls, uint32 ncols);
+							 bool *nulls, uint32 ncols,
+							 bool fetch_blk_ref);
 static void GetWALRecordsInfo(FunctionCallInfo fcinfo,
 							  XLogRecPtr start_lsn,
 							  XLogRecPtr end_lsn);
@@ -180,7 +181,8 @@ ReadNextXLogRecord(XLogReaderState *xlogreader)
  */
 static void
 GetWALRecordInfo(XLogReaderState *record, Datum *values,
-				 bool *nulls, uint32 ncols)
+				 bool *nulls, uint32 ncols,
+				 bool fetch_blk_ref)
 {
 	const char *id;
 	RmgrData	desc;
@@ -199,8 +201,8 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
 	initStringInfo(&rec_desc);
 	desc.rm_desc(&rec_desc, record);
 
-	/* Get block references, if any. */
-	if (XLogRecHasAnyBlockRefs(record))
+	/* Get block references, if any and if asked. */
+	if (XLogRecHasAnyBlockRefs(record) && fetch_blk_ref)
 	{
 		initStringInfo(&rec_blk_ref);
 		XLogRecGetBlockRefInfo(record, false, true, &rec_blk_ref, &fpi_len);
@@ -216,13 +218,16 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
 	values[i++] = CStringGetTextDatum(id);
 	values[i++] = UInt32GetDatum(XLogRecGetTotalLen(record));
 	values[i++] = UInt32GetDatum(main_data_len);
-	values[i++] = UInt32GetDatum(fpi_len);
+
+	if (fetch_blk_ref)
+		values[i++] = UInt32GetDatum(fpi_len);
+
 	values[i++] = CStringGetTextDatum(rec_desc.data);
 
-	/* Output block references, if any. */
-	if (XLogRecHasAnyBlockRefs(record))
+	/* Output block references, if any and if asked. */
+	if (XLogRecHasAnyBlockRefs(record) && fetch_blk_ref)
 		values[i++] = CStringGetTextDatum(rec_blk_ref.data);
-	else
+	else if (fetch_blk_ref)
 		nulls[i++] = true;
 
 	Assert(i == ncols);
@@ -236,19 +241,30 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,
 static void
 GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
 {
-#define PG_GET_WAL_BLOCK_INFO_COLS 11
+#define PG_GET_WAL_BLOCK_INFO_COLS 19
+#define PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS 9
+	Datum		values[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
+	bool		nulls[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
 	int			block_id;
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 
+	/* Get per-record info. */
+	GetWALRecordInfo(record, values, nulls,
+					 PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS, false);
+
 	for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++)
 	{
 		DecodedBkpBlock *blk;
 		BlockNumber blkno;
 		RelFileLocator rnode;
 		ForkNumber	fork;
-		Datum		values[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
-		bool		nulls[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
-		int			i = 0;
+		int			i = PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS;
+
+		/* Reset only per-block output columns, keep per-record info as-is. */
+		memset(&nulls[PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS], 0,
+			   PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS * sizeof(bool));
+		memset(&values[PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS], 0,
+			   PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS * sizeof(bool));
 
 		if (!XLogRecHasBlockRef(record, block_id))
 			continue;
@@ -258,7 +274,6 @@ GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
 		(void) XLogRecGetBlockTagExtended(record, block_id,
 										  &rnode, &fork, &blkno, NULL);
 
-		values[i++] = LSNGetDatum(record->ReadRecPtr);
 		values[i++] = Int16GetDatum(block_id);
 		values[i++] = ObjectIdGetDatum(blk->rlocator.spcOid);
 		values[i++] = ObjectIdGetDatum(blk->rlocator.dbOid);
@@ -353,6 +368,7 @@ GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
 							 values, nulls);
 	}
 
+#undef PG_GET_WAL_BLOCK_INFO_PER_RECORD_COLS
 #undef PG_GET_WAL_FPI_BLOCK_COLS
 }
 
@@ -446,7 +462,8 @@ pg_get_wal_record_info(PG_FUNCTION_ARGS)
 				 errmsg("could not read WAL at %X/%X",
 						LSN_FORMAT_ARGS(xlogreader->EndRecPtr))));
 
-	GetWALRecordInfo(xlogreader, values, nulls, PG_GET_WAL_RECORD_INFO_COLS);
+	GetWALRecordInfo(xlogreader, values, nulls, PG_GET_WAL_RECORD_INFO_COLS,
+					 true);
 
 	pfree(xlogreader->private_data);
 	XLogReaderFree(xlogreader);
@@ -518,7 +535,7 @@ GetWALRecordsInfo(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
 		old_cxt = MemoryContextSwitchTo(tmp_cxt);
 
 		GetWALRecordInfo(xlogreader, values, nulls,
-						 PG_GET_WAL_RECORDS_INFO_COLS);
+						 PG_GET_WAL_RECORDS_INFO_COLS, true);
 
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
 							 values, nulls);
diff --git a/doc/src/sgml/pgwalinspect.sgml b/doc/src/sgml/pgwalinspect.sgml
index 9a0241a8d6..864ea298f3 100644
--- a/doc/src/sgml/pgwalinspect.sgml
+++ b/doc/src/sgml/pgwalinspect.sgml
@@ -177,23 +177,33 @@ combined_size_percentage     | 2.8634072910530795
       <replaceable>start_lsn</replaceable> is not available. For example,
       usage of the function is as follows:
 <screen>
-postgres=# SELECT lsn, blockid, reltablespace, reldatabase, relfilenode,
+postgres=# SELECT start_lsn, end_lsn, prev_lsn, xid, resource_manager,
+                  record_type, record_length, main_data_length, description,
+                  blockid, reltablespace, reldatabase, relfilenode,
                   relblocknumber, forkname,
                   substring(blockdata for 24) as block_trimmed,
                   substring(fpi for 24) as fpi_trimmed, fpilen, fpiinfo
-             FROM pg_get_wal_block_info('0/1871080', '0/1871440');
--[ RECORD 1 ]--+---------------------------------------------------
-lsn            | 0/18712F8
-blockid        | 0
-reltablespace  | 1663
-reldatabase    | 16384
-relfilenode    | 16392
-relblocknumber | 0
-forkname       | main
-block_trimmed  | \x02800128180164000000
-fpi_trimmed    | \x0000000050108701000000002c00601f00200420e0020000
-fpilen         | 204
-fpiinfo        | {HAS_HOLE,APPLY}
+             FROM pg_get_wal_block_info('0/15457B0', '0/1545A40');
+-[ RECORD 1 ]----+---------------------------------------------------
+start_lsn        | 0/1545860
+end_lsn          | 0/1545998
+prev_lsn         | 0/15457E8
+xid              | 750
+resource_manager | Heap
+record_type      | HOT_UPDATE
+record_length    | 305
+main_data_length | 14
+description      | off 2 xmax 750 flags 0x00 ; new off 6 xmax 0
+blockid          | 0
+reltablespace    | 1663
+reldatabase      | 5
+relfilenode      | 24594
+relblocknumber   | 0
+forkname         | main
+block_trimmed    | \x02800128180164000000
+fpi_trimmed      | \x00000000203a5401000000003000401f00200420ed020000
+fpilen           | 240
+fpiinfo          | {HAS_HOLE,APPLY}
 </screen>
      </para>
     </listitem>
-- 
2.34.1

Reply via email to