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

On Thu, Feb 24, 2022 at 11:06 AM David Christensen
<david.christensen@crunchydata.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.christensen@crunchydata.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.christensen@crunchydata.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.christensen@crunchydata.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.rupireddyforpostgres@gmail.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@hotmail.com> wrote:

On Fri, 25 Feb 2022 at 20:48, David Christensen <
david.christensen@crunchydata.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.rupireddyforpostgres@gmail.com> writes:

On Fri, Feb 25, 2022 at 12:36 AM David Christensen
<david.christensen@crunchydata.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.christensen@crunchydata.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.munro@gmail.com> wrote:

On Sat, Feb 26, 2022 at 7:58 AM David Christensen
<david.christensen@crunchydata.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.munro@gmail.com>
wrote:

On Mon, Mar 21, 2022 at 4:36 PM Thomas Munro <thomas.munro@gmail.com>
wrote:

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

On Mon, Mar 21, 2022 at 4:39 PM Thomas Munro <thomas.munro@gmail.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.eisentraut@enterprisedb.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.