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?