MIME-Version: 1.0
References: <lzzgmgm6e5.fsf@veeddrois.attlocal.net>
In-Reply-To: <lzzgmgm6e5.fsf@veeddrois.attlocal.net>
From: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Date: Fri, 25 Feb 2022 19:03:38 +0530
Message-ID: <CALj2ACXjZRWXvUhvUh7xJeRJd7i40KXdS7p6eHHq+q4+xUNf9w@mail.gmail.com>
Subject: Re: [PATCH] add relation and block-level filtering to pg_waldump
To: David Christensen <david.christensen@crunchydata.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: <https://www.postgresql.org/message-id/CALj2ACXjZRWXvUhvUh7xJeRJd7i40KXdS7p6eHHq%2Bq4%2BxUNf9w%40mail.gmail.com>
Precedence: bulk

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.