On Fri, Feb 25, 2022 at 12:36 AM David Christensen
<david.christen...@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.
