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

Greetings,

This patch adds the ability to specify a RelFileNode and optional BlockNum to limit output of
pg_waldump records to only those which match the given criteria. This should be more performant
than `pg_waldump | grep` as well as more reliable given specific variations in output style
depending on how the blocks are specified.

This currently affects only the main fork, but we could presumably add the option to filter by fork
as well, if that is considered useful.

Best,

David

On Thu, Feb 24, 2022 at 11:06 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)com> wrote:
> This patch adds the ability to specify a RelFileNode and optional BlockNum to limit output of
> pg_waldump records to only those which match the given criteria. This should be more performant
> than `pg_waldump | grep` as well as more reliable given specific variations in output style
> depending on how the blocks are specified.

Sounds useful to me.

--
Peter Geoghegan

Added to commitfest as:

https://commitfest.postgresql.org/37/3565/

On Fri, 25 Feb 2022 at 03:02, David Christensen <david(dot)christensen(at)crunchydata(dot)com> wrote:
> Greetings,
>
> This patch adds the ability to specify a RelFileNode and optional BlockNum to limit output of
> pg_waldump records to only those which match the given criteria. This should be more performant
> than `pg_waldump | grep` as well as more reliable given specific variations in output style
> depending on how the blocks are specified.
>
> This currently affects only the main fork, but we could presumably add the option to filter by fork
> as well, if that is considered useful.
>

Cool. I think we can report an error instead of reading wal files,
if the tablespace, database, or relation is invalid. Does there any
WAL record that has invalid tablespace, database, or relation OID?

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

> Cool. I think we can report an error instead of reading wal files,
> if the tablespace, database, or relation is invalid. Does there any
> WAL record that has invalid tablespace, database, or relation OID?

The only sort of validity check we could do here is range checking for the underlying data types (which we certainly could/should add if it’s known to never be valid for the underlying types); non-existence of objects is a no-go, since that depends purely on the WAL range you are looking at and you’d have to, you know, scan it to see if it existed before marking as invalid. :)

Thanks,

David

On Fri, 25 Feb 2022 at 20:48, David Christensen <david(dot)christensen(at)crunchydata(dot)com> wrote:
>> Cool. I think we can report an error instead of reading wal files,
>> if the tablespace, database, or relation is invalid. Does there any
>> WAL record that has invalid tablespace, database, or relation OID?
>
> The only sort of validity check we could do here is range checking for the underlying data types
> (which we certainly could/should add if it’s known to never be valid for the underlying types);

The invalid OID I said here is such as negative number and zero, for those
parameters, we do not need to read the WAL files, since it always invalid.

> non-existence of objects is a no-go, since that depends purely on the WAL range you are
> looking at and you’d have to, you know, scan it to see if it existed before marking as invalid. :)
>

Agreed.

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

On Fri, Feb 25, 2022 at 12:36 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)com> wrote:
>
> Greetings,
>
> This patch adds the ability to specify a RelFileNode and optional BlockNum to limit output of
> pg_waldump records to only those which match the given criteria. This should be more performant
> than `pg_waldump | grep` as well as more reliable given specific variations in output style
> depending on how the blocks are specified.
>
> This currently affects only the main fork, but we could presumably add the option to filter by fork
> as well, if that is considered useful.

Thanks for the patch. This is not adding something that users can't do
right now, but definitely improves the usability of the pg_waldump as
it avoids external filterings. Also, it can give the stats/info at
table and block level. So, +1 from my side.

I have some comments on the patch:

1) Let's use capitalized "OID" as is the case elsewhere in the documentation.
+ specified via tablespace oid, database oid, and relfilenode separated

2) Good idea to specify an example:
+ by slashes.
Something like, "by slashes, for instance, XXXX/XXXX/XXXX

3) Crossing 80 char limit
+/*
+ * Boolean to return whether the given WAL record matches a specific
relation and optional block
+ */
+static bool
+XLogRecordMatchesRelationBlock(XLogReaderState *record, RelFileNode
matchRnode, BlockNumber matchBlock
)

+ pg_log_error("could not parse block number \"%s\"", optarg);
+ pg_log_error("could not parse relation from \"%s\" (expecting
\"spc/dat/rel\")", optarg);

4) How about (expecting \"tablespace OID/database OID/relation OID\")?
Let's be clear.
+ pg_log_error("could not parse relation from \"%s\" (expecting
\"spc/dat/rel\")", optarg);

5) I would also see a need for "filter by FPW" i.e. list all WAL
records with "FPW".

6) How about "--block option requires --relation option" or some other
better phrasing?
+ pg_log_error("cannot filter by --block without also filtering --relation");

7) Extra new line between } and return false;
+ return true;
+ }
+ return false;
+}

8) Can we have this for-loop local variables instead of function level
variables?
+ RelFileNode rnode;
+ ForkNumber forknum;
+ BlockNumber blk;

Regards,
Bharath Rupireddy.

On Fri, Feb 25, 2022 at 7:33 AM Bharath Rupireddy <
bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:

> Thanks for the patch. This is not adding something that users can't do
> right now, but definitely improves the usability of the pg_waldump as
> it avoids external filterings. Also, it can give the stats/info at
> table and block level. So, +1 from my side.
>

Thanks for the feedback; I will be incorporating most of this into a new
version, with a couple of responses below.

> 3) Crossing 80 char limit
>

This is neither here nor there, but have we as a project considered
increasing that to something more modern? I know a lot of current projects
consider 132 to be a more reasonable limit. (Will reduce it down to that
for now, but consider this a vote towards increasing that limit.)

> 5) I would also see a need for "filter by FPW" i.e. list all WAL
> records with "FPW".
>

Yes, that wouldn't be too hard to add to this, can add to the next
version. We probably ought to also add the fork number as specifiable as
well. Other thoughts on could be some wildcard value in the relation part,
so `1234/23456/*` could filter WAL to a specific database only, say, or
some other multiple specifier, like `--block=1234,123456,121234`. (I
honestly consider this to be more advanced than we'd need to support in
this patch, but if probably wouldn't be too hard to add to it.)

Thanks,

David

On Fri, Feb 25, 2022 at 7:08 AM Japin Li <japinli(at)hotmail(dot)com> wrote:

>
> On Fri, 25 Feb 2022 at 20:48, David Christensen <
> david(dot)christensen(at)crunchydata(dot)com> wrote:
> >> Cool. I think we can report an error instead of reading wal files,
> >> if the tablespace, database, or relation is invalid. Does there any
> >> WAL record that has invalid tablespace, database, or relation OID?
> >
> > The only sort of validity check we could do here is range checking for
> the underlying data types
> > (which we certainly could/should add if it’s known to never be valid for
> the underlying types);
>
> The invalid OID I said here is such as negative number and zero, for those
> parameters, we do not need to read the WAL files, since it always invalid.
>

Agreed. Can add some additional range validation to the parsed values.

David

Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com> writes:

> On Fri, Feb 25, 2022 at 12:36 AM David Christensen
> <david(dot)christensen(at)crunchydata(dot)com> wrote:
>>
>> Greetings,
>>
>> This patch adds the ability to specify a RelFileNode and optional BlockNum to limit output of
>> pg_waldump records to only those which match the given criteria. This should be more performant
>> than `pg_waldump | grep` as well as more reliable given specific variations in output style
>> depending on how the blocks are specified.
>>
>> This currently affects only the main fork, but we could presumably add the option to filter by fork
>> as well, if that is considered useful.
>
> Thanks for the patch. This is not adding something that users can't do
> right now, but definitely improves the usability of the pg_waldump as
> it avoids external filterings. Also, it can give the stats/info at
> table and block level. So, +1 from my side.

Attached is V2 with additional feedback from this email, as well as the specification of the
ForkNumber and FPW as specifiable options.

Best,

David

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed

Hi

I am glad to find this patch here because it helps with my current development work, which involves a lot of debugging with the WAL records and this patch definitely make this much easier rather than using grep externally.

I have tried all of the new options added by the patch and every combination seems to result correctly.

The only comment I would have is in the documentation, where I would replace:
"Display only records touching the given block" with "Display only records associated with the given block"
"Display only records touching the given relation" with " Display only records associated with the given relation"

just to make it sound more formal. :)

best

Cary Huang
------------------
HighGo Software Canada
www.highgo.ca

On Sat, Feb 26, 2022 at 7:58 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)com> 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.

On Mon, Mar 21, 2022 at 4:36 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> On Sat, Feb 26, 2022 at 7:58 AM David Christensen
> <david(dot)christensen(at)crunchydata(dot)com> 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.

On Sun, Mar 20, 2022 at 11:56 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
wrote:

> On Mon, Mar 21, 2022 at 4:36 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
> wrote:
> > On Sat, Feb 26, 2022 at 7:58 AM David Christensen
> > <david(dot)christensen(at)crunchydata(dot)com> 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.
>

Do you need me to fix this, or are you incorporating that into a V4 of this
patch? (Similar to your fixup prior in this thread?)

Updated to include the V3 fixes as well as the unsigned int/enum fix.

>

On Tue, Mar 22, 2022 at 6:14 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)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?

On Mon, Mar 21, 2022 at 4:39 PM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:

[snip]

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?
>

Makes sense, but I think I'd actually thought it was +1 of the max forks,
so you give me more credit than I deserve in this case... :-)

> 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?
>

Agreed; made it default to all, with the ability to filter down if desired.

> 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".
>

Agreed; reworked to support the use case of only showing target forks.

> 3. It seems funny to have no short switch for --fork when everything
> else has one... what about -F?
>

Good idea; I'd hadn't seen capitals in the getopt list so didn't consider
them, but I like this.

Enclosed is v6, incorporating these fixes and docs tweaks.

Best,

David

On 21.03.22 05:55, Thomas Munro wrote:
> [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.

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

On Thu, Mar 24, 2022 at 9:53 AM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)com> wrote:
> On 21.03.22 05:55, Thomas Munro wrote:
> > [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.
>
> That's because ForkNum is a signed type. You will probably succeed if
> you use "%d" instead.

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.

On 2022-03-24 11:54:15 +1300, Thomas Munro wrote:
> 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.

/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.

On Tue, Mar 22, 2022 at 12:01 PM David Christensen
<david(dot)christensen(at)crunchydata(dot)com> wrote:
> Enclosed is v6, incorporating these fixes and docs tweaks.

Thanks!

I made a couple of minor changes in the docs, to wit: fixed
copy/paste-o "-F block" -> "-F fork", fork names didn't have initial
caps elsewhere, tablespace is better represented by
<replaceable>tblspc</replaceable> than <replaceable>tbl</replaceable>,
some minor wording changes to avoid constructions with "filter" where
it seemed to me a little ambiguous whether that means something is
included or excluded, and some other wording changes for consistency
with nearby paragraphs.

And... pushed.

On 23.03.22 23:54, Thomas Munro wrote:
>> That's because ForkNum is a signed type. You will probably succeed if
>> you use "%d" instead.
>
> 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.

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?

On 24.03.22 11:57, Peter Eisentraut wrote:
> On 23.03.22 23:54, Thomas Munro wrote:
>>> That's because ForkNum is a signed type.  You will probably succeed if
>>> you use "%d" instead.
>>
>> 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.
>
> 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?

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.

On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut
<peter(dot)eisentraut(at)enterprisedb(dot)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.

On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut
> <peter(dot)eisentraut(at)enterprisedb(dot)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.

> On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut
>>> <peter(dot)eisentraut(at)enterprisedb(dot)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.

David

On Fri, Mar 25, 2022 at 1:43 AM David Christensen
<david(dot)christensen(at)crunchydata(dot)com> wrote:
> > On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut
> >>> <peter(dot)eisentraut(at)enterprisedb(dot)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).

> On Mar 24, 2022, at 4:12 PM, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, Mar 25, 2022 at 1:43 AM David Christensen
> <david(dot)christensen(at)crunchydata(dot)com> wrote:
>>>> On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>>> On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>>>>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut
>>>>> <peter(dot)eisentraut(at)enterprisedb(dot)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?

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.

> 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).

On Fri, 25 Mar 2022 at 05:11, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> 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).

Thanks for updating the patch!

+ printf(_(" -x, --xid=XID only show records with transaction ID XID\n"));

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"));

+ 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))

It seems we should also check the dbNode.

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

On Fri, Mar 25, 2022 at 1:43 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
> + printf(_(" -x, --xid=XID only show records with transaction ID XID\n"));
>
> I think the description of transaction ID is enough, IIUC, XID is use in core,
> 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.

> 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"));

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.

> + 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))
>
> 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.

On Fri, 25 Mar 2022 at 08:55, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
> On Fri, Mar 25, 2022 at 1:43 PM Japin Li <japinli(at)hotmail(dot)com> wrote:
>> + printf(_(" -x, --xid=XID only show records with transaction ID XID\n"));
>>
>> I think the description of transaction ID is enough, IIUC, XID is use in core,
>> 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.
>
>> 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"));
>
> 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.
>

Thanks for your explanation!

>> + 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))
>>
>> 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
>

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

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