Thread: [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
From 9194b2cb07172e636030b9b4e979b7f2caf7cbc0 Mon Sep 17 00:00:00 2001
From: David Christensen <david.christensen@crunchydata.com>
Date: Thu, 24 Feb 2022 11:00:46 -0600
Subject: [PATCH] Add relation/block filtering to pg_waldump
This feature allows you to only output records that are targeting a specific RelFileNode and optional
BlockNumber within this relation. Currently only applies this filter to the relation's main fork.
---
doc/src/sgml/ref/pg_waldump.sgml | 23 ++++++++++
src/bin/pg_waldump/pg_waldump.c | 74 +++++++++++++++++++++++++++++++-
2 files changed, 96 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 5735a161ce..c953703bc8 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -100,6 +100,29 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-k <replaceable>block</replaceable></option></term>
+ <term><option>--block=<replaceable>block</replaceable></option></term>
+ <listitem>
+ <para>
+ Display only records touching the given block. (Requires also
+ providing the relation via <option>--relation</option>.)
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><option>-l
<replaceable>tbl</replaceable>/<replaceable>db</replaceable>/<replaceable>rel</replaceable></option></term>
+
<term><option>--relation=<replaceable>tbl</replaceable>/<replaceable>db</replaceable>/<replaceable>rel</replaceable></option></term>
+ <listitem>
+ <para>
+ Display only records touching the given relation. The relation is
+ specified via tablespace oid, database oid, and relfilenode separated
+ by slashes.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-n <replaceable>limit</replaceable></option></term>
<term><option>--limit=<replaceable>limit</replaceable></option></term>
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index a6251e1a96..faae547a5c 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -55,6 +55,10 @@ typedef struct XLogDumpConfig
bool filter_by_rmgr_enabled;
TransactionId filter_by_xid;
bool filter_by_xid_enabled;
+ RelFileNode filter_by_relation;
+ bool filter_by_relation_enabled;
+ BlockNumber filter_by_relation_block;
+ bool filter_by_relation_block_enabled;
} XLogDumpConfig;
typedef struct Stats
@@ -394,6 +398,34 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
return count;
}
+/*
+ * Boolean to return whether the given WAL record matches a specific relation and optional block
+ */
+static bool
+XLogRecordMatchesRelationBlock(XLogReaderState *record, RelFileNode matchRnode, BlockNumber matchBlock)
+{
+ RelFileNode rnode;
+ ForkNumber forknum;
+ BlockNumber blk;
+ int block_id;
+
+ for (block_id = 0; block_id <= record->max_block_id; block_id++)
+ {
+ if (!XLogRecHasBlockRef(record, block_id))
+ continue;
+
+ XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk);
+
+ if (forknum == MAIN_FORKNUM &&
+ matchRnode.spcNode == rnode.spcNode &&
+ matchRnode.dbNode == rnode.dbNode &&
+ matchRnode.relNode == rnode.relNode &&
+ (matchBlock == InvalidBlockNumber || matchBlock == blk))
+ return true;
+ }
+ return false;
+}
+
/*
* Calculate the size of a record, split into !FPI and FPI parts.
*/
@@ -767,6 +799,8 @@ usage(void)
printf(_(" -b, --bkp-details output detailed information about backup blocks\n"));
printf(_(" -e, --end=RECPTR stop reading at WAL location RECPTR\n"));
printf(_(" -f, --follow keep retrying after reaching end of WAL\n"));
+ printf(_(" -k, --block=N only show records matching a given relation block (requires -l)\n"));
+ printf(_(" -l, --relation=N/N/N only show records that touch a specific relation\n"));
printf(_(" -n, --limit=N number of records to display\n"));
printf(_(" -p, --path=PATH directory in which to find log segment files or a\n"
" directory with a ./pg_wal that contains such files\n"
@@ -802,12 +836,14 @@ main(int argc, char **argv)
static struct option long_options[] = {
{"bkp-details", no_argument, NULL, 'b'},
+ {"block", required_argument, NULL, 'k'},
{"end", required_argument, NULL, 'e'},
{"follow", no_argument, NULL, 'f'},
{"help", no_argument, NULL, '?'},
{"limit", required_argument, NULL, 'n'},
{"path", required_argument, NULL, 'p'},
{"quiet", no_argument, NULL, 'q'},
+ {"relation", required_argument, NULL, 'l'},
{"rmgr", required_argument, NULL, 'r'},
{"start", required_argument, NULL, 's'},
{"timeline", required_argument, NULL, 't'},
@@ -860,6 +896,8 @@ main(int argc, char **argv)
config.filter_by_rmgr_enabled = false;
config.filter_by_xid = InvalidTransactionId;
config.filter_by_xid_enabled = false;
+ config.filter_by_relation_enabled = false;
+ config.filter_by_relation_block_enabled = false;
config.stats = false;
config.stats_per_record = false;
@@ -872,7 +910,7 @@ main(int argc, char **argv)
goto bad_argument;
}
- while ((option = getopt_long(argc, argv, "be:fn:p:qr:s:t:x:z",
+ while ((option = getopt_long(argc, argv, "be:fk:l:n:p:qr:s:t:x:z",
long_options, &optindex)) != -1)
{
switch (option)
@@ -892,6 +930,25 @@ main(int argc, char **argv)
case 'f':
config.follow = true;
break;
+ case 'k':
+ if (sscanf(optarg, "%ul", &config.filter_by_relation_block) != 1)
+ {
+ pg_log_error("could not parse block number \"%s\"", optarg);
+ goto bad_argument;
+ }
+ config.filter_by_relation_block_enabled = true;
+ break;
+ case 'l':
+ if (sscanf(optarg, "%d/%d/%d",
+ &config.filter_by_relation.spcNode,
+ &config.filter_by_relation.dbNode,
+ &config.filter_by_relation.relNode) != 3)
+ {
+ pg_log_error("could not parse relation from \"%s\" (expecting \"spc/dat/rel\")", optarg);
+ goto bad_argument;
+ }
+ config.filter_by_relation_enabled = true;
+ break;
case 'n':
if (sscanf(optarg, "%d", &config.stop_after_records) != 1)
{
@@ -978,6 +1035,12 @@ main(int argc, char **argv)
}
}
+ if (config.filter_by_relation_block_enabled && !config.filter_by_relation_enabled)
+ {
+ pg_log_error("cannot filter by --block without also filtering --relation");
+ goto bad_argument;
+ }
+
if ((optind + 2) < argc)
{
pg_log_error("too many command-line arguments (first is \"%s\")",
@@ -1150,6 +1213,15 @@ main(int argc, char **argv)
config.filter_by_xid != record->xl_xid)
continue;
+ /* check for extended filtering */
+ if (config.filter_by_relation_enabled &&
+ !XLogRecordMatchesRelationBlock(
+ xlogreader_state,
+ config.filter_by_relation,
+ config.filter_by_relation_block_enabled ? config.filter_by_relation_block : InvalidBlockNumber
+ ))
+ continue;
+
/* perform any per-record work */
if (!config.quiet)
{
--
2.32.0 (Apple Git-132)
--
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:
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/shouldadd if it’s known to never be valid for the underlying types); non-existence of objects is a no-go, since thatdepends purely on the WAL range you are looking at and you’d have to, you know, scan it to see if it existed before markingas 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
From 1b04f04317d364006371bdab0db9086f79138b25 Mon Sep 17 00:00:00 2001
From: David Christensen <david.christensen@crunchydata.com>
Date: Fri, 25 Feb 2022 12:52:56 -0600
Subject: [PATCH] Add additional filtering options to pg_waldump
This feature allows you to only output records that are targeting a specific RelFileNode and optional
BlockNumber within this relation, while specifying which ForkNum you want to filter to.
We also add the independent ability to filter via Full Page Write.
---
doc/src/sgml/ref/pg_waldump.sgml | 48 ++++++++++++
src/bin/pg_waldump/pg_waldump.c | 128 ++++++++++++++++++++++++++++++-
2 files changed, 175 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml
index 5735a161ce..f157175764 100644
--- a/doc/src/sgml/ref/pg_waldump.sgml
+++ b/doc/src/sgml/ref/pg_waldump.sgml
@@ -100,6 +100,44 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-k <replaceable>block</replaceable></option></term>
+ <term><option>--block=<replaceable>block</replaceable></option></term>
+ <listitem>
+ <para>
+ Display only records touching the given block. (Requires also
+ providing the relation via <option>--relation</option>.)
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><option>--fork=<replaceable>fork</replaceable></option></term>
+ <listitem>
+ <para>
+ When using the <option>--relation</option> filter, output only records
+ from the given fork. The valid values here are <literal>0</literal>
+ for the main fork, <literal>1</literal> for the Free Space
+ Map, <literal>2</literal> for the Visibility Map,
+ and <literal>3</literal> for the Init fork. If unspecified, defaults
+ to the main fork.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><option>-l
<replaceable>tbl</replaceable>/<replaceable>db</replaceable>/<replaceable>rel</replaceable></option></term>
+
<term><option>--relation=<replaceable>tbl</replaceable>/<replaceable>db</replaceable>/<replaceable>rel</replaceable></option></term>
+ <listitem>
+ <para>
+ Display only records touching the given relation. The relation is
+ specified via tablespace OID, database OID, and relfilenode separated
+ by slashes, for instance, <literal>1234/12345/12345</literal>. This
+ is the same format used for relations in the WAL dump output.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-n <replaceable>limit</replaceable></option></term>
<term><option>--limit=<replaceable>limit</replaceable></option></term>
@@ -183,6 +221,16 @@ PostgreSQL documentation
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>-w</option></term>
+ <term><option>--fullpage</option></term>
+ <listitem>
+ <para>
+ Filter records to only those which have full page writes.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-x <replaceable>xid</replaceable></option></term>
<term><option>--xid=<replaceable>xid</replaceable></option></term>
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index a6251e1a96..a527cd4dc6 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -55,6 +55,12 @@ typedef struct XLogDumpConfig
bool filter_by_rmgr_enabled;
TransactionId filter_by_xid;
bool filter_by_xid_enabled;
+ RelFileNode filter_by_relation;
+ bool filter_by_relation_enabled;
+ BlockNumber filter_by_relation_block;
+ bool filter_by_relation_block_enabled;
+ ForkNumber filter_by_relation_forknum;
+ bool filter_by_fpw;
} XLogDumpConfig;
typedef struct Stats
@@ -394,6 +400,56 @@ WALDumpReadPage(XLogReaderState *state, XLogRecPtr targetPagePtr, int reqLen,
return count;
}
+/*
+ * Boolean to return whether the given WAL record matches a specific relation and optional block
+ */
+static bool
+XLogRecordMatchesRelationBlock(XLogReaderState *record, RelFileNode matchRnode, BlockNumber matchBlock, ForkNumber
matchFork)
+{
+ int block_id;
+
+ for (block_id = 0; block_id <= record->max_block_id; block_id++)
+ {
+ RelFileNode rnode;
+ ForkNumber forknum;
+ BlockNumber blk;
+
+ if (!XLogRecHasBlockRef(record, block_id))
+ continue;
+
+ XLogRecGetBlockTag(record, block_id, &rnode, &forknum, &blk);
+
+ if (forknum == matchFork &&
+ matchRnode.spcNode == rnode.spcNode &&
+ matchRnode.dbNode == rnode.dbNode &&
+ matchRnode.relNode == rnode.relNode &&
+ (matchBlock == InvalidBlockNumber || matchBlock == blk))
+ return true;
+ }
+
+ return false;
+}
+
+/*
+ * Boolean to return whether the given WAL record contains a full page write
+ */
+static bool
+XLogRecordHasFPW(XLogReaderState *record)
+{
+ int block_id;
+
+ for (block_id = 0; block_id <= record->max_block_id; block_id++)
+ {
+ if (!XLogRecHasBlockRef(record, block_id))
+ continue;
+
+ if (XLogRecHasBlockImage(record, block_id))
+ return true;
+ }
+
+ return false;
+}
+
/*
* Calculate the size of a record, split into !FPI and FPI parts.
*/
@@ -767,6 +823,10 @@ usage(void)
printf(_(" -b, --bkp-details output detailed information about backup blocks\n"));
printf(_(" -e, --end=RECPTR stop reading at WAL location RECPTR\n"));
printf(_(" -f, --follow keep retrying after reaching end of WAL\n"));
+ printf(_(" -k, --block=N with --relation, only show records matching this block\n"));
+ printf(_(" --fork=N with --relation, only show records matching this fork\n"
+ " (defaults to 0, the main fork)\n"));
+ printf(_(" -l, --relation=N/N/N only show records that touch a specific relation\n"));
printf(_(" -n, --limit=N number of records to display\n"));
printf(_(" -p, --path=PATH directory in which to find log segment files or a\n"
" directory with a ./pg_wal that contains such files\n"
@@ -779,6 +839,7 @@ usage(void)
" (default: 1 or the value used in STARTSEG)\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -x, --xid=XID only show records with transaction ID XID\n"));
+ printf(_(" -w, --fullpage only show records with a full page write\n"));
printf(_(" -z, --stats[=record] show statistics instead of records\n"
" (optionally, show per-record statistics)\n"));
printf(_(" -?, --help show this help, then exit\n"));
@@ -802,12 +863,16 @@ main(int argc, char **argv)
static struct option long_options[] = {
{"bkp-details", no_argument, NULL, 'b'},
+ {"block", required_argument, NULL, 'k'},
{"end", required_argument, NULL, 'e'},
{"follow", no_argument, NULL, 'f'},
+ {"fork", required_argument, NULL, 1},
+ {"fullpage", no_argument, NULL, 'w'},
{"help", no_argument, NULL, '?'},
{"limit", required_argument, NULL, 'n'},
{"path", required_argument, NULL, 'p'},
{"quiet", no_argument, NULL, 'q'},
+ {"relation", required_argument, NULL, 'l'},
{"rmgr", required_argument, NULL, 'r'},
{"start", required_argument, NULL, 's'},
{"timeline", required_argument, NULL, 't'},
@@ -860,6 +925,10 @@ main(int argc, char **argv)
config.filter_by_rmgr_enabled = false;
config.filter_by_xid = InvalidTransactionId;
config.filter_by_xid_enabled = false;
+ config.filter_by_relation_enabled = false;
+ config.filter_by_relation_block_enabled = false;
+ config.filter_by_relation_forknum = MAIN_FORKNUM;
+ config.filter_by_fpw = false;
config.stats = false;
config.stats_per_record = false;
@@ -872,7 +941,7 @@ main(int argc, char **argv)
goto bad_argument;
}
- while ((option = getopt_long(argc, argv, "be:fn:p:qr:s:t:x:z",
+ while ((option = getopt_long(argc, argv, "be:fk:l:n:p:qr:s:t:wx:z",
long_options, &optindex)) != -1)
{
switch (option)
@@ -892,6 +961,41 @@ main(int argc, char **argv)
case 'f':
config.follow = true;
break;
+ case 1: /* fork number */
+ if (sscanf(optarg, "%u", &config.filter_by_relation_forknum) != 1 ||
+ config.filter_by_relation_forknum >= MAX_FORKNUM)
+ {
+ pg_log_error("could not parse valid fork number (0..%d) \"%s\"",
+ MAX_FORKNUM - 1, optarg);
+ goto bad_argument;
+ }
+ break;
+ case 'k':
+ if (sscanf(optarg, "%u", &config.filter_by_relation_block) != 1 ||
+ !BlockNumberIsValid(config.filter_by_relation_block))
+ {
+ pg_log_error("could not parse valid block number \"%s\"", optarg);
+ goto bad_argument;
+ }
+ config.filter_by_relation_block_enabled = true;
+ break;
+ case 'l':
+ 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.dbNode) ||
+ !OidIsValid(config.filter_by_relation.relNode)
+ )
+ {
+ pg_log_error("could not parse valid relation from \"%s\"/"
+ " (expecting \"tablespace OID/database OID/"
+ "relation filenode\")", optarg);
+ goto bad_argument;
+ }
+ config.filter_by_relation_enabled = true;
+ break;
case 'n':
if (sscanf(optarg, "%d", &config.stop_after_records) != 1)
{
@@ -949,6 +1053,9 @@ main(int argc, char **argv)
goto bad_argument;
}
break;
+ case 'w':
+ config.filter_by_fpw = true;
+ break;
case 'x':
if (sscanf(optarg, "%u", &config.filter_by_xid) != 1)
{
@@ -978,6 +1085,12 @@ main(int argc, char **argv)
}
}
+ if (config.filter_by_relation_block_enabled && !config.filter_by_relation_enabled)
+ {
+ pg_log_error("--block option requires --relation option to be specified");
+ goto bad_argument;
+ }
+
if ((optind + 2) < argc)
{
pg_log_error("too many command-line arguments (first is \"%s\")",
@@ -1150,6 +1263,19 @@ main(int argc, char **argv)
config.filter_by_xid != record->xl_xid)
continue;
+ /* check for extended filtering */
+ if (config.filter_by_relation_enabled &&
+ !XLogRecordMatchesRelationBlock(
+ xlogreader_state,
+ config.filter_by_relation,
+ config.filter_by_relation_block_enabled ? config.filter_by_relation_block : InvalidBlockNumber,
+ config.filter_by_relation_forknum
+ ))
+ continue;
+
+ if (config.filter_by_fpw && !XLogRecordHasFPW(xlogreader_state))
+ continue;
+
/* perform any per-record work */
if (!config.quiet)
{
--
2.32.0 (Apple Git-132)
--
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 withthe 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.
Attachment
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.
Attachment
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?
Attachment
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
Attachment
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.
On Tue, Mar 22, 2022 at 12:01 PM David Christensen <david.christensen@crunchydata.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.eisentraut@enterprisedb.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.
Attachment
On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas.munro@gmail.com> wrote: > On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut > <peter.eisentraut@enterprisedb.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.
Attachment
> On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas.munro@gmail.com> wrote: >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut >>> <peter.eisentraut@enterprisedb.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.christensen@crunchydata.com> wrote: > > On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas.munro@gmail.com> wrote: > >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut > >>> <peter.eisentraut@enterprisedb.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).
Attachment
> On Mar 24, 2022, at 4:12 PM, Thomas Munro <thomas.munro@gmail.com> wrote: > > On Fri, Mar 25, 2022 at 1:43 AM David Christensen > <david.christensen@crunchydata.com> wrote: >>>> On Mar 24, 2022, at 6:43 AM, Thomas Munro <thomas.munro@gmail.com> wrote: >>> On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <thomas.munro@gmail.com> wrote: >>>>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut >>>>> <peter.eisentraut@enterprisedb.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, andit 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).
Attachment
On Fri, 25 Mar 2022 at 05:11, Thomas Munro <thomas.munro@gmail.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@hotmail.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.munro@gmail.com> wrote:
> On Fri, Mar 25, 2022 at 1:43 PM Japin Li <japinli@hotmail.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.