Subject: Re: Add pg_walinspect function with block info columns

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Mar 2023 16:28:11 +0900

On Tue, Mar 07, 2023 at 03:56:22PM +0530, Bharath Rupireddy wrote:
> That would be a lot better. Not just the test, but also the
> documentation can have it. Simple way to generate such a record (both
> block data and FPI) is to just change the wal_level to logical in
> walinspect.conf [1], see code around REGBUF_KEEP_DATA and
> RelationIsLogicallyLogged in heapam.c

I don't agree that we need to go down to wal_level=logical for this.
The important part is to check that the non-NULL and NULL paths for
the block data and FPI data are both taken, making 4 paths to check.
So we need two tests at minimum, which would be either:
- One SQL generating no FPI with no block data and a second generating
a FPI with block data.  v2 was doing that but did not cover the first
case.
- One SQL generating a FPI with no block data and a second generating
no FPI with block data.

So let's just geenrate a heap record on an UPDATE, for example, like
in the version attached.

> 2. Used int4 instead of int for fpilen just to be in sync with
> fpi_length of pg_get_wal_record_info.

Okay.

> 3. Changed to be consistent and use just FPI or "F/full page".
>             /* FPI flags */
>             /* No full page image, so store NULLs for all its fields */
>         /* Full-page image */
>         /* Full page exists, so let's save it. */
>  * and end LSNs.  This produces information about the full page images with
>  * to a record.  Decompression is applied to the full-page images, if

Fine by me.

> 4. I think we need to free raw_data, raw_page and flags as we loop
> over multiple blocks (XLR_MAX_BLOCK_ID) and will leak memory which can
> be a problem if we have many blocks assocated with a single WAL
> record.
>             flags = (Datum *) palloc0(sizeof(Datum) * bitcnt);
> Also, we will leak all CStringGetTextDatum memory in the block_id for loop.
> Another way is to use and reset temp memory context in the for loop
> over block_ids. I prefer this approach over multiple pfree()s in
> block_id for loop.

I disagree, this was on purpose in the last version.  This version
finishes by calling AllocSetContextCreate() and MemoryContextDelete()
once per *record*, which will not be free, and we are arguing about
resetting the memory context after scanning up to XLR_MAX_BLOCK_ID
blocks, or 32 blocks which would go up to 32kB per page in the worst
case.  That's not going to matter in a large scan for each record, but
the extra AllocSet*() calls could.  And we basically do the same thing
on HEAD.

> 5. I think it'd be good to say if the FPI is for WAL_VERIFICATION, so
> I changed it to the following. Feel free to ignore this if you think
> it's not required.
>             if (blk->apply_image)
>                 flags[cnt++] = CStringGetTextDatum("APPLY");
>             else
>                 flags[cnt++] = CStringGetTextDatum("WAL_VERIFICATION");

Disagreed here as well.  WAL_VERIFICATION does not map with any of the
internal flags, and actually it may be finished by not being used
at replay if the LSN of the page read if higher than what the WAL
stores.

> 7. Added test case which shows both block data and fpi in the
> documentation.

Okay on that.

> 8. Changed wal_level to logical in walinspect.conf to test case with block data.

This change is not necessary, per the argument above.

Any comments?
--
Michael
From 8a2bc9ab2c4d28f37f28b93ad9713fbd8ecb9dd3 Mon Sep 17 00:00:00 2001
From: Michael Paquier <[email-masked]>
Date: Wed, 8 Mar 2023 16:20:33 +0900
Subject: [PATCH v3] Rework pg_walinspect to retrieve more block information

---
 doc/src/sgml/pgwalinspect.sgml                |  48 ++++--
 .../pg_walinspect/expected/pg_walinspect.out  |  35 +++--
 .../pg_walinspect/pg_walinspect--1.0--1.1.sql |  16 +-
 contrib/pg_walinspect/pg_walinspect.c         | 140 +++++++++++++-----
 contrib/pg_walinspect/sql/pg_walinspect.sql   |  33 +++--
 5 files changed, 186 insertions(+), 86 deletions(-)

diff --git a/doc/src/sgml/pgwalinspect.sgml b/doc/src/sgml/pgwalinspect.sgml
index 3d7cdb95cc..d26115d6f2 100644
--- a/doc/src/sgml/pgwalinspect.sgml
+++ b/doc/src/sgml/pgwalinspect.sgml
@@ -190,31 +190,51 @@ combined_size_percentage     | 2.8634072910530795
 
    <varlistentry>
     <term>
-     <function>pg_get_wal_fpi_info(start_lsn pg_lsn, end_lsn pg_lsn) returns setof record</function>
+     <function>pg_get_wal_block_info(start_lsn pg_lsn, end_lsn pg_lsn) returns setof record</function>
     </term>
 
     <listitem>
      <para>
-      Gets a copy of full page images as <type>bytea</type> values (after
-      applying decompression when necessary) and their information associated
-      with all the valid WAL records between
+      Gets a copy of the block information stored in WAL records. This includes
+      copies of the block data (<literal>NULL</literal> if none) and full page
+      images as <type>bytea</type> values (after
+      applying decompression when necessary, or <literal>NULL</literal> if none)
+      and their information associated with all the valid WAL records between
       <replaceable>start_lsn</replaceable> and
-      <replaceable>end_lsn</replaceable>. Returns one row per full page image.
-      If <replaceable>start_lsn</replaceable> or
+      <replaceable>end_lsn</replaceable>. Returns one row per block registered
+      in a WAL record. If <replaceable>start_lsn</replaceable> or
       <replaceable>end_lsn</replaceable> are not yet available, the function
       will raise an error. For example:
 <screen>
-postgres=# SELECT lsn, reltablespace, reldatabase, relfilenode, relblocknumber,
-                  forkname, substring(fpi for 24) as fpi_trimmed
-             FROM pg_get_wal_fpi_info('0/1801690', '0/1825C60');
+postgres=# SELECT lsn, 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/1807E20
+lsn            | 0/1871208
+blockid        | 0
 reltablespace  | 1663
-reldatabase    | 5
-relfilenode    | 16396
-relblocknumber | 43
+reldatabase    | 16384
+relfilenode    | 1255
+relblocknumber | 96
 forkname       | main
-fpi_trimmed    | \x00000000b89e660100000000a003c0030020042000000000
+block_trimmed  | \x0700080009000a00
+fpi_trimmed    |
+fpilen         |
+fpiinfo        |
+-[ RECORD 2 ]--+---------------------------------------------------
+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}
 </screen>
      </para>
     </listitem>
diff --git a/contrib/pg_walinspect/expected/pg_walinspect.out b/contrib/pg_walinspect/expected/pg_walinspect.out
index 9bcb05354e..e0eb7ca08f 100644
--- a/contrib/pg_walinspect/expected/pg_walinspect.out
+++ b/contrib/pg_walinspect/expected/pg_walinspect.out
@@ -74,17 +74,28 @@ SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_records_info(:'wal_lsn1', :'wal_lsn2'
 (1 row)
 
 -- ===================================================================
--- Tests to get full page image (FPI) from WAL record
+-- Tests to get block information from WAL record
 -- ===================================================================
+-- Update table to generate some block data
 SELECT pg_current_wal_lsn() AS wal_lsn3 \gset
--- Force FPI on the next update.
-CHECKPOINT;
--- Update table to generate an FPI.
-UPDATE sample_tbl SET col1 = col1 * 100 WHERE col1 = 1;
+UPDATE sample_tbl SET col1 = col1 + 1 WHERE col1 = 1;
 SELECT pg_current_wal_lsn() AS wal_lsn4 \gset
+-- Check if we get block data from WAL record.
+SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_block_info(:'wal_lsn3', :'wal_lsn4')
+  WHERE relfilenode = :'sample_tbl_oid' AND blockdata IS NOT NULL;
+ ok 
+----
+ t
+(1 row)
+
+-- Force full-page image on the next update.
+SELECT pg_current_wal_lsn() AS wal_lsn5 \gset
+CHECKPOINT;
+UPDATE sample_tbl SET col1 = col1 + 1 WHERE col1 = 2;
+SELECT pg_current_wal_lsn() AS wal_lsn6 \gset
 -- Check if we get FPI from WAL record.
-SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_fpi_info(:'wal_lsn3', :'wal_lsn4')
-  WHERE relfilenode = :'sample_tbl_oid';
+SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_block_info(:'wal_lsn5', :'wal_lsn6')
+  WHERE relfilenode = :'sample_tbl_oid' AND fpi IS NOT NULL;
  ok 
 ----
  t
@@ -116,7 +127,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
 (1 row)
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- no
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- no
  has_function_privilege 
 ------------------------
  f
@@ -146,7 +157,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
 (1 row)
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
  has_function_privilege 
 ------------------------
  t
@@ -160,7 +171,7 @@ GRANT EXECUTE ON FUNCTION pg_get_wal_records_info(pg_lsn, pg_lsn)
   TO regress_pg_walinspect;
 GRANT EXECUTE ON FUNCTION pg_get_wal_stats(pg_lsn, pg_lsn, boolean)
   TO regress_pg_walinspect;
-GRANT EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn)
+GRANT EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn)
   TO regress_pg_walinspect;
 SELECT has_function_privilege('regress_pg_walinspect',
   'pg_get_wal_record_info(pg_lsn)', 'EXECUTE'); -- yes
@@ -184,7 +195,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
 (1 row)
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
  has_function_privilege 
 ------------------------
  t
@@ -196,7 +207,7 @@ REVOKE EXECUTE ON FUNCTION pg_get_wal_records_info(pg_lsn, pg_lsn)
   FROM regress_pg_walinspect;
 REVOKE EXECUTE ON FUNCTION pg_get_wal_stats(pg_lsn, pg_lsn, boolean)
   FROM regress_pg_walinspect;
-REVOKE EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn)
+REVOKE EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn)
   FROM regress_pg_walinspect;
 -- ===================================================================
 -- Clean up
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 1e9e1e6115..e674ef25aa 100644
--- a/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
+++ b/contrib/pg_walinspect/pg_walinspect--1.0--1.1.sql
@@ -4,21 +4,25 @@
 \echo Use "ALTER EXTENSION pg_walinspect UPDATE TO '1.1'" to load this file. \quit
 
 --
--- pg_get_wal_fpi_info()
+-- pg_get_wal_block_info()
 --
-CREATE FUNCTION pg_get_wal_fpi_info(IN start_lsn pg_lsn,
+CREATE FUNCTION pg_get_wal_block_info(IN start_lsn pg_lsn,
 	IN end_lsn pg_lsn,
 	OUT lsn pg_lsn,
+	OUT blockid int2,
 	OUT reltablespace oid,
 	OUT reldatabase oid,
 	OUT relfilenode oid,
 	OUT relblocknumber int8,
 	OUT forkname text,
-	OUT fpi bytea
+	OUT blockdata bytea,
+	OUT fpi bytea,
+	OUT fpilen int4,
+	OUT fpiinfo text[]
 )
 RETURNS SETOF record
-AS 'MODULE_PATHNAME', 'pg_get_wal_fpi_info'
+AS 'MODULE_PATHNAME', 'pg_get_wal_block_info'
 LANGUAGE C STRICT PARALLEL SAFE;
 
-REVOKE EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn) FROM PUBLIC;
-GRANT EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn) TO pg_read_server_files;
+REVOKE EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn) FROM PUBLIC;
+GRANT EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn) TO pg_read_server_files;
diff --git a/contrib/pg_walinspect/pg_walinspect.c b/contrib/pg_walinspect/pg_walinspect.c
index b7b0a805ee..ee88dc4992 100644
--- a/contrib/pg_walinspect/pg_walinspect.c
+++ b/contrib/pg_walinspect/pg_walinspect.c
@@ -20,6 +20,7 @@
 #include "access/xlogutils.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/pg_lsn.h"
 
@@ -30,7 +31,7 @@
 
 PG_MODULE_MAGIC;
 
-PG_FUNCTION_INFO_V1(pg_get_wal_fpi_info);
+PG_FUNCTION_INFO_V1(pg_get_wal_block_info);
 PG_FUNCTION_INFO_V1(pg_get_wal_record_info);
 PG_FUNCTION_INFO_V1(pg_get_wal_records_info);
 PG_FUNCTION_INFO_V1(pg_get_wal_records_info_till_end_of_wal);
@@ -56,7 +57,7 @@ static void FillXLogStatsRow(const char *name, uint64 n, uint64 total_count,
 							 Datum *values, bool *nulls, uint32 ncols);
 static void GetWalStats(FunctionCallInfo fcinfo, XLogRecPtr start_lsn,
 						XLogRecPtr end_lsn, bool stats_per_record);
-static void GetWALFPIInfo(FunctionCallInfo fcinfo, XLogReaderState *record);
+static void GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record);
 
 /*
  * Check if the given LSN is in future. Also, return the LSN up to which the
@@ -221,49 +222,40 @@ GetWALRecordInfo(XLogReaderState *record, Datum *values,

 /*
- * Store a set of full page images from a single record.
+ * Store a set of block information from a single record (FPI and block
+ * information).
  */
 static void
-GetWALFPIInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
+GetWALBlockInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
 {
-#define PG_GET_WAL_FPI_INFO_COLS 7
+#define PG_GET_WAL_BLOCK_INFO_COLS 11
 	int			block_id;
 	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
 
 	for (block_id = 0; block_id <= XLogRecMaxBlockId(record); block_id++)
 	{
-		PGAlignedBlock buf;
-		Page		page;
-		bytea	   *raw_page;
-		BlockNumber blk;
+		DecodedBkpBlock *blk;
+		BlockNumber blkno;
 		RelFileLocator rnode;
 		ForkNumber	fork;
-		Datum		values[PG_GET_WAL_FPI_INFO_COLS] = {0};
-		bool		nulls[PG_GET_WAL_FPI_INFO_COLS] = {0};
+		Datum		values[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
+		bool		nulls[PG_GET_WAL_BLOCK_INFO_COLS] = {0};
 		int			i = 0;
 
 		if (!XLogRecHasBlockRef(record, block_id))
 			continue;
 
-		if (!XLogRecHasBlockImage(record, block_id))
-			continue;
+		blk = XLogRecGetBlock(record, block_id);
 
-		page = (Page) buf.data;
-
-		if (!RestoreBlockImage(record, block_id, page))
-			ereport(ERROR,
-					(errcode(ERRCODE_INTERNAL_ERROR),
-					 errmsg_internal("%s", record->errormsg_buf)));
-
-		/* Full page exists, so let's save it. */
 		(void) XLogRecGetBlockTagExtended(record, block_id,
-										  &rnode, &fork, &blk, NULL);
+										  &rnode, &fork, &blkno, NULL);
 
 		values[i++] = LSNGetDatum(record->ReadRecPtr);
-		values[i++] = ObjectIdGetDatum(rnode.spcOid);
-		values[i++] = ObjectIdGetDatum(rnode.dbOid);
-		values[i++] = ObjectIdGetDatum(rnode.relNumber);
-		values[i++] = Int64GetDatum((int64) blk);
+		values[i++] = Int16GetDatum(block_id);
+		values[i++] = ObjectIdGetDatum(blk->rlocator.spcOid);
+		values[i++] = ObjectIdGetDatum(blk->rlocator.dbOid);
+		values[i++] = ObjectIdGetDatum(blk->rlocator.relNumber);
+		values[i++] = Int64GetDatum((int64) blkno);
 
 		if (fork >= 0 && fork <= MAX_FORKNUM)
 			values[i++] = CStringGetTextDatum(forkNames[fork]);
@@ -272,34 +264,102 @@ GetWALFPIInfo(FunctionCallInfo fcinfo, XLogReaderState *record)
 					(errcode(ERRCODE_INTERNAL_ERROR),
 					 errmsg_internal("invalid fork number: %u", fork)));
 
-		/* Initialize bytea buffer to copy the FPI to. */
-		raw_page = (bytea *) palloc(BLCKSZ + VARHDRSZ);
-		SET_VARSIZE(raw_page, BLCKSZ + VARHDRSZ);
+		/* Block data */
+		if (blk->has_data)
+		{
+			bytea	   *raw_data;
 
-		/* Take a verbatim copy of the FPI. */
-		memcpy(VARDATA(raw_page), page, BLCKSZ);
+			/* Initialize bytea buffer to copy the data to */
+			raw_data = (bytea *) palloc(blk->data_len + VARHDRSZ);
+			SET_VARSIZE(raw_data, blk->data_len + VARHDRSZ);
 
-		values[i++] = PointerGetDatum(raw_page);
+			/* Copy the data */
+			memcpy(VARDATA(raw_data), blk->data, blk->data_len);
+			values[i++] = PointerGetDatum(raw_data);
+		}
+		else
+		{
+			/* No data, so set this field to NULL */
+			nulls[i++] = true;
+		}
 
-		Assert(i == PG_GET_WAL_FPI_INFO_COLS);
+		if (blk->has_image)
+		{
+			PGAlignedBlock buf;
+			Page		page;
+			bytea	   *raw_page;
+			int			bitcnt;
+			int			cnt = 0;
+			Datum	   *flags;
+			ArrayType  *a;
+
+			page = (Page) buf.data;
+
+			/* Full page image exists, so let's save it */
+			if (!RestoreBlockImage(record, block_id, page))
+				ereport(ERROR,
+						(errcode(ERRCODE_INTERNAL_ERROR),
+						 errmsg_internal("%s", record->errormsg_buf)));
+
+			/* Initialize bytea buffer to copy the FPI to */
+			raw_page = (bytea *) palloc(BLCKSZ + VARHDRSZ);
+			SET_VARSIZE(raw_page, BLCKSZ + VARHDRSZ);
+
+			/* Take a verbatim copy of the FPI */
+			memcpy(VARDATA(raw_page), page, BLCKSZ);
+
+			values[i++] = PointerGetDatum(raw_page);
+			values[i++] = UInt32GetDatum(blk->bimg_len);
+
+			/* FPI flags */
+			bitcnt = pg_popcount((const char *) &blk->bimg_info,
+								 sizeof(uint8));
+			/* Build set of raw flags */
+			flags = (Datum *) palloc0(sizeof(Datum) * bitcnt);
+
+			if ((blk->bimg_info & BKPIMAGE_HAS_HOLE) != 0)
+				flags[cnt++] = CStringGetTextDatum("HAS_HOLE");
+			if (blk->apply_image)
+				flags[cnt++] = CStringGetTextDatum("APPLY");
+			if ((blk->bimg_info & BKPIMAGE_COMPRESS_PGLZ) != 0)
+				flags[cnt++] = CStringGetTextDatum("COMPRESS_PGLZ");
+			if ((blk->bimg_info & BKPIMAGE_COMPRESS_LZ4) != 0)
+				flags[cnt++] = CStringGetTextDatum("COMPRESS_LZ4");
+			if ((blk->bimg_info & BKPIMAGE_COMPRESS_ZSTD) != 0)
+				flags[cnt++] = CStringGetTextDatum("COMPRESS_ZSTD");
+
+			Assert(cnt <= bitcnt);
+			a = construct_array_builtin(flags, cnt, TEXTOID);
+			values[i++] = PointerGetDatum(a);
+		}
+		else
+		{
+			/* No full page image, so store NULLs for all its fields */
+			memset(&nulls[i], true, 3 * sizeof(bool));
+			i += 3;
+		}
+
+		Assert(i == PG_GET_WAL_BLOCK_INFO_COLS);
 
 		tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
 							 values, nulls);
 	}
 
-#undef PG_GET_WAL_FPI_INFO_COLS
+#undef PG_GET_WAL_FPI_BLOCK_COLS
 }
 
 /*
- * Get full page images with their relation information for all the WAL
- * records between start and end LSNs.  Decompression is applied to the
- * blocks, if necessary.
+ * Get information about all the blocks saved in WAL records between start
+ * and end LSNs. This produces information about the full page images with
+ * their relation information, and the data saved in each block associated
+ * to a record. Decompression is applied to the full page images, if
+ * necessary.
  *
  * This function emits an error if a future start or end WAL LSN i.e. WAL LSN
  * the database system doesn't know about is specified.
  */
 Datum
-pg_get_wal_fpi_info(PG_FUNCTION_ARGS)
+pg_get_wal_block_info(PG_FUNCTION_ARGS)
 {
 	XLogRecPtr	start_lsn;
 	XLogRecPtr	end_lsn;
@@ -317,7 +377,7 @@ pg_get_wal_fpi_info(PG_FUNCTION_ARGS)
 	xlogreader = InitXLogReaderState(start_lsn);
 
 	tmp_cxt = AllocSetContextCreate(CurrentMemoryContext,
-									"pg_get_wal_fpi_info temporary cxt",
+									"pg_get_block_fpi_info temporary cxt",
 									ALLOCSET_DEFAULT_SIZES);
 
 	while (ReadNextXLogRecord(xlogreader) &&
@@ -326,7 +386,7 @@ pg_get_wal_fpi_info(PG_FUNCTION_ARGS)
 		/* Use the tmp context so we can clean up after each tuple is done */
 		old_cxt = MemoryContextSwitchTo(tmp_cxt);
 
-		GetWALFPIInfo(fcinfo, xlogreader);
+		GetWALBlockInfo(fcinfo, xlogreader);
 
 		/* clean up and switch back */
 		MemoryContextSwitchTo(old_cxt);
diff --git a/contrib/pg_walinspect/sql/pg_walinspect.sql b/contrib/pg_walinspect/sql/pg_walinspect.sql
index 849201a1f8..01a120f398 100644
--- a/contrib/pg_walinspect/sql/pg_walinspect.sql
+++ b/contrib/pg_walinspect/sql/pg_walinspect.sql
@@ -53,20 +53,25 @@ SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_records_info(:'wal_lsn1', :'wal_lsn2'
 			WHERE resource_manager = 'Heap' AND record_type = 'INSERT';
 
 -- ===================================================================
--- Tests to get full page image (FPI) from WAL record
+-- Tests to get block information from WAL record
 -- ===================================================================
+
+-- Update table to generate some block data
 SELECT pg_current_wal_lsn() AS wal_lsn3 \gset
-
--- Force FPI on the next update.
-CHECKPOINT;
-
--- Update table to generate an FPI.
-UPDATE sample_tbl SET col1 = col1 * 100 WHERE col1 = 1;
+UPDATE sample_tbl SET col1 = col1 + 1 WHERE col1 = 1;
 SELECT pg_current_wal_lsn() AS wal_lsn4 \gset
+-- Check if we get block data from WAL record.
+SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_block_info(:'wal_lsn3', :'wal_lsn4')
+  WHERE relfilenode = :'sample_tbl_oid' AND blockdata IS NOT NULL;
 
+-- Force full-page image on the next update.
+SELECT pg_current_wal_lsn() AS wal_lsn5 \gset
+CHECKPOINT;
+UPDATE sample_tbl SET col1 = col1 + 1 WHERE col1 = 2;
+SELECT pg_current_wal_lsn() AS wal_lsn6 \gset
 -- Check if we get FPI from WAL record.
-SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_fpi_info(:'wal_lsn3', :'wal_lsn4')
-  WHERE relfilenode = :'sample_tbl_oid';
+SELECT COUNT(*) >= 1 AS ok FROM pg_get_wal_block_info(:'wal_lsn5', :'wal_lsn6')
+  WHERE relfilenode = :'sample_tbl_oid' AND fpi IS NOT NULL;
 
 -- ===================================================================
 -- Tests for permissions
@@ -83,7 +88,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
   'pg_get_wal_stats(pg_lsn, pg_lsn, boolean) ', 'EXECUTE'); -- no
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- no
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- no
 
 -- Functions accessible by users with role pg_read_server_files
 
@@ -99,7 +104,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
   'pg_get_wal_stats(pg_lsn, pg_lsn, boolean) ', 'EXECUTE'); -- yes
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
 
 REVOKE pg_read_server_files FROM regress_pg_walinspect;
 
@@ -113,7 +118,7 @@ GRANT EXECUTE ON FUNCTION pg_get_wal_records_info(pg_lsn, pg_lsn)
 GRANT EXECUTE ON FUNCTION pg_get_wal_stats(pg_lsn, pg_lsn, boolean)
   TO regress_pg_walinspect;
 
-GRANT EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn)
+GRANT EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn)
   TO regress_pg_walinspect;
 
 SELECT has_function_privilege('regress_pg_walinspect',
@@ -126,7 +131,7 @@ SELECT has_function_privilege('regress_pg_walinspect',
   'pg_get_wal_stats(pg_lsn, pg_lsn, boolean) ', 'EXECUTE'); -- yes
 
 SELECT has_function_privilege('regress_pg_walinspect',
-  'pg_get_wal_fpi_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
+  'pg_get_wal_block_info(pg_lsn, pg_lsn) ', 'EXECUTE'); -- yes
 
 REVOKE EXECUTE ON FUNCTION pg_get_wal_record_info(pg_lsn)
   FROM regress_pg_walinspect;
@@ -137,7 +142,7 @@ REVOKE EXECUTE ON FUNCTION pg_get_wal_records_info(pg_lsn, pg_lsn)
 REVOKE EXECUTE ON FUNCTION pg_get_wal_stats(pg_lsn, pg_lsn, boolean)
   FROM regress_pg_walinspect;
 
-REVOKE EXECUTE ON FUNCTION pg_get_wal_fpi_info(pg_lsn, pg_lsn)
+REVOKE EXECUTE ON FUNCTION pg_get_wal_block_info(pg_lsn, pg_lsn)
   FROM regress_pg_walinspect;
 
 -- ===================================================================
-- 
2.39.2

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 8 Mar 2023 16:01:56 +0530

> So let's just geenrate a heap record on an UPDATE, for example, like

Yup, that should work too because block data gets logged [1].

> the extra AllocSet*() calls could.  And we basically do the same thing

It's not just 32kB per page right? 32*8KB on HEAD (no block data,
flags and CStringGetTextDatum there). With the patch, the number of
pallocs for each block_id = 6 CStringGetTextDatum + BLCKSZ (8KB) +
flags (5*size of ptr) + block data_len. In the worst case, all
XLR_MAX_BLOCK_ID can have both FPIs and block data. Furthermore,
imagine if someone initialized their cluster with a higher BLCKSZ (>=
8KB), then the memory leak happens noticeably on a lower-end system.

I understand that performance is critical here but we need to ensure
memory is used wisely. Therefore, I'd still vote to free at least the
major contributors here, that is, pfree(raw_data);, pfree(raw_page);
and pfree(flags); right after they are done using. I'm sure pfree()s
don't hurt more than resetting memory context for every block_id.

> Any comments?

I think we need to output block data length (blk->data_len) similar to
fpilen to save users from figuring out how to get the length of a
bytea column. This will also keep block data in sync with FPI info.

[1]
needs_backup = (page_lsn <= RedoRecPtr);

(gdb) p page_lsn
$2 = 21581544
(gdb) p RedoRecPtr
$3 = 21484808
(gdb) p needs_backup
$4 = false
(gdb)
(gdb) bt
#0  XLogRecordAssemble (rmid=10 '\n', info=64 '@',
RedoRecPtr=21484808, doPageWrites=true, fpw_lsn=0x7ffde118d640,
    num_fpi=0x7ffde118d634, topxid_included=0x7ffde118d633) at xloginsert.c:582
#1  0x00005598cd9c3ef7 in XLogInsert (rmid=10 '\n', info=64 '@') at
xloginsert.c:497
#2  0x00005598cd930452 in log_heap_update (reln=0x7f4a4c7cd808,
oldbuf=136, newbuf=136, oldtup=0x7ffde118d820,
    newtup=0x5598d00cb098, old_key_tuple=0x0,
all_visible_cleared=false, new_all_visible_cleared=false)
    at heapam.c:8473
#3  0x00005598cd92876e in heap_update (relation=0x7f4a4c7cd808,
otid=0x7ffde118dab2, newtup=0x5598d00cb098, cid=0,
    crosscheck=0x0, wait=true, tmfd=0x7ffde118db60,
lockmode=0x7ffde118da74) at heapam.c:3741

--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

---

From: Michael Paquier <[email-masked]>
Date: Wed, 8 Mar 2023 19:53:13 +0900

> don't hurt more than resetting memory context for every block_id.

Okay by me to have intermediate pfrees between each block scanned if
you feel strongly about it.

> bytea column. This will also keep block data in sync with FPI info.

length() works fine on bytea, so it can be used on the block data.
fpilen is a very different matter as it would be the length of a page
without a hole, or just something compressed.
--
Michael

---

From: Bharath Rupireddy <[email-masked]>
Date: Wed, 8 Mar 2023 20:18:06 +0530

> you feel strongly about it.

Thanks. Attached v4 with that change.