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

On Tue, Mar 22, 2022 at 6:14 AM David Christensen
<david.christensen@crunchydata.com> wrote:
> Updated to include the V3 fixes as well as the unsigned int/enum fix.

Hi David,

I ran this though pg_indent and adjusted some remaining
non-project-style whitespace, and took it for a spin.  Very minor
comments:

pg_waldump: error: could not parse valid relation from ""/ (expecting
"tablespace OID/database OID/relation filenode")
-> There was a stray "/" in that message, which I've removed in the attached.

pg_waldump: error: could not parse valid relation from "1664/0/1262"
(expecting "tablespace OID/database OID/relation filenode")
-> Why not?  Shared relations like pg_database have invalid database
OID, so I think it should be legal to write --relation=1664/0/1262.  I
took out that restriction.

+                                       if (sscanf(optarg, "%u",
&forknum) != 1 ||
+                                               forknum >= MAX_FORKNUM)
+                                       {
+                                               pg_log_error("could
not parse valid fork number (0..%d) \"%s\"",
+
  MAX_FORKNUM - 1, optarg);
+                                               goto bad_argument;
+                                       }

I guess you did this because init fork references aren't really
expected in the WAL, but I think it's more consistent to allow up to
MAX_FORKNUM, not least because your documentation mentions 3 as a
valid value.  So I adjust this to allow MAX_FORKNUM.  Make sense?

Here are some more details I noticed, as a likely future user of this
very handy feature, which I haven't changed, because they seem more
debatable and you might disagree...

1.  I think it'd be less surprising if the default value for --fork
wasn't 0... why not show all forks?
2.  I think it'd be less surprising if --fork without --relation
either raised an error (like --block without --relation), or were
allowed, with the meaning "show me this fork of all relations".
3.  It seems funny to have no short switch for --fork when everything
else has one... what about -F?