Hi David, Thanks for looking into this. On Fri, May 31, 2024 at 2:19 AM David Christensen <david...@pgguru.net> wrote:
> Hello, > > I am looking through some outstanding CommitFest entries; I wonder if > this particular patch is already effectively fixed by 5278d0a2, which > is both attributed to the original author as well as an extremely > similar approach. Can this entry > (https://commitfest.postgresql.org/48/4553/) be closed? > This is different. But it needs a rebase. PFA rebased patch. The revised commit message explains the change better. Here are numbers revised after 5278d0a2. Since the code changes only affect partitionwise join code path, I am reporting only partition wise join numbers. The first column reports the number of joining relations, each having 1000 partitions. Rest of the column names are self-explanatory. Planning memory used: num_joins | master | patched | memory saved | memory saved -----------+---------+---------+--------------+---------------------------- 2 | 31 MB | 30 MB | 525 kB | 1.68% 3 | 111 MB | 107 MB | 4588 kB | 4.03% 4 | 339 MB | 321 MB | 17 MB | 5.13% 5 | 1062 MB | 967 MB | 95 MB | 8.96% Here's planning time measurements. num_joins | master (ms) | patched (ms) | change in planning time (ms) | change in planning time -----------+-------------+--------------+------------------------------+--------------------------------------- 2 | 187.86 | 177.27 | 10.59 | 5.64% 3 | 721.81 | 758.80 | -36.99 | -5.12% 4 | 2239.87 | 2236.19 | 3.68 | 0.16% 5 | 6830.86 | 7027.76 | -196.90 | -2.88% The patch calls find_appinfos_by_relids() only once (instead of twice), calls bms_union() on child relids only once, allocates and deallocates appinfos array only once saving some CPU cycles but spends some CPU cycles in bms_free(). There's expected to be a net small saving in planning time. But above numbers show planning time increases in some cases and decreases in some cases. Repeating the experiment multiple times does not show a stable increase or decrease. But the variations all being within noise levels. We could conclude that the patch does not affect planning time adversely. The scripts I used originally [1] need a revision too since EXPLAIN MEMORY output has changed. PFA the scripts. setup.sql creates the required functions and tables. queries.sql EXPLAINs queries with different number of joining relations collecting planning time and memory numbers in a table (msmts). We need to change psql variable code to 'patched' or 'master' when using master + patches or master branch respectively. Following queries are used to report above measurements from msmt. planning memory: select p.num_joins, pg_size_pretty(m.average * 1024) "master", pg_size_pretty(p.average * 1024) as "patched", pg_size_pretty((m.average - p.average) * 1024) "memory saved", ((m.average - p.average) * 100/m.average)::numeric(42, 2) "memory saved in percentage" from msmts p, msmts m where p.num_joins = m.num_joins and p.variant = m.variant and p.code = 'patched' and m.code = 'master' and p.measurement = m.measurement and p.measurement = 'planning memory' and p.variant = 'pwj' order by num_joins; planning time: select p.num_joins, m.average "master (ms)", p.average "patched (ms)", m.average - p.average "change in planning time (ms)", ((m.average - p. average) * 100/m.average)::numeric(42, 2) "change in planning time as percentage" from msmts p, msmts m where p.num_joins = m.num_joins and p.variant = m.var iant and p.code = 'patched' and m.code = 'master' and p.measurement = m.measurement and p.measurement = 'planning time' and p.variant = 'pwj' order by p.vari ant, num_joins; [1] https://www.postgresql.org/message-id/CAExHW5snUW7pD2RdtaBa1T_TqJYaY6W_YPVjWDrgSf33i-0uqA%40mail.gmail.com -- Best Wishes, Ashutosh Bapat
setup.sql
Description: application/sql
queries.sql
Description: application/sql
From 4a37f580806a388e5d78e452426b99f738a583e5 Mon Sep 17 00:00:00 2001 From: Ashutosh Bapat <ashutosh.ba...@enterprisedb.com> Date: Wed, 26 Jul 2023 12:08:55 +0530 Subject: [PATCH] Avoid repeated computations in try_partitionwise_join() and minions try_partitionwise_join() computes bms_union() of Bitmapsets representing joining child relations and fetches AppendRelInfos corresponding child base relations participating in the join. The same computation is repeated in build_child_join_rel(). Avoid this repeated computation by performing it only once in try_partitionwise_join() and passing the AppendRelInfos gathered there to build_child_join_rel(). Bitmapsets representing child relids consume large memory when thousands of partitions are involved. By performing the bms_union() only once and freeing the result when it's no longer required, we save memory. The memory savings are considerable when many partitioned tables with thousands of partitions are joined using partitionwise join. Author: Ashutosh Bapat Discussion: https://www.postgresql.org/message-id/flat/CAExHW5snUW7pD2RdtaBa1T_TqJYaY6W_YPVjWDrgSf33i-0uqA%40mail.gmail.com#1f9e0950518310dd300e21574979646f --- src/backend/optimizer/path/joinrels.c | 10 ++++++---- src/backend/optimizer/util/relnode.c | 18 +++--------------- src/include/optimizer/pathnode.h | 3 ++- 3 files changed, 11 insertions(+), 20 deletions(-) diff --git a/src/backend/optimizer/path/joinrels.c b/src/backend/optimizer/path/joinrels.c index f3a9412d18..887135869c 100644 --- a/src/backend/optimizer/path/joinrels.c +++ b/src/backend/optimizer/path/joinrels.c @@ -1544,6 +1544,7 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, RelOptInfo *child_joinrel; AppendRelInfo **appinfos; int nappinfos; + Bitmapset *child_relids = NULL; if (joinrel->partbounds_merged) { @@ -1639,9 +1640,8 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, child_rel2->relids); /* Find the AppendRelInfo structures */ - appinfos = find_appinfos_by_relids(root, - bms_union(child_rel1->relids, - child_rel2->relids), + child_relids = bms_union(child_rel1->relids, child_rel2->relids); + appinfos = find_appinfos_by_relids(root, child_relids, &nappinfos); /* @@ -1659,7 +1659,8 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, { child_joinrel = build_child_join_rel(root, child_rel1, child_rel2, joinrel, child_restrictlist, - child_sjinfo); + child_sjinfo, appinfos, + nappinfos); joinrel->part_rels[cnt_parts] = child_joinrel; joinrel->live_parts = bms_add_member(joinrel->live_parts, cnt_parts); joinrel->all_partrels = bms_add_members(joinrel->all_partrels, @@ -1678,6 +1679,7 @@ try_partitionwise_join(PlannerInfo *root, RelOptInfo *rel1, RelOptInfo *rel2, pfree(appinfos); free_child_join_sjinfo(child_sjinfo); + bms_free(child_relids); } } diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c index e05b21c884..989bee0fa8 100644 --- a/src/backend/optimizer/util/relnode.c +++ b/src/backend/optimizer/util/relnode.c @@ -876,15 +876,15 @@ build_join_rel(PlannerInfo *root, * 'restrictlist': list of RestrictInfo nodes that apply to this particular * pair of joinable relations * 'sjinfo': child join's join-type details + * 'appinfos' and 'nappinfos': AppendRelInfo array for child relids */ RelOptInfo * build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, RelOptInfo *inner_rel, RelOptInfo *parent_joinrel, - List *restrictlist, SpecialJoinInfo *sjinfo) + List *restrictlist, SpecialJoinInfo *sjinfo, + AppendRelInfo **appinfos, int nappinfos) { RelOptInfo *joinrel = makeNode(RelOptInfo); - AppendRelInfo **appinfos; - int nappinfos; /* Only joins between "other" relations land here. */ Assert(IS_OTHER_REL(outer_rel) && IS_OTHER_REL(inner_rel)); @@ -892,16 +892,6 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, /* The parent joinrel should have consider_partitionwise_join set. */ Assert(parent_joinrel->consider_partitionwise_join); - /* - * Find the AppendRelInfo structures for the child baserels. We'll need - * these for computing the child join's relid set, and later for mapping - * Vars to the child rel. - */ - appinfos = find_appinfos_by_relids(root, - bms_union(outer_rel->relids, - inner_rel->relids), - &nappinfos); - joinrel->reloptkind = RELOPT_OTHER_JOINREL; joinrel->relids = adjust_child_relids(parent_joinrel->relids, nappinfos, appinfos); @@ -1017,8 +1007,6 @@ build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, nappinfos, appinfos, parent_joinrel, joinrel); - pfree(appinfos); - return joinrel; } diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index 112e7c23d4..71fda9f2a4 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -346,6 +346,7 @@ extern Bitmapset *get_param_path_clause_serials(Path *path); extern RelOptInfo *build_child_join_rel(PlannerInfo *root, RelOptInfo *outer_rel, RelOptInfo *inner_rel, RelOptInfo *parent_joinrel, List *restrictlist, - SpecialJoinInfo *sjinfo); + SpecialJoinInfo *sjinfo, + AppendRelInfo **appinfos, int nappinfos); #endif /* PATHNODE_H */ -- 2.34.1
