On Fri, Mar 25, 2022 at 1:43 AM David Christensen <david.christen...@crunchydata.com> wrote: > > On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas.mu...@gmail.com> wrote: > > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas.mu...@gmail.com> > > wrote: > >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut > >>> <peter.eisentr...@enterprisedb.com> wrote: > >>> 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. > >> > >> 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. > > > > Oh, and there's already a name lookup function to use for this. > > +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 <tmu...@postgresql.org> 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 <peter.eisentr...@enterprisedb.com> Reviewed-by: David Christensen <david.christen...@crunchydata.com> 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
