Subject: Re: [PATCH] add relation and block-level filtering to pg_waldump

From: Thomas Munro <[email-masked]>
Date: Mon, 21 Mar 2022 17:55:42 +1300

On Mon, Mar 21, 2022 at 4:36 PM Thomas Munro <[email-masked]> wrote:
> On Sat, Feb 26, 2022 at 7:58 AM David Christensen
> <[email-masked]> wrote:
> > Attached is V2 with additional feedback from this email, as well as the specification of the
> > ForkNumber and FPW as specifiable options.
>
> Trivial fixup needed after commit 3f1ce973.

[04:30:50.630] pg_waldump.c:963:26: error: format ‘%u’ expects
argument of type ‘unsigned int *’, but argument 3 has type ‘ForkNumber
*’ [-Werror=format=]
[04:30:50.630] 963 | if (sscanf(optarg, "%u",
&config.filter_by_relation_forknum) != 1 ||
[04:30:50.630] | ~^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[04:30:50.630] | | |
[04:30:50.630] | | ForkNumber *
[04:30:50.630] | unsigned int *

And now that this gets to the CompilerWarnings CI task, it looks like
GCC doesn't like an enum as a scanf %u destination (I didn't see that
warning locally when I compiled the above fixup because clearly Clang
is cool with it...).  Probably needs a temporary unsigned int to
sscanf into first.

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 23 Mar 2022 21:53:21 +0100

> &config.filter_by_relation_forknum) != 1 ||

That's because ForkNum is a signed type.  You will probably succeed if 
you use "%d" instead.

---

From: Thomas Munro <[email-masked]>
Date: Thu, 24 Mar 2022 11:54:15 +1300

> That's because ForkNum is a signed type.  You will probably succeed if

Erm, is that really OK?  C says "Each enumerated type shall be
compatible with char, a signed integer type, or an
unsigned integer type. The choice of type is implementation-defined,
but shall be capable of representing the values of all the members of
the enumeration."  It could even legally vary from enum to enum,
though in practice most compilers probably just use ints all the time
unless you use weird pragma pack incantation.  Therefore I think you
need an intermediate variable with the size and signedness matching the
format string, if you're going to scanf directly into it, which
David's V6 did.

---

From: Andres Freund <[email-masked]>
Date: Wed, 23 Mar 2022 18:06:36 -0700

> format string, if you're going to scanf directly into it, which

/me yearns for the perfectly reasonable C++ 11 feature of defining the base
type for enums (enum name : basetype { }). One of those features C should have
adopted long ago. Not that we could use it yet, given we insist that C
standards have reached at least european drinking age before relying on them.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Mar 2022 11:57:56 +0100

> format string, if you're going to scanf directly into it, which

An intermediate variable is probably the best way to avoid thinking 
about this much more. ;-)  But note that the committed patch uses a %u 
format whereas the ForkNum enum is signed.

Btw., why the sscanf() instead of just strtol/stroul?

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 24 Mar 2022 12:01:47 +0100

> Btw., why the sscanf() instead of just strtol/stroul?

Or even:  Why are we exposing fork *numbers* in the user interface? 
Even low-level tools such as pageinspect use fork *names* in their 
interface.

---

From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 00:26:43 +1300

> Even low-level tools such as pageinspect use fork *names* in their

I wondered about that but thought it seemed OK for such a low level
tool.  It's a fair point though, especially if other low level tools
are doing that.  Here's a patch to change it.
From 79060adc6db3e38c1935426145115c9eed39d85f Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 00:22:01 +1300
Subject: [PATCH] Use fork names, not numbers, in pg_waldump option.

Improvement for commit 127aea2a.

Suggested-by: Peter Eisentraut <[email-masked]>
Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com
---
 doc/src/sgml/ref/pg_waldump.sgml |  8 ++++----
 src/bin/pg_waldump/pg_waldump.c  | 20 +++++++++++++-------
 2 files changed, 17 insertions(+), 11 deletions(-)

diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 981d3c9038..9e1b91683d 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -118,10 +118,10 @@ PostgreSQL documentation
       <listitem>
        <para>
         If provided, only display records that modify blocks in the given fork.
-        The valid values are <literal>0</literal> for the main fork,
-        <literal>1</literal> for the free space map,
-        <literal>2</literal> for the visibility map,
-        and <literal>3</literal> for the init fork.
+        The valid values are <literal>main</literal> for the main fork,
+        <literal>fsm</literal> for the free space map,
+        <literal>vm</literal> for the visibility map,
+        and <literal>init</literal> for the init fork.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 92238f30c9..e878c90803 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -828,8 +828,8 @@ usage(void)
 	printf(_("  -e, --end=RECPTR       stop reading at WAL location RECPTR\n"));
 	printf(_("  -f, --follow           keep retrying after reaching end of WAL\n"));
 	printf(_("  -k, --block=N          with --relation, only show records matching this block\n"));
-	printf(_("  -F, --fork=N           only show records matching a specific fork number\n"
-			 "                         (defaults to showing all)\n"));
+	printf(_("  -F, --fork=FORK        only show records matching a specific fork;\n"
+			 "                         valid fork names are main, fsm, vm, init\n"));
 	printf(_("  -l, --relation=N/N/N   only show records that affect a specific relation\n"));
 	printf(_("  -n, --limit=N          number of records to display\n"));
 	printf(_("  -p, --path=PATH        directory in which to find log segment files or a\n"
@@ -968,13 +968,19 @@ main(int argc, char **argv)
 				break;
 			case 'F':
 				{
-					unsigned int forknum;
+					int			forknum = InvalidForkNumber;
 
-					if (sscanf(optarg, "%u", &forknum) != 1 ||
-						forknum > MAX_FORKNUM)
+					for (int i = 0; i <= MAX_FORKNUM; ++i)
 					{
-						pg_log_error("could not parse valid fork number (0..%d) \"%s\"",
-									 MAX_FORKNUM, optarg);
+						if (strcmp(optarg, forkNames[i]) == 0)
+						{
+							forknum = i;
+							break;
+						}
+					}
+					if (forknum == InvalidForkNumber)
+					{
+						pg_log_error("could not parse fork \"%s\"", optarg);
 						goto bad_argument;
 					}
 					config.filter_by_relation_forknum = (ForkNumber) forknum;
-- 
2.30.2

---

From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 00:42:50 +1300

> are doing that.  Here's a patch to change it.

Oh, and there's already a name lookup function to use for this.
From e276c28414645022ead3f33e6a6bc11ae0479496 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 00:22:01 +1300
Subject: [PATCH v2] Use fork names, not numbers, in pg_waldump option.

Suggested-by: Peter Eisentraut <[email-masked]>
Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com
---
 doc/src/sgml/ref/pg_waldump.sgml |  8 ++++----
 src/bin/pg_waldump/pg_waldump.c  | 15 +++++++--------
 2 files changed, 11 insertions(+), 12 deletions(-)

index 92238f30c9..bb6b7576fd 100644
@@ -968,16 +968,15 @@ main(int argc, char **argv)
 				break;
 			case 'F':
 				{
-					unsigned int forknum;
+					ForkNumber	forknum;
 
-					if (sscanf(optarg, "%u", &forknum) != 1 ||
-						forknum > MAX_FORKNUM)
+					forknum = forkname_to_number(optarg);
+					if (forknum == InvalidForkNumber)
 					{
-						pg_log_error("could not parse valid fork number (0..%d) \"%s\"",
-									 MAX_FORKNUM, optarg);
+						pg_log_error("could not parse fork \"%s\"", optarg);
 						goto bad_argument;
 					}
-					config.filter_by_relation_forknum = (ForkNumber) forknum;
+					config.filter_by_relation_forknum = forknum;
 					config.filter_by_extended = true;
 				}
 				break;
-- 
2.30.2

---

From: David Christensen <[email-masked]>
Date: Thu, 24 Mar 2022 07:43:02 -0500

> On Mar 24, 2022, at 6:43 AM, Thomas Munro <[email-masked]> wrote:
> 

+1 on the semantic names. 

David

---

From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 10:11:32 +1300

> +1 on the semantic names.

Cool.

I had another thought while changing that (and also re-alphabetising):
 Why don't we switch to  -B for --block and -R for --relation?  I
gather you used -k and -l because -b and -r were already taken, but
since we already started using upper case for -F, it seems consistent
this way.  Or were they chosen for consistency with something else?

It's also slightly more helpful to a user if the help says
--relation=T/D/R instead of N/N/N (TS/DB/REL would be nicer but
doesn't fit in the space).
From c4aac1b1f02ff44b8f6b5340283faf87b5eff509 Mon Sep 17 00:00:00 2001
From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 09:13:39 +1300
Subject: [PATCH] Improve command line switches in pg_waldump option.

Improvements for commit 127aea2a:
* use fork name for --fork, not number
* use -R, -B as short switches for --relation, --block

Suggested-by: Peter Eisentraut <[email-masked]>
Reviewed-by: David Christensen <[email-masked]>
Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com
---
 doc/src/sgml/ref/pg_waldump.sgml |  8 ++--
 src/bin/pg_waldump/pg_waldump.c  | 76 +++++++++++++++-----------------
 2 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 981d3c9038..9e1b91683d 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -118,10 +118,10 @@ PostgreSQL documentation
       <listitem>
        <para>
         If provided, only display records that modify blocks in the given fork.
-        The valid values are <literal>0</literal> for the main fork,
-        <literal>1</literal> for the free space map,
-        <literal>2</literal> for the visibility map,
-        and <literal>3</literal> for the init fork.
+        The valid values are <literal>main</literal> for the main fork,
+        <literal>fsm</literal> for the free space map,
+        <literal>vm</literal> for the visibility map,
+        and <literal>init</literal> for the init fork.
        </para>
       </listitem>
      </varlistentry>
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 92238f30c9..75a3964263 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -825,12 +825,11 @@ usage(void)
 	printf(_("  %s [OPTION]... [STARTSEG [ENDSEG]]\n"), progname);
 	printf(_("\nOptions:\n"));
 	printf(_("  -b, --bkp-details      output detailed information about backup blocks\n"));
+	printf(_("  -B, --block=N          with --relation, only show records that modify block N\n"));
 	printf(_("  -e, --end=RECPTR       stop reading at WAL location RECPTR\n"));
 	printf(_("  -f, --follow           keep retrying after reaching end of WAL\n"));
-	printf(_("  -k, --block=N          with --relation, only show records matching this block\n"));
-	printf(_("  -F, --fork=N           only show records matching a specific fork number\n"
-			 "                         (defaults to showing all)\n"));
-	printf(_("  -l, --relation=N/N/N   only show records that affect a specific relation\n"));
+	printf(_("  -F, --fork=FORK        only show records that modify blocks in fork FORK;\n"
+			 "                         valid names are main, fsm, vm, init\n"));
 	printf(_("  -n, --limit=N          number of records to display\n"));
 	printf(_("  -p, --path=PATH        directory in which to find log segment files or a\n"
 			 "                         directory with a ./pg_wal that contains such files\n"
@@ -838,12 +837,13 @@ usage(void)
 	printf(_("  -q, --quiet            do not print any output, except for errors\n"));
 	printf(_("  -r, --rmgr=RMGR        only show records generated by resource manager RMGR;\n"
 			 "                         use --rmgr=list to list valid resource manager names\n"));
+	printf(_("  -R, --relation=T/D/R   only show records that modify blocks in relation T/D/R\n"));
 	printf(_("  -s, --start=RECPTR     start reading at WAL location RECPTR\n"));
 	printf(_("  -t, --timeline=TLI     timeline from which to read log records\n"
 			 "                         (default: 1 or the value used in STARTSEG)\n"));
 	printf(_("  -V, --version          output version information, then exit\n"));
-	printf(_("  -x, --xid=XID          only show records with transaction ID XID\n"));
 	printf(_("  -w, --fullpage         only show records with a full page write\n"));
+	printf(_("  -x, --xid=XID          only show records with transaction ID XID\n"));
 	printf(_("  -z, --stats[=record]   show statistics instead of records\n"
 			 "                         (optionally, show per-record statistics)\n"));
 	printf(_("  -?, --help             show this help, then exit\n"));
@@ -946,7 +946,7 @@ main(int argc, char **argv)
 		goto bad_argument;
 	}
 
-	while ((option = getopt_long(argc, argv, "be:fF:k:l:n:p:qr:s:t:wx:z",
+	while ((option = getopt_long(argc, argv, "bB:e:fF:n:p:qr:R:s:t:wx:z",
 								 long_options, &optindex)) != -1)
 	{
 		switch (option)
@@ -954,6 +954,16 @@ main(int argc, char **argv)
 			case 'b':
 				config.bkp_details = true;
 				break;
+			case 'B':
+				if (sscanf(optarg, "%u", &config.filter_by_relation_block) != 1 ||
+					!BlockNumberIsValid(config.filter_by_relation_block))
+				{
+					pg_log_error("could not parse valid block number \"%s\"", optarg);
+					goto bad_argument;
+				}
+				config.filter_by_relation_block_enabled = true;
+				config.filter_by_extended = true;
+				break;
 			case 'e':
 				if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2)
 				{
@@ -967,44 +977,12 @@ main(int argc, char **argv)
 				config.follow = true;
 				break;
 			case 'F':
+				config.filter_by_relation_forknum = forkname_to_number(optarg);
+				if (config.filter_by_relation_forknum == InvalidForkNumber)
 				{
-					unsigned int forknum;
-
-					if (sscanf(optarg, "%u", &forknum) != 1 ||
-						forknum > MAX_FORKNUM)
-					{
-						pg_log_error("could not parse valid fork number (0..%d) \"%s\"",
-									 MAX_FORKNUM, optarg);
-						goto bad_argument;
-					}
-					config.filter_by_relation_forknum = (ForkNumber) forknum;
-					config.filter_by_extended = true;
-				}
-				break;
-			case 'k':
-				if (sscanf(optarg, "%u", &config.filter_by_relation_block) != 1 ||
-					!BlockNumberIsValid(config.filter_by_relation_block))
-				{
-					pg_log_error("could not parse valid block number \"%s\"", optarg);
-					goto bad_argument;
-				}
-				config.filter_by_relation_block_enabled = true;
-				config.filter_by_extended = true;
-				break;
-			case 'l':
-				if (sscanf(optarg, "%u/%u/%u",
-						   &config.filter_by_relation.spcNode,
-						   &config.filter_by_relation.dbNode,
-						   &config.filter_by_relation.relNode) != 3 ||
-					!OidIsValid(config.filter_by_relation.spcNode) ||
-					!OidIsValid(config.filter_by_relation.relNode))
-				{
-					pg_log_error("could not parse valid relation from \"%s\""
-								 " (expecting \"tablespace OID/database OID/"
-								 "relation filenode\")", optarg);
+					pg_log_error("could not parse fork \"%s\"", optarg);
 					goto bad_argument;
 				}
-				config.filter_by_relation_enabled = true;
 				config.filter_by_extended = true;
 				break;
 			case 'n':
@@ -1047,6 +1025,22 @@ main(int argc, char **argv)
 					}
 				}
 				break;
+			case 'R':
+				if (sscanf(optarg, "%u/%u/%u",
+						   &config.filter_by_relation.spcNode,
+						   &config.filter_by_relation.dbNode,
+						   &config.filter_by_relation.relNode) != 3 ||
+					!OidIsValid(config.filter_by_relation.spcNode) ||
+					!OidIsValid(config.filter_by_relation.relNode))
+				{
+					pg_log_error("could not parse valid relation from \"%s\""
+								 " (expecting \"tablespace OID/database OID/"
+								 "relation filenode\")", optarg);
+					goto bad_argument;
+				}
+				config.filter_by_relation_enabled = true;
+				config.filter_by_extended = true;
+				break;
 			case 's':
 				if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2)
 				{
-- 
2.30.2

---

From: David Christensen <[email-masked]>
Date: Thu, 24 Mar 2022 17:40:30 -0500

> On Mar 24, 2022, at 4:12 PM, Thomas Munro <[email-masked]> wrote:
> 

Works here; was just trying to get semi-memorable ones from the available lowercase ones, but I like your idea here, and it kind of puts them in the same mental space for remembering.

---

From: Japin Li <[email-masked]>
Date: Fri, 25 Mar 2022 08:43:30 +0800

> doesn't fit in the space).

Thanks for updating the patch!

I think the description of transaction ID is enough, IIUC, XID is use in core,
which means transaction ID.

See: src/bin/pg_resetwal/pg_resetwal.c

1239     printf(_("  -V, --version                    output version information, then exit\n"));
1240     printf(_("  -x, --next-transaction-id=XID    set next transaction ID\n"));

It seems we should also check the dbNode.

--
Regrads,
Japin Li.
ChengDu WenWu Information Technology Co.,Ltd.

---

From: Thomas Munro <[email-masked]>
Date: Fri, 25 Mar 2022 13:55:04 +1300

> which means transaction ID.

The mention of "XID" corresponds to XID on the left, like a sort of
variable.  That text is not changed by this patch.

> 1240     printf(_("  -x, --next-transaction-id=XID    set next transaction ID\n"));

Hmm, yeah that is inconsistent, but it seems like it is pg_resetwal.c
that is not following the notational convention there.  Other things
in pg_resetwal's --help use that 'variable' style.

> It seems we should also check the dbNode.

This was discussed earlier: it's OK for the dbNode to be invalid (0),
because that's how shared relations like pg_database are referenced.
Like this:

$ pg_waldump pgdata/pg_wal/000000010000000000000001 --relation
1664/0/1262 --fork vm --limit 1
rmgr: Heap2       len (rec/tot):     64/  8256, tx:          0, lsn:
0/01491F20, prev 0/01491EC0, desc: VISIBLE cutoff xid 1 flags 0x03,
blkref #0: rel 1664/0/1262 fork vm blk 0 FPW, blkref #1: rel
1664/0/1262 blk 0

Thanks for looking!  I've now pushed the improvements discussed so far.

---

From: Japin Li <[email-masked]>
Date: Fri, 25 Mar 2022 09:32:49 +0800

> in pg_resetwal's --help use that 'variable' style.

Thanks for your explanation!

> blkref #0: rel 1664/0/1262 fork vm blk 0 FPW, blkref #1: rel

Oh, my bad, I missed the discussion email.  Sorry for the noise.