public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 2/2] review 16+ messages / 6 participants [nested] [flat]
* [PATCH 2/2] review @ 2021-03-09 20:09 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Tomas Vondra @ 2021-03-09 20:09 UTC (permalink / raw) --- src/backend/optimizer/path/costsize.c | 77 ++++++++++++++++++++------- src/backend/optimizer/path/pathkeys.c | 14 ++++- 2 files changed, 69 insertions(+), 22 deletions(-) diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c index 1639258aaf..f55a4f20e5 100644 --- a/src/backend/optimizer/path/costsize.c +++ b/src/backend/optimizer/path/costsize.c @@ -1755,6 +1755,8 @@ cost_recursive_union(Path *runion, Path *nrterm, Path *rterm) * is_fake_var * Workaround for generate_append_tlist() which generates fake Vars with * varno == 0, that will cause a fail of estimate_num_group() call + * + * XXX Ummm, why would estimate_num_group fail with this? */ static bool is_fake_var(Expr *expr) @@ -1828,27 +1830,53 @@ get_width_cost_multiplier(PlannerInfo *root, Expr *expr) * compute_cpu_sort_cost * compute CPU cost of sort (i.e. in-memory) * + * The main thing we need to calculate to estimate sort CPU costs is the number + * of calls to the comparator functions. The difficulty is that for multi-column + * sorts there may be different data types involved (for some of which the calls + * may be much more expensive). Furthermore, the columns may have very different + * number of distinct values - the higher the number, the fewer comparisons will + * be needed for the following columns. + * + * The algoritm is incremental - we add pathkeys one by one, and at each step we + * estimate the number of necessary comparisons (based on the number of distinct + * groups in the current pathkey prefix and the new pathkey), and the comparison + * costs (which is data type specific). + * + * Estimation of the number of comparisons is based on ideas from two sources: + * + * 1) "Algorithms" (course), Robert Sedgewick, Kevin Wayne [https://algs4.cs.princeton.edu/home/] + * + * 2) "Quicksort Is Optimal For Many Equal Keys" (paper), Sebastian Wild, + * arXiv:1608.04906v4 [cs.DS] 1 Nov 2017. [https://arxiv.org/abs/1608.04906] + * + * In term of that paper, let N - number of tuples, Xi - number of tuples with + * key Ki, then the estimate of number of comparisons is: + * + * log(N! / (X1! * X2! * ..)) ~ sum(Xi * log(N/Xi)) + * + * In our case all Xi are the same because now we don't have any estimation of + * group sizes, we have only know the estimate of number of groups (distinct + * values). In that case, formula becomes: + * + * N * log(NumberOfGroups) + * + * For multi-column sorts we need to estimate the number of comparisons for + * each individual column - for example with columns (c1, c2, ..., ck) we + * can estimate that number of comparions on ck is roughly + * + * ncomparisons(c1, c2, ..., ck) / ncomparisons(c1, c2, ..., c(k-1)) + * + * Let k be a column number, Gk - number of groups defined by k columns, and Fk + * the cost of the comparison is + * + * N * sum( Fk * log(Gk) ) + * + * Note: We also consider column witdth, not just the comparator cost. + * * NOTE: some callers currently pass NIL for pathkeys because they * can't conveniently supply the sort keys. In this case, it will fallback to * simple comparison cost estimate. - * - * Estimation algorithm is based on ideas from course Algorithms, - * Robert Sedgewick, Kevin Wayne, https://algs4.cs.princeton.edu/home/ and paper - * "Quicksort Is Optimal For Many Equal Keys", Sebastian Wild, - * arXiv:1608.04906v4 [cs.DS] 1 Nov 2017. - * - * In term of that papers, let N - number of tuples, Xi - number of tuples with - * key Ki, then estimation is: - * log(N! / (X1! * X2! * ..)) ~ sum(Xi * log(N/Xi)) - * In our case all Xi are the same because now we don't have an estimation of - * group sizes, we have only estimation of number of groups. In this case, - * formula becomes: N * log(NumberOfGroups). Next, to support correct estimation - * of multi-column sort we need separately compute each column, so, let k is a - * column number, Gk - number of groups defined by k columns: - * N * sum( Fk * log(Gk) ) - * Fk is a function costs (including width) for k columns. */ - static Cost compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, Cost comparison_cost, double tuples, double output_tuples, @@ -1862,7 +1890,7 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, bool has_fake_var = false; int i = 0; Oid prev_datatype = InvalidOid; - Cost funcCost = 0.; + Cost funcCost = 0.0; List *cache_varinfos = NIL; /* fallback if pathkeys is unknown */ @@ -1873,6 +1901,10 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, * a total number of tuple comparisons of N log2 K; but the constant * factor is a bit higher than for quicksort. Tweak it so that the * cost curve is continuous at the crossover point. + * + * XXX I suppose the "quicksort factor" references to 1.5 at the end + * of this function, but I'm not sure. I suggest we introduce some simple + * constants for that, instead of magic values. */ output_tuples = (heapSort) ? 2.0 * output_tuples : tuples; per_tuple_cost += 2.0 * cpu_operator_cost * LOG2(output_tuples); @@ -1888,7 +1920,6 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, * - per column comparison function cost * - we try to compute needed number of comparison per column */ - foreach(lc, pathkeys) { PathKey *pathkey = (PathKey*)lfirst(lc); @@ -1952,6 +1983,11 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, * Don't use DEFAULT_NUM_DISTINCT because it used for only one * column while here we try to estimate number of groups over * set of columns. + * + * XXX Perhaps this should use DEFAULT_NUM_DISTINCT at least to + * limit the calculated values, somehow? + * + * XXX What's the logic of the following formula? */ nGroups = ceil(2.0 + sqrt(tuples) * list_length(pathkeyExprs) / list_length(pathkeys)); @@ -1968,6 +2004,7 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, { if (tuplesPerPrevGroup < output_tuples) /* comparing only inside output_tuples */ + /* XXX why not to use the same multiplier (1.5)? */ correctedNGroups = ceil(2.0 * output_tuples / (tuplesPerPrevGroup / nGroups)); else @@ -1993,7 +2030,7 @@ compute_cpu_sort_cost(PlannerInfo *root, List *pathkeys, int nPresortedKeys, tuplesPerPrevGroup = ceil(1.5 * tuplesPerPrevGroup / nGroups); /* - * We could skip all followed columns for cost estimation, because we + * We could skip all following columns for cost estimation, because we * believe that tuples are unique by set ot previous columns */ if (tuplesPerPrevGroup <= 1.0) diff --git a/src/backend/optimizer/path/pathkeys.c b/src/backend/optimizer/path/pathkeys.c index 7beb32488a..b092c3e055 100644 --- a/src/backend/optimizer/path/pathkeys.c +++ b/src/backend/optimizer/path/pathkeys.c @@ -515,10 +515,19 @@ pathkey_sort_cost_comparator(const void *_a, const void *_b) return 0; return 1; } + /* * Order tail of list of group pathkeys by uniqueness descendetly. It allows to * speedup sorting. Returns newly allocated lists, old ones stay untouched. * n_preordered defines a head of list which order should be prevented. + * + * XXX But we're not generating this only based on uniqueness (that's a bad + * term anyway, because we're using ndistinct estimates, not uniqueness). + * We're also using the comparator cost to calculate the expected sort cost, + * and optimize that. + * + * XXX This should explain how we generate the values - all permutations for + * up to 4 values, etc. */ void get_cheapest_group_keys_order(PlannerInfo *root, double nrows, @@ -597,8 +606,9 @@ get_cheapest_group_keys_order(PlannerInfo *root, double nrows, else { /* - * Since v13 list_free() can clean list elements so for original list not to be modified it should be copied to - * a new one which can then be cleaned safely if needed. + * Since v13 list_free() can clean list elements so for original list + * not to be modified it should be copied to a new one which can then + * be cleaned safely if needed. */ new_group_pathkeys = list_copy(*group_pathkeys); nToPermute = nFreeKeys; -- 2.29.2 --------------A955BFDEDE369DCD57A45C5A-- ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-21 04:55 Thomas Munro <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Thomas Munro @ 2022-03-21 04:55 UTC (permalink / raw) To: David Christensen <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers On Mon, Mar 21, 2022 at 4:36 PM Thomas Munro <[email protected]> wrote: > On Sat, Feb 26, 2022 at 7:58 AM David Christensen > <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-23 20:53 Peter Eisentraut <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Peter Eisentraut @ 2022-03-23 20:53 UTC (permalink / raw) To: Thomas Munro <[email protected]>; David Christensen <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-23 22:54 Thomas Munro <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Thomas Munro @ 2022-03-23 22:54 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers On Thu, Mar 24, 2022 at 9:53 AM Peter Eisentraut <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 01:06 Andres Freund <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Andres Freund @ 2022-03-24 01:06 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 08:50 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Thomas Munro @ 2022-03-24 08:50 UTC (permalink / raw) To: David Christensen <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; pgsql-hackers On Tue, Mar 22, 2022 at 12:01 PM David Christensen <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 10:57 Peter Eisentraut <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Peter Eisentraut @ 2022-03-24 10:57 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers 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? ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 11:01 Peter Eisentraut <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Peter Eisentraut @ 2022-03-24 11:01 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 11:26 Thomas Munro <[email protected]> parent: Peter Eisentraut <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Thomas Munro @ 2022-03-24 11:26 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut <[email protected]> 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. Attachments: [text/x-patch] 0001-Use-fork-names-not-numbers-in-pg_waldump-option.patch (3.2K, ../../CA+hUKG+h-1qbQS0w0ARR73gw+Mv1fcxVZaFMycQS3JGc8iZiZA@mail.gmail.com/2-0001-Use-fork-names-not-numbers-in-pg_waldump-option.patch) download | inline diff: From 79060adc6db3e38c1935426145115c9eed39d85f Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 25 Mar 2022 00:22:01 +1300 Subject: [PATCH] Use fork names, not numbers, in pg_waldump option. Improvement for commit 127aea2a. Suggested-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com --- doc/src/sgml/ref/pg_waldump.sgml | 8 ++++---- src/bin/pg_waldump/pg_waldump.c | 20 +++++++++++++------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 981d3c9038..9e1b91683d 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -118,10 +118,10 @@ PostgreSQL documentation <listitem> <para> If provided, only display records that modify blocks in the given fork. - The valid values 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. + The valid values are <literal>main</literal> for the main fork, + <literal>fsm</literal> for the free space map, + <literal>vm</literal> for the visibility map, + and <literal>init</literal> for the init fork. </para> </listitem> </varlistentry> diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 92238f30c9..e878c90803 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -828,8 +828,8 @@ usage(void) 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(_(" -F, --fork=N only show records matching a specific fork number\n" - " (defaults to showing all)\n")); + printf(_(" -F, --fork=FORK only show records matching a specific fork;\n" + " valid fork names are main, fsm, vm, init\n")); printf(_(" -l, --relation=N/N/N only show records that affect 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" @@ -968,13 +968,19 @@ main(int argc, char **argv) break; case 'F': { - unsigned int forknum; + int forknum = InvalidForkNumber; - if (sscanf(optarg, "%u", &forknum) != 1 || - forknum > MAX_FORKNUM) + for (int i = 0; i <= MAX_FORKNUM; ++i) { - pg_log_error("could not parse valid fork number (0..%d) \"%s\"", - MAX_FORKNUM, optarg); + if (strcmp(optarg, forkNames[i]) == 0) + { + forknum = i; + break; + } + } + if (forknum == InvalidForkNumber) + { + pg_log_error("could not parse fork \"%s\"", optarg); goto bad_argument; } config.filter_by_relation_forknum = (ForkNumber) forknum; -- 2.30.2 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 11:42 Thomas Munro <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Thomas Munro @ 2022-03-24 11:42 UTC (permalink / raw) To: Peter Eisentraut <[email protected]>; +Cc: David Christensen <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <[email protected]> wrote: > On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut > <[email protected]> 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. Attachments: [text/x-patch] v2-0001-Use-fork-names-not-numbers-in-pg_waldump-option.patch (3.1K, ../../CA+hUKGL+0PrQmEnnHidi1Vc-5f8Bqo4bpHAbuBk778c1k8Aiow@mail.gmail.com/2-v2-0001-Use-fork-names-not-numbers-in-pg_waldump-option.patch) download | inline diff: From e276c28414645022ead3f33e6a6bc11ae0479496 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 25 Mar 2022 00:22:01 +1300 Subject: [PATCH v2] Use fork names, not numbers, in pg_waldump option. Improvement for commit 127aea2a. Suggested-by: Peter Eisentraut <[email protected]> Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com --- doc/src/sgml/ref/pg_waldump.sgml | 8 ++++---- src/bin/pg_waldump/pg_waldump.c | 15 +++++++-------- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 981d3c9038..9e1b91683d 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -118,10 +118,10 @@ PostgreSQL documentation <listitem> <para> If provided, only display records that modify blocks in the given fork. - The valid values 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. + The valid values are <literal>main</literal> for the main fork, + <literal>fsm</literal> for the free space map, + <literal>vm</literal> for the visibility map, + and <literal>init</literal> for the init fork. </para> </listitem> </varlistentry> diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 92238f30c9..bb6b7576fd 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -828,8 +828,8 @@ usage(void) 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(_(" -F, --fork=N only show records matching a specific fork number\n" - " (defaults to showing all)\n")); + printf(_(" -F, --fork=FORK only show records matching a specific fork;\n" + " valid fork names are main, fsm, vm, init\n")); printf(_(" -l, --relation=N/N/N only show records that affect 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" @@ -968,16 +968,15 @@ main(int argc, char **argv) break; case 'F': { - unsigned int forknum; + ForkNumber forknum; - if (sscanf(optarg, "%u", &forknum) != 1 || - forknum > MAX_FORKNUM) + forknum = forkname_to_number(optarg); + if (forknum == InvalidForkNumber) { - pg_log_error("could not parse valid fork number (0..%d) \"%s\"", - MAX_FORKNUM, optarg); + pg_log_error("could not parse fork \"%s\"", optarg); goto bad_argument; } - config.filter_by_relation_forknum = (ForkNumber) forknum; + config.filter_by_relation_forknum = forknum; config.filter_by_extended = true; } break; -- 2.30.2 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 12:43 David Christensen <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: David Christensen @ 2022-03-24 12:43 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers > On Mar 24, 2022, at 6:43 AM, Thomas Munro <[email protected]> wrote: > > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <[email protected]> wrote: >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut >>> <[email protected]> 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 21:11 Thomas Munro <[email protected]> parent: David Christensen <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Thomas Munro @ 2022-03-24 21:11 UTC (permalink / raw) To: David Christensen <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers On Fri, Mar 25, 2022 at 1:43 AM David Christensen <[email protected]> wrote: > > On Mar 24, 2022, at 6:43 AM, Thomas Munro <[email protected]> wrote: > > On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <[email protected]> wrote: > >>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut > >>> <[email protected]> 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). Attachments: [text/x-patch] 0001-Improve-command-line-switches-in-pg_waldump-option.patch (7.3K, ../../CA+hUKGJL3LztRrLa=ba87_08cKnfoSJ3U1f=E+9o+wG7hwwDhg@mail.gmail.com/2-0001-Improve-command-line-switches-in-pg_waldump-option.patch) download | inline diff: From c4aac1b1f02ff44b8f6b5340283faf87b5eff509 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 25 Mar 2022 09:13:39 +1300 Subject: [PATCH] Improve command line switches in pg_waldump option. Improvements for commit 127aea2a: * use fork name for --fork, not number * use -R, -B as short switches for --relation, --block Suggested-by: Peter Eisentraut <[email protected]> Reviewed-by: David Christensen <[email protected]> Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com --- doc/src/sgml/ref/pg_waldump.sgml | 8 ++-- src/bin/pg_waldump/pg_waldump.c | 76 +++++++++++++++----------------- 2 files changed, 39 insertions(+), 45 deletions(-) diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 981d3c9038..9e1b91683d 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -118,10 +118,10 @@ PostgreSQL documentation <listitem> <para> If provided, only display records that modify blocks in the given fork. - The valid values 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. + The valid values are <literal>main</literal> for the main fork, + <literal>fsm</literal> for the free space map, + <literal>vm</literal> for the visibility map, + and <literal>init</literal> for the init fork. </para> </listitem> </varlistentry> diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 92238f30c9..75a3964263 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -825,12 +825,11 @@ usage(void) printf(_(" %s [OPTION]... [STARTSEG [ENDSEG]]\n"), progname); printf(_("\nOptions:\n")); printf(_(" -b, --bkp-details output detailed information about backup blocks\n")); + printf(_(" -B, --block=N with --relation, only show records that modify block N\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(_(" -F, --fork=N only show records matching a specific fork number\n" - " (defaults to showing all)\n")); - printf(_(" -l, --relation=N/N/N only show records that affect a specific relation\n")); + printf(_(" -F, --fork=FORK only show records that modify blocks in fork FORK;\n" + " valid names are main, fsm, vm, init\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" @@ -838,12 +837,13 @@ usage(void) printf(_(" -q, --quiet do not print any output, except for errors\n")); printf(_(" -r, --rmgr=RMGR only show records generated by resource manager RMGR;\n" " use --rmgr=list to list valid resource manager names\n")); + printf(_(" -R, --relation=T/D/R only show records that modify blocks in relation T/D/R\n")); printf(_(" -s, --start=RECPTR start reading at WAL location RECPTR\n")); printf(_(" -t, --timeline=TLI timeline from which to read log records\n" " (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(_(" -x, --xid=XID only show records with transaction ID XID\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")); @@ -946,7 +946,7 @@ main(int argc, char **argv) goto bad_argument; } - while ((option = getopt_long(argc, argv, "be:fF:k:l:n:p:qr:s:t:wx:z", + while ((option = getopt_long(argc, argv, "bB:e:fF:n:p:qr:R:s:t:wx:z", long_options, &optindex)) != -1) { switch (option) @@ -954,6 +954,16 @@ main(int argc, char **argv) case 'b': config.bkp_details = true; break; + case 'B': + 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; + config.filter_by_extended = true; + break; case 'e': if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2) { @@ -967,44 +977,12 @@ main(int argc, char **argv) config.follow = true; break; case 'F': + config.filter_by_relation_forknum = forkname_to_number(optarg); + if (config.filter_by_relation_forknum == InvalidForkNumber) { - unsigned int forknum; - - if (sscanf(optarg, "%u", &forknum) != 1 || - forknum > MAX_FORKNUM) - { - pg_log_error("could not parse valid fork number (0..%d) \"%s\"", - MAX_FORKNUM, optarg); - goto bad_argument; - } - config.filter_by_relation_forknum = (ForkNumber) forknum; - config.filter_by_extended = true; - } - 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; - config.filter_by_extended = 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.relNode)) - { - pg_log_error("could not parse valid relation from \"%s\"" - " (expecting \"tablespace OID/database OID/" - "relation filenode\")", optarg); + pg_log_error("could not parse fork \"%s\"", optarg); goto bad_argument; } - config.filter_by_relation_enabled = true; config.filter_by_extended = true; break; case 'n': @@ -1047,6 +1025,22 @@ main(int argc, char **argv) } } break; + case 'R': + 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)) + { + 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; + config.filter_by_extended = true; + break; case 's': if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2) { -- 2.30.2 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-24 22:40 David Christensen <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: David Christensen @ 2022-03-24 22:40 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers > On Mar 24, 2022, at 4:12 PM, Thomas Munro <[email protected]> wrote: > > On Fri, Mar 25, 2022 at 1:43 AM David Christensen > <[email protected]> wrote: >>>> On Mar 24, 2022, at 6:43 AM, Thomas Munro <[email protected]> wrote: >>> On Fri, Mar 25, 2022 at 12:26 AM Thomas Munro <[email protected]> wrote: >>>>> On Fri, Mar 25, 2022 at 12:01 AM Peter Eisentraut >>>>> <[email protected]> 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, and it 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). Attachments: [application/octet-stream] 0001-Improve-command-line-switches-in-pg_waldump-option.patch (7.3K, ../../[email protected]/2-0001-Improve-command-line-switches-in-pg_waldump-option.patch) download | inline diff: From c4aac1b1f02ff44b8f6b5340283faf87b5eff509 Mon Sep 17 00:00:00 2001 From: Thomas Munro <[email protected]> Date: Fri, 25 Mar 2022 09:13:39 +1300 Subject: [PATCH] Improve command line switches in pg_waldump option. Improvements for commit 127aea2a: * use fork name for --fork, not number * use -R, -B as short switches for --relation, --block Suggested-by: Peter Eisentraut <[email protected]> Reviewed-by: David Christensen <[email protected]> Discussion: https://postgr.es/m/3a4c2e93-7976-2320-fc0a-32097fe148a7%40enterprisedb.com --- doc/src/sgml/ref/pg_waldump.sgml | 8 ++-- src/bin/pg_waldump/pg_waldump.c | 76 +++++++++++++++----------------- 2 files changed, 39 insertions(+), 45 deletions(-) diff --git a/doc/src/sgml/ref/pg_waldump.sgml b/doc/src/sgml/ref/pg_waldump.sgml index 981d3c9038..9e1b91683d 100644 --- a/doc/src/sgml/ref/pg_waldump.sgml +++ b/doc/src/sgml/ref/pg_waldump.sgml @@ -118,10 +118,10 @@ PostgreSQL documentation <listitem> <para> If provided, only display records that modify blocks in the given fork. - The valid values 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. + The valid values are <literal>main</literal> for the main fork, + <literal>fsm</literal> for the free space map, + <literal>vm</literal> for the visibility map, + and <literal>init</literal> for the init fork. </para> </listitem> </varlistentry> diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c index 92238f30c9..75a3964263 100644 --- a/src/bin/pg_waldump/pg_waldump.c +++ b/src/bin/pg_waldump/pg_waldump.c @@ -825,12 +825,11 @@ usage(void) printf(_(" %s [OPTION]... [STARTSEG [ENDSEG]]\n"), progname); printf(_("\nOptions:\n")); printf(_(" -b, --bkp-details output detailed information about backup blocks\n")); + printf(_(" -B, --block=N with --relation, only show records that modify block N\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(_(" -F, --fork=N only show records matching a specific fork number\n" - " (defaults to showing all)\n")); - printf(_(" -l, --relation=N/N/N only show records that affect a specific relation\n")); + printf(_(" -F, --fork=FORK only show records that modify blocks in fork FORK;\n" + " valid names are main, fsm, vm, init\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" @@ -838,12 +837,13 @@ usage(void) printf(_(" -q, --quiet do not print any output, except for errors\n")); printf(_(" -r, --rmgr=RMGR only show records generated by resource manager RMGR;\n" " use --rmgr=list to list valid resource manager names\n")); + printf(_(" -R, --relation=T/D/R only show records that modify blocks in relation T/D/R\n")); printf(_(" -s, --start=RECPTR start reading at WAL location RECPTR\n")); printf(_(" -t, --timeline=TLI timeline from which to read log records\n" " (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(_(" -x, --xid=XID only show records with transaction ID XID\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")); @@ -946,7 +946,7 @@ main(int argc, char **argv) goto bad_argument; } - while ((option = getopt_long(argc, argv, "be:fF:k:l:n:p:qr:s:t:wx:z", + while ((option = getopt_long(argc, argv, "bB:e:fF:n:p:qr:R:s:t:wx:z", long_options, &optindex)) != -1) { switch (option) @@ -954,6 +954,16 @@ main(int argc, char **argv) case 'b': config.bkp_details = true; break; + case 'B': + 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; + config.filter_by_extended = true; + break; case 'e': if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2) { @@ -967,44 +977,12 @@ main(int argc, char **argv) config.follow = true; break; case 'F': + config.filter_by_relation_forknum = forkname_to_number(optarg); + if (config.filter_by_relation_forknum == InvalidForkNumber) { - unsigned int forknum; - - if (sscanf(optarg, "%u", &forknum) != 1 || - forknum > MAX_FORKNUM) - { - pg_log_error("could not parse valid fork number (0..%d) \"%s\"", - MAX_FORKNUM, optarg); - goto bad_argument; - } - config.filter_by_relation_forknum = (ForkNumber) forknum; - config.filter_by_extended = true; - } - 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; - config.filter_by_extended = 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.relNode)) - { - pg_log_error("could not parse valid relation from \"%s\"" - " (expecting \"tablespace OID/database OID/" - "relation filenode\")", optarg); + pg_log_error("could not parse fork \"%s\"", optarg); goto bad_argument; } - config.filter_by_relation_enabled = true; config.filter_by_extended = true; break; case 'n': @@ -1047,6 +1025,22 @@ main(int argc, char **argv) } } break; + case 'R': + 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)) + { + 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; + config.filter_by_extended = true; + break; case 's': if (sscanf(optarg, "%X/%X", &xlogid, &xrecoff) != 2) { -- 2.30.2 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-25 00:43 Japin Li <[email protected]> parent: Thomas Munro <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Japin Li @ 2022-03-25 00:43 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: David Christensen <[email protected]>; Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers; [email protected] On Fri, 25 Mar 2022 at 05:11, Thomas Munro <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-25 00:55 Thomas Munro <[email protected]> parent: Japin Li <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Thomas Munro @ 2022-03-25 00:55 UTC (permalink / raw) To: Japin Li <[email protected]>; +Cc: David Christensen <[email protected]>; Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers; PostgreSQL Hackers <[email protected]> On Fri, Mar 25, 2022 at 1:43 PM Japin Li <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: [PATCH] add relation and block-level filtering to pg_waldump @ 2022-03-25 01:32 Japin Li <[email protected]> parent: Thomas Munro <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Japin Li @ 2022-03-25 01:32 UTC (permalink / raw) To: Thomas Munro <[email protected]>; +Cc: David Christensen <[email protected]>; Peter Eisentraut <[email protected]>; Bharath Rupireddy <[email protected]>; pgsql-hackers; PostgreSQL Hackers <[email protected]> On Fri, 25 Mar 2022 at 08:55, Thomas Munro <[email protected]> wrote: > On Fri, Mar 25, 2022 at 1:43 PM Japin Li <[email protected]> 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. ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2022-03-25 01:32 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-03-09 20:09 [PATCH 2/2] review Tomas Vondra <[email protected]> 2022-03-21 04:55 Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-23 20:53 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Peter Eisentraut <[email protected]> 2022-03-23 22:54 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-24 01:06 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Andres Freund <[email protected]> 2022-03-24 10:57 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Peter Eisentraut <[email protected]> 2022-03-24 11:01 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Peter Eisentraut <[email protected]> 2022-03-24 11:26 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-24 11:42 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-24 12:43 ` Re: [PATCH] add relation and block-level filtering to pg_waldump David Christensen <[email protected]> 2022-03-24 21:11 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-24 22:40 ` Re: [PATCH] add relation and block-level filtering to pg_waldump David Christensen <[email protected]> 2022-03-25 00:43 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Japin Li <[email protected]> 2022-03-25 00:55 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]> 2022-03-25 01:32 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Japin Li <[email protected]> 2022-03-24 08:50 ` Re: [PATCH] add relation and block-level filtering to pg_waldump Thomas Munro <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox