public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bharath Rupireddy <[email protected]>
To: David Christensen <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Subject: Re: [PATCH] add relation and block-level filtering to pg_waldump
Date: Fri, 25 Feb 2022 19:03:38 +0530
Message-ID: <CALj2ACXjZRWXvUhvUh7xJeRJd7i40KXdS7p6eHHq+q4+xUNf9w@mail.gmail.com> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>

On Fri, Feb 25, 2022 at 12:36 AM David Christensen
<[email protected]> 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.

view thread (2+ messages)

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected]
  Subject: Re: [PATCH] add relation and block-level filtering to pg_waldump
  In-Reply-To: <CALj2ACXjZRWXvUhvUh7xJeRJd7i40KXdS7p6eHHq+q4+xUNf9w@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox