public inbox for [email protected]help / color / mirror / Atom feed
Re: Check lateral references within PHVs for memoize cache keys 9+ messages / 5 participants [nested] [flat]
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-04 07:33 Richard Guo <[email protected]> 0 siblings, 2 replies; 9+ messages in thread From: Richard Guo @ 2023-07-04 07:33 UTC (permalink / raw) To: pgsql-hackers On Fri, Dec 30, 2022 at 11:00 AM Richard Guo <[email protected]> wrote: > On Fri, Dec 9, 2022 at 5:16 PM Richard Guo <[email protected]> wrote: > >> Actually we do have checked PHVs for lateral references, earlier in >> create_lateral_join_info. But that time we only marked lateral_relids >> and direct_lateral_relids, without remembering the lateral expressions. >> So I'm wondering whether we can fix that by fetching Vars (or PHVs) of >> lateral references within PlaceHolderVars and remembering them in the >> baserel's lateral_vars. >> >> Attach a draft patch to show my thoughts. >> > > Update the patch to fix test failures. > Rebase the patch on HEAD as cfbot reminds. Thanks Richard Attachments: [application/octet-stream] v3-0001-Check-lateral-refs-within-PHVs-for-memoize-cache-keys.patch (7.0K, ../../CAMbWs4_E2HcTJ0b2srM3hwnQbH5SogkZMM399WUG4gTQSa1FOQ@mail.gmail.com/3-v3-0001-Check-lateral-refs-within-PHVs-for-memoize-cache-keys.patch) download | inline diff: From 1fa67fc11c3483e5cf7e9c3bbc44924abce05bb7 Mon Sep 17 00:00:00 2001 From: Richard Guo <[email protected]> Date: Fri, 30 Dec 2022 10:35:36 +0800 Subject: [PATCH v3] Check lateral refs within PHVs for memoize cache keys --- .../postgres_fdw/expected/postgres_fdw.out | 12 +++-- src/backend/optimizer/plan/initsplan.c | 51 +++++++++++++++++++ src/test/regress/expected/memoize.out | 31 +++++++++++ src/test/regress/sql/memoize.sql | 11 ++++ 4 files changed, 101 insertions(+), 4 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 852b5b4707..4e654ffa3a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -3687,15 +3687,19 @@ ORDER BY ref_0."C 1"; -> Index Scan using t1_pkey on "S 1"."T 1" ref_0 Output: ref_0."C 1", ref_0.c2, ref_0.c3, ref_0.c4, ref_0.c5, ref_0.c6, ref_0.c7, ref_0.c8 Index Cond: (ref_0."C 1" < 10) - -> Foreign Scan on public.ft1 ref_1 - Output: ref_1.c3, ref_0.c2 - Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) + -> Memoize + Output: ref_1.c3, (ref_0.c2) + Cache Key: ref_0.c2 + Cache Mode: binary + -> Foreign Scan on public.ft1 ref_1 + Output: ref_1.c3, ref_0.c2 + Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) -> Materialize Output: ref_3.c3 -> Foreign Scan on public.ft2 ref_3 Output: ref_3.c3 Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) -(15 rows) +(19 rows) SELECT ref_0.c2, subq_1.* FROM diff --git a/src/backend/optimizer/plan/initsplan.c b/src/backend/optimizer/plan/initsplan.c index b31d892121..80f2902e12 100644 --- a/src/backend/optimizer/plan/initsplan.c +++ b/src/backend/optimizer/plan/initsplan.c @@ -582,6 +582,9 @@ create_lateral_join_info(PlannerInfo *root) Relids eval_at = phinfo->ph_eval_at; Relids lateral_refs; int varno; + List *vars; + List *ph_lateral_vars; + ListCell *cell; if (phinfo->ph_lateral == NULL) continue; /* PHV is uninteresting if no lateral refs */ @@ -597,6 +600,40 @@ create_lateral_join_info(PlannerInfo *root) lateral_refs = bms_intersect(phinfo->ph_lateral, root->all_baserels); Assert(!bms_is_empty(lateral_refs)); + /* Fetch Vars and PHVs of lateral references within PlaceHolderVars */ + vars = pull_vars_of_level((Node *) phinfo->ph_var->phexpr, 0); + + ph_lateral_vars = NIL; + foreach(cell, vars) + { + Node *node = (Node *) lfirst(cell); + + node = copyObject(node); + if (IsA(node, Var)) + { + Var *var = (Var *) node; + + Assert(var->varlevelsup == 0); + + if (bms_is_member(var->varno, phinfo->ph_lateral)) + ph_lateral_vars = lappend(ph_lateral_vars, node); + } + else if (IsA(node, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) node; + + Assert(phv->phlevelsup == 0); + + if (bms_is_subset(find_placeholder_info(root, phv)->ph_eval_at, + phinfo->ph_lateral)) + ph_lateral_vars = lappend(ph_lateral_vars, node); + } + else + Assert(false); + } + + list_free(vars); + if (bms_get_singleton_member(eval_at, &varno)) { /* Evaluation site is a baserel */ @@ -608,6 +645,13 @@ create_lateral_join_info(PlannerInfo *root) brel->lateral_relids = bms_add_members(brel->lateral_relids, lateral_refs); + + /* + * Remember the lateral references. We need this info for searching + * memoize cache keys. + */ + brel->lateral_vars = + list_concat(brel->lateral_vars, ph_lateral_vars); } else { @@ -621,6 +665,13 @@ create_lateral_join_info(PlannerInfo *root) continue; /* ignore outer joins in eval_at */ brel->lateral_relids = bms_add_members(brel->lateral_relids, lateral_refs); + + /* + * Remember the lateral references. We need this info for + * searching memoize cache keys. + */ + brel->lateral_vars = list_concat(brel->lateral_vars, + ph_lateral_vars); } } } diff --git a/src/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out index f5202430f8..1f59777903 100644 --- a/src/test/regress/expected/memoize.out +++ b/src/test/regress/expected/memoize.out @@ -92,6 +92,37 @@ WHERE t1.unique1 < 1000; 1000 | 9.5000000000000000 (1 row) +-- Try with LATERAL references within PlaceHolderVars +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + explain_memoize +------------------------------------------------------------------------------------------- + Aggregate (actual rows=1 loops=N) + -> Nested Loop (actual rows=1000 loops=N) + -> Seq Scan on tenk1 t1 (actual rows=1000 loops=N) + Filter: (unique1 < 1000) + Rows Removed by Filter: 9000 + -> Memoize (actual rows=1 loops=N) + Cache Key: t1.twenty + Cache Mode: binary + Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB + -> Index Only Scan using tenk1_unique1 on tenk1 t2 (actual rows=1 loops=N) + Filter: (t1.twenty = unique1) + Rows Removed by Filter: 9999 + Heap Fetches: N +(13 rows) + +-- And check we get the expected results. +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + count | avg +-------+-------------------- + 1000 | 9.5000000000000000 +(1 row) + -- Reduce work_mem and hash_mem_multiplier so that we see some cache evictions SET work_mem TO '64kB'; SET hash_mem_multiplier TO 1.0; diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql index 29ab1ea62d..86f1aae0c7 100644 --- a/src/test/regress/sql/memoize.sql +++ b/src/test/regress/sql/memoize.sql @@ -57,6 +57,17 @@ LATERAL (SELECT t2.unique1 FROM tenk1 t2 WHERE t1.twenty = t2.unique1 OFFSET 0) t2 WHERE t1.unique1 < 1000; +-- Try with LATERAL references within PlaceHolderVars +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + +-- And check we get the expected results. +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + -- Reduce work_mem and hash_mem_multiplier so that we see some cache evictions SET work_mem TO '64kB'; SET hash_mem_multiplier TO 1.0; -- 2.31.0 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-08 05:24 Paul A Jungwirth <[email protected]> parent: Richard Guo <[email protected]> 1 sibling, 1 reply; 9+ messages in thread From: Paul A Jungwirth @ 2023-07-08 05:24 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers On Tue, Jul 4, 2023 at 12:33 AM Richard Guo <[email protected]> wrote: > > Rebase the patch on HEAD as cfbot reminds. All of this seems good to me. I can reproduce the problem, tests pass, and the change is sensible as far as I can tell. One adjacent thing I noticed is that when we renamed "Result Cache" to "Memoize" this bit of the docs in config.sgml got skipped (probably because of the line break): Hash tables are used in hash joins, hash-based aggregation, result cache nodes and hash-based processing of <literal>IN</literal> subqueries. I believe that should say "memoize nodes" instead. Is it worth correcting that as part of this patch? Or perhaps another one? Regards, -- Paul ~{:-) [email protected] ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-08 17:28 Tom Lane <[email protected]> parent: Richard Guo <[email protected]> 1 sibling, 3 replies; 9+ messages in thread From: Tom Lane @ 2023-07-08 17:28 UTC (permalink / raw) To: Richard Guo <[email protected]>; +Cc: pgsql-hackers Richard Guo <[email protected]> writes: > Rebase the patch on HEAD as cfbot reminds. This fix seems like a mess. The function that is in charge of filling RelOptInfo.lateral_vars is extract_lateral_references; or at least that was how it was done up to now. Can't we compute these additional references there? If not, maybe we ought to just merge extract_lateral_references into create_lateral_join_info, rather than having the responsibility split. I also wonder whether this change isn't creating hidden dependencies on RTE order (which would likely be bugs), since create_lateral_join_info itself examines the lateral_vars lists as it walks the rtable. More generally, it's not clear to me why we should need to look inside lateral PHVs in the first place. Wouldn't the lateral PHV itself serve fine as a cache key? regards, tom lane ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-09 01:11 David Rowley <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 9+ messages in thread From: David Rowley @ 2023-07-09 01:11 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Richard Guo <[email protected]>; pgsql-hackers On Sun, 9 Jul 2023 at 05:28, Tom Lane <[email protected]> wrote: > More generally, it's not clear to me why we should need to look inside > lateral PHVs in the first place. Wouldn't the lateral PHV itself > serve fine as a cache key? For Memoize specifically, I purposefully made it so the expression was used as a cache key rather than extracting the Vars from it and using those. The reason for that was that the expression may result in fewer distinct values to cache tuples for. For example: create table t1 (a int primary key); create table t2 (a int primary key); create statistics on (a % 10) from t2; insert into t2 select x from generate_Series(1,1000000)x; insert into t1 select x from generate_Series(1,1000000)x; analyze t1,t2; explain (analyze, costs off) select * from t1 inner join t2 on t1.a=t2.a%10; QUERY PLAN -------------------------------------------------------------------------------------------- Nested Loop (actual time=0.015..212.798 rows=900000 loops=1) -> Seq Scan on t2 (actual time=0.006..33.479 rows=1000000 loops=1) -> Memoize (actual time=0.000..0.000 rows=1 loops=1000000) Cache Key: (t2.a % 10) Cache Mode: logical Hits: 999990 Misses: 10 Evictions: 0 Overflows: 0 Memory Usage: 1kB -> Index Only Scan using t1_pkey on t1 (actual time=0.001..0.001 rows=1 loops=10) Index Cond: (a = (t2.a % 10)) Heap Fetches: 0 Planning Time: 0.928 ms Execution Time: 229.621 ms (11 rows) ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-09 04:17 David Rowley <[email protected]> parent: Paul A Jungwirth <[email protected]> 0 siblings, 1 reply; 9+ messages in thread From: David Rowley @ 2023-07-09 04:17 UTC (permalink / raw) To: Paul A Jungwirth <[email protected]>; +Cc: Richard Guo <[email protected]>; pgsql-hackers On Sat, 8 Jul 2023 at 17:24, Paul A Jungwirth <[email protected]> wrote: > One adjacent thing I noticed is that when we renamed "Result Cache" to > "Memoize" this bit of the docs in config.sgml got skipped (probably > because of the line break): > > Hash tables are used in hash joins, hash-based aggregation, result > cache nodes and hash-based processing of <literal>IN</literal> > subqueries. > > I believe that should say "memoize nodes" instead. Is it worth > correcting that as part of this patch? Or perhaps another one? I just pushed a fix for this. Thanks for reporting it. David ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-13 07:12 Richard Guo <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 9+ messages in thread From: Richard Guo @ 2023-07-13 07:12 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On Sun, Jul 9, 2023 at 1:28 AM Tom Lane <[email protected]> wrote: > Richard Guo <[email protected]> writes: > > Rebase the patch on HEAD as cfbot reminds. > > This fix seems like a mess. The function that is in charge of filling > RelOptInfo.lateral_vars is extract_lateral_references; or at least > that was how it was done up to now. Can't we compute these additional > references there? If not, maybe we ought to just merge > extract_lateral_references into create_lateral_join_info, rather than > having the responsibility split. I also wonder whether this change > isn't creating hidden dependencies on RTE order (which would likely be > bugs), since create_lateral_join_info itself examines the lateral_vars > lists as it walks the rtable. Yeah, you're right. Currently all RelOptInfo.lateral_vars are filled in extract_lateral_references. And then in create_lateral_join_info these lateral_vars, together with all PlaceHolderInfos, are examined to compute the per-base-relation lateral refs relids. However, in the process of extract_lateral_references, it's possible that we create new PlaceHolderInfos. So I think it may not be a good idea to extract the lateral references within PHVs there. But I agree with you that it's also not a good idea to compute these additional lateral Vars within PHVs in create_lateral_join_info as the patch does. Actually with the patch I find that with PHVs that are due to be evaluated at a join we may get a problematic plan. For instance explain (costs off) select * from t t1 left join lateral (select t1.a as t1a, t2.a as t2a from t t2 join t t3 on true) s on true where s.t1a = s.t2a; QUERY PLAN ------------------------------------ Nested Loop -> Seq Scan on t t1 -> Nested Loop Join Filter: (t1.a = t2.a) -> Seq Scan on t t2 -> Memoize Cache Key: t1.a Cache Mode: binary -> Seq Scan on t t3 (9 rows) There are no lateral refs in the subtree of the Memoize node, so it should be a Materialize node rather than a Memoize node. This is caused by that for a PHV that is due to be evaluated at a join, we fill its lateral refs in each baserel in the join, which is wrong. So I'm wondering if it'd be better that we move all this logic of computing additional lateral references within PHVs to get_memoize_path, where we can examine only PHVs that are evaluated at innerrel. And considering that these lateral refs are only used by Memoize, it seems more sensible to compute them there. But I'm a little worried that doing this would make get_memoize_path too expensive. Please see v4 patch for this change. Thanks Richard Attachments: [application/octet-stream] v4-0001-Check-lateral-refs-within-PHVs-for-memoize-cache-keys.patch (11.3K, ../../CAMbWs49+Cjoy0S0xkCRDcHXGHvsYLOdvr9jq9OTONOBnsgzXOg@mail.gmail.com/3-v4-0001-Check-lateral-refs-within-PHVs-for-memoize-cache-keys.patch) download | inline diff: From 39a3817151e3dec1890923b7e5ae2b6b0b612e12 Mon Sep 17 00:00:00 2001 From: Richard Guo <[email protected]> Date: Fri, 30 Dec 2022 10:35:36 +0800 Subject: [PATCH v4] Check lateral refs within PHVs for memoize cache keys --- .../postgres_fdw/expected/postgres_fdw.out | 12 ++- src/backend/optimizer/path/joinpath.c | 85 ++++++++++++++++++- src/test/regress/expected/memoize.out | 68 +++++++++++++++ src/test/regress/sql/memoize.sql | 24 ++++++ 4 files changed, 181 insertions(+), 8 deletions(-) diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out index 852b5b4707..4e654ffa3a 100644 --- a/contrib/postgres_fdw/expected/postgres_fdw.out +++ b/contrib/postgres_fdw/expected/postgres_fdw.out @@ -3687,15 +3687,19 @@ ORDER BY ref_0."C 1"; -> Index Scan using t1_pkey on "S 1"."T 1" ref_0 Output: ref_0."C 1", ref_0.c2, ref_0.c3, ref_0.c4, ref_0.c5, ref_0.c6, ref_0.c7, ref_0.c8 Index Cond: (ref_0."C 1" < 10) - -> Foreign Scan on public.ft1 ref_1 - Output: ref_1.c3, ref_0.c2 - Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) + -> Memoize + Output: ref_1.c3, (ref_0.c2) + Cache Key: ref_0.c2 + Cache Mode: binary + -> Foreign Scan on public.ft1 ref_1 + Output: ref_1.c3, ref_0.c2 + Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) -> Materialize Output: ref_3.c3 -> Foreign Scan on public.ft2 ref_3 Output: ref_3.c3 Remote SQL: SELECT c3 FROM "S 1"."T 1" WHERE ((c3 = '00001')) -(15 rows) +(19 rows) SELECT ref_0.c2, subq_1.* FROM diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index f047ad9ba4..7a0b155065 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -23,6 +23,7 @@ #include "optimizer/optimizer.h" #include "optimizer/pathnode.h" #include "optimizer/paths.h" +#include "optimizer/placeholder.h" #include "optimizer/planmain.h" #include "utils/typcache.h" @@ -434,10 +435,11 @@ have_unsafe_outer_join_ref(PlannerInfo *root, static bool paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, RelOptInfo *outerrel, RelOptInfo *innerrel, - List **param_exprs, List **operators, - bool *binary_mode) + List *ph_lateral_vars, List **param_exprs, + List **operators, bool *binary_mode) { + List *lateral_vars; ListCell *lc; *param_exprs = NIL; @@ -509,7 +511,8 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, } /* Now add any lateral vars to the cache key too */ - foreach(lc, innerrel->lateral_vars) + lateral_vars = list_concat(ph_lateral_vars, innerrel->lateral_vars); + foreach(lc, lateral_vars) { Node *expr = (Node *) lfirst(lc); TypeCacheEntry *typentry; @@ -552,6 +555,71 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, return true; } +/* + * extract_lateral_vars_from_PHVs + * Extract lateral references within PlaceHolderVars that are due to be + * evaluated at 'innerrelids'. + */ +static List * +extract_lateral_vars_from_PHVs(PlannerInfo *root, Relids innerrelids) +{ + List *ph_lateral_vars = NIL; + ListCell *lc; + + /* Nothing would be found if the query contains no LATERAL RTEs */ + if (!root->hasLateralRTEs) + return NIL; + + foreach(lc, root->placeholder_list) + { + PlaceHolderInfo *phinfo = (PlaceHolderInfo *) lfirst(lc); + List *vars; + ListCell *cell; + + /* PHV is uninteresting if no lateral refs */ + if (phinfo->ph_lateral == NULL) + continue; + + /* PHV is uninteresting if not due to be evaluated at innerrelids */ + if (!bms_equal(phinfo->ph_eval_at, innerrelids)) + continue; + + /* Fetch Vars and PHVs of lateral references within PlaceHolderVars */ + vars = pull_vars_of_level((Node *) phinfo->ph_var->phexpr, 0); + foreach(cell, vars) + { + Node *node = (Node *) lfirst(cell); + + node = copyObject(node); + if (IsA(node, Var)) + { + Var *var = (Var *) node; + + Assert(var->varlevelsup == 0); + + if (bms_is_member(var->varno, phinfo->ph_lateral)) + ph_lateral_vars = lappend(ph_lateral_vars, node); + } + else if (IsA(node, PlaceHolderVar)) + { + PlaceHolderVar *phv = (PlaceHolderVar *) node; + + Assert(phv->phlevelsup == 0); + + if (bms_is_subset(find_placeholder_info(root, phv)->ph_eval_at, + phinfo->ph_lateral)) + ph_lateral_vars = lappend(ph_lateral_vars, node); + } + else + Assert(false); + } + + list_free(vars); + } + + return ph_lateral_vars; +} + /* * get_memoize_path * If possible, make and return a Memoize path atop of 'inner_path'. @@ -567,6 +635,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, List *hash_operators; ListCell *lc; bool binary_mode; + List *ph_lateral_vars; /* Obviously not if it's disabled */ if (!enable_memoize) @@ -581,6 +650,12 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, if (outer_path->parent->rows < 2) return NULL; + /* + * Extract lateral Vars within PlaceHolderVars that are due to be evaluated + * at innerrel. These lateral Vars could be used as memoize cache keys. + */ + ph_lateral_vars = extract_lateral_vars_from_PHVs(root, innerrel->relids); + /* * We can only have a memoize node when there's some kind of cache key, * either parameterized path clauses or lateral Vars. No cache key sounds @@ -588,7 +663,8 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, */ if ((inner_path->param_info == NULL || inner_path->param_info->ppi_clauses == NIL) && - innerrel->lateral_vars == NIL) + innerrel->lateral_vars == NIL && + ph_lateral_vars == NIL) return NULL; /* @@ -658,6 +734,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, outerrel->top_parent ? outerrel->top_parent : outerrel, innerrel, + ph_lateral_vars, ¶m_exprs, &hash_operators, &binary_mode)) diff --git a/src/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out index f5202430f8..37dbe073fe 100644 --- a/src/test/regress/expected/memoize.out +++ b/src/test/regress/expected/memoize.out @@ -92,6 +92,74 @@ WHERE t1.unique1 < 1000; 1000 | 9.5000000000000000 (1 row) +-- Try with LATERAL references within PlaceHolderVars at a baserel +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + explain_memoize +------------------------------------------------------------------------------------------- + Aggregate (actual rows=1 loops=N) + -> Nested Loop (actual rows=1000 loops=N) + -> Seq Scan on tenk1 t1 (actual rows=1000 loops=N) + Filter: (unique1 < 1000) + Rows Removed by Filter: 9000 + -> Memoize (actual rows=1 loops=N) + Cache Key: t1.twenty + Cache Mode: binary + Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB + -> Index Only Scan using tenk1_unique1 on tenk1 t2 (actual rows=1 loops=N) + Filter: (t1.twenty = unique1) + Rows Removed by Filter: 9999 + Heap Fetches: N +(13 rows) + +-- And check we get the expected results. +SELECT COUNT(*), AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + count | avg +-------+-------------------- + 1000 | 9.5000000000000000 +(1 row) + +-- Try with LATERAL references within PlaceHolderVars at a joinrel +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2, tenk1 t3 + WHERE t3.unique1 = 1) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + explain_memoize +----------------------------------------------------------------------------------------------------- + Aggregate (actual rows=1 loops=N) + -> Nested Loop (actual rows=1000 loops=N) + -> Seq Scan on tenk1 t1 (actual rows=1000 loops=N) + Filter: (unique1 < 1000) + Rows Removed by Filter: 9000 + -> Memoize (actual rows=1 loops=N) + Cache Key: t1.twenty + Cache Mode: binary + Hits: 980 Misses: 20 Evictions: Zero Overflows: 0 Memory Usage: NkB + -> Nested Loop (actual rows=1 loops=N) + Join Filter: (t1.twenty = t2.unique1) + Rows Removed by Join Filter: 9999 + -> Index Only Scan using tenk1_unique1 on tenk1 t3 (actual rows=1 loops=N) + Index Cond: (unique1 = 1) + Heap Fetches: N + -> Index Only Scan using tenk1_unique1 on tenk1 t2 (actual rows=10000 loops=N) + Heap Fetches: N +(17 rows) + +-- And check we get the expected results. +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2, tenk1 t3 + WHERE t3.unique1 = 1) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + count | avg +-------+-------------------- + 1000 | 9.5000000000000000 +(1 row) + -- Reduce work_mem and hash_mem_multiplier so that we see some cache evictions SET work_mem TO '64kB'; SET hash_mem_multiplier TO 1.0; diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql index 29ab1ea62d..2ef30de986 100644 --- a/src/test/regress/sql/memoize.sql +++ b/src/test/regress/sql/memoize.sql @@ -57,6 +57,30 @@ LATERAL (SELECT t2.unique1 FROM tenk1 t2 WHERE t1.twenty = t2.unique1 OFFSET 0) t2 WHERE t1.unique1 < 1000; +-- Try with LATERAL references within PlaceHolderVars at a baserel +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + +-- And check we get the expected results. +SELECT COUNT(*), AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + +-- Try with LATERAL references within PlaceHolderVars at a joinrel +SELECT explain_memoize(' +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2, tenk1 t3 + WHERE t3.unique1 = 1) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000;', false); + +-- And check we get the expected results. +SELECT COUNT(*),AVG(t1.twenty) FROM tenk1 t1 LEFT JOIN +LATERAL (SELECT t1.twenty as c1, t2.unique1 as c2 FROM tenk1 t2, tenk1 t3 + WHERE t3.unique1 = 1) s on true +WHERE s.c1 = s.c2 and t1.unique1 < 1000; + -- Reduce work_mem and hash_mem_multiplier so that we see some cache evictions SET work_mem TO '64kB'; SET hash_mem_multiplier TO 1.0; -- 2.31.0 ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-13 09:21 Richard Guo <[email protected]> parent: Tom Lane <[email protected]> 2 siblings, 0 replies; 9+ messages in thread From: Richard Guo @ 2023-07-13 09:21 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: pgsql-hackers On Sun, Jul 9, 2023 at 1:28 AM Tom Lane <[email protected]> wrote: > More generally, it's not clear to me why we should need to look inside > lateral PHVs in the first place. Wouldn't the lateral PHV itself > serve fine as a cache key? Do you mean we use the lateral PHV directly as a cache key? Hmm, it seems to me that we'd have problem if the PHV references rels that are inside the PHV's syntactic scope. For instance select * from t t1 left join lateral (select t1.a+t2.a as t1a, t2.a as t2a from t t2) s on true where s.t1a = s.t2a; The PHV references t1.a so it's lateral. But it also references t2.a, so if we use the PHV itself as cache key, the plan would look like QUERY PLAN ---------------------------------------- Nested Loop -> Seq Scan on t t1 -> Memoize Cache Key: (t1.a + t2.a) Cache Mode: binary -> Seq Scan on t t2 Filter: ((t1.a + a) = a) (7 rows) which is an invalid plan as the cache key contains t2.a. Thanks Richard ^ permalink raw reply [nested|flat] 9+ messages in thread
* Re: Check lateral references within PHVs for memoize cache keys @ 2023-07-13 09:29 Richard Guo <[email protected]> parent: David Rowley <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Richard Guo @ 2023-07-13 09:29 UTC (permalink / raw) To: David Rowley <[email protected]>; +Cc: Paul A Jungwirth <[email protected]>; pgsql-hackers On Sun, Jul 9, 2023 at 12:17 PM David Rowley <[email protected]> wrote: > I just pushed a fix for this. Thanks for reporting it. BTW, I noticed a typo in the comment inside paraminfo_get_equal_hashops. foreach(lc, innerrel->lateral_vars) { Node *expr = (Node *) lfirst(lc); TypeCacheEntry *typentry; /* Reject if there are any volatile functions in PHVs */ if (contain_volatile_functions(expr)) { list_free(*operators); list_free(*param_exprs); return false; } The expressions in RelOptInfo.lateral_vars are not necessarily from PHVs. For instance explain (costs off) select * from t t1 join lateral (select * from t t2 where t1.a = t2.a offset 0) on true; QUERY PLAN ---------------------------------- Nested Loop -> Seq Scan on t t1 -> Memoize Cache Key: t1.a Cache Mode: binary -> Seq Scan on t t2 Filter: (t1.a = a) (7 rows) The lateral Var 't1.a' comes from the lateral subquery, not PHV. This seems a typo from 63e4f13d. How about we change it to the below? - /* Reject if there are any volatile functions in PHVs */ + /* Reject if there are any volatile functions in lateral vars */ Thanks Richard ^ permalink raw reply [nested|flat] 9+ messages in thread
* [PATCH v47 1/9] Make index_concurrently_create_copy more general @ 2026-03-24 18:02 Álvaro Herrera <[email protected]> 0 siblings, 0 replies; 9+ messages in thread From: Álvaro Herrera @ 2026-03-24 18:02 UTC (permalink / raw) Add a 'boolean concurrent' option, and make it work for both cases. Also rename it to index_create_copy. This allows it to be reused for other purposes -- specifically, for REPACK CONCURRENTLY. With the CONCURRENTLY option, REPACK cannot simply swap the heap file and rebuild its indexes. Instead, it needs to build a separate set of indexes (including system catalog entries) *before* the actual swap, to reduce the time AccessExclusiveLock needs to be held for. This approach is different from what CREATE INDEX CONCURRENTLY does. Per a suggestion from Mihail Nikalayeu. Author: Antonin Houska <[email protected]> Discussion: https://postgr.es/m/41104.1754922120@localhost --- src/backend/catalog/index.c | 39 +++++++++++++++++++------------- src/backend/commands/indexcmds.c | 15 +++++++----- src/backend/nodes/makefuncs.c | 9 ++++---- src/include/catalog/index.h | 7 +++--- src/include/nodes/makefuncs.h | 4 +++- 5 files changed, 43 insertions(+), 31 deletions(-) diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c index 1ccfa687f05..b86ad73c626 100644 --- a/src/backend/catalog/index.c +++ b/src/backend/catalog/index.c @@ -1289,17 +1289,17 @@ index_create(Relation heapRelation, } /* - * index_concurrently_create_copy + * index_create_copy * - * Create concurrently an index based on the definition of the one provided by - * caller. The index is inserted into catalogs and needs to be built later - * on. This is called during concurrent reindex processing. + * Create an index based on the definition of the one provided by caller. The + * index is inserted into catalogs. If 'concurrently' is TRUE, it needs to be + * built later on; otherwise it's built immediately. * * "tablespaceOid" is the tablespace to use for this index. */ Oid -index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, - Oid tablespaceOid, const char *newName) +index_create_copy(Relation heapRelation, bool concurrently, + Oid oldIndexId, Oid tablespaceOid, const char *newName) { Relation indexRelation; IndexInfo *oldInfo, @@ -1318,6 +1318,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, List *indexColNames = NIL; List *indexExprs = NIL; List *indexPreds = NIL; + int flags = 0; indexRelation = index_open(oldIndexId, RowExclusiveLock); @@ -1328,7 +1329,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, * Concurrent build of an index with exclusion constraints is not * supported. */ - if (oldInfo->ii_ExclusionOps != NULL) + if (oldInfo->ii_ExclusionOps != NULL && concurrently) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("concurrent index creation for exclusion constraints is not supported"))); @@ -1384,9 +1385,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, } /* - * Build the index information for the new index. Note that rebuild of - * indexes with exclusion constraints is not supported, hence there is no - * need to fill all the ii_Exclusion* fields. + * Build the index information for the new index. */ newInfo = makeIndexInfo(oldInfo->ii_NumIndexAttrs, oldInfo->ii_NumIndexKeyAttrs, @@ -1395,10 +1394,13 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, indexPreds, oldInfo->ii_Unique, oldInfo->ii_NullsNotDistinct, - false, /* not ready for inserts */ - true, + !concurrently, /* isready */ + concurrently, /* concurrent */ indexRelation->rd_indam->amsummarizing, - oldInfo->ii_WithoutOverlaps); + oldInfo->ii_WithoutOverlaps, + oldInfo->ii_ExclusionOps, + oldInfo->ii_ExclusionProcs, + oldInfo->ii_ExclusionStrats); /* * Extract the list of column names and the column numbers for the new @@ -1436,6 +1438,9 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, stattargets[i].isnull = isnull; } + if (concurrently) + flags = INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT; + /* * Now create the new index. * @@ -1459,7 +1464,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId, indcoloptions->values, stattargets, reloptionsDatum, - INDEX_CREATE_SKIP_BUILD | INDEX_CREATE_CONCURRENT, + flags, 0, true, /* allow table to be a system catalog? */ false, /* is_internal? */ @@ -2453,7 +2458,8 @@ BuildIndexInfo(Relation index) indexStruct->indisready, false, index->rd_indam->amsummarizing, - indexStruct->indisexclusion && indexStruct->indisunique); + indexStruct->indisexclusion && indexStruct->indisunique, + NULL, NULL, NULL); /* fill in attribute numbers */ for (i = 0; i < numAtts; i++) @@ -2513,7 +2519,8 @@ BuildDummyIndexInfo(Relation index) indexStruct->indisready, false, index->rd_indam->amsummarizing, - indexStruct->indisexclusion && indexStruct->indisunique); + indexStruct->indisexclusion && indexStruct->indisunique, + NULL, NULL, NULL); /* fill in attribute numbers */ for (i = 0; i < numAtts; i++) diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c index 373e8234794..4a2d21915b1 100644 --- a/src/backend/commands/indexcmds.c +++ b/src/backend/commands/indexcmds.c @@ -244,7 +244,8 @@ CheckIndexCompatible(Oid oldId, */ indexInfo = makeIndexInfo(numberOfAttributes, numberOfAttributes, accessMethodId, NIL, NIL, false, false, - false, false, amsummarizing, isWithoutOverlaps); + false, false, amsummarizing, isWithoutOverlaps, + NULL, NULL, NULL); typeIds = palloc_array(Oid, numberOfAttributes); collationIds = palloc_array(Oid, numberOfAttributes); opclassIds = palloc_array(Oid, numberOfAttributes); @@ -931,7 +932,8 @@ DefineIndex(ParseState *pstate, !concurrent, concurrent, amissummarizing, - stmt->iswithoutoverlaps); + stmt->iswithoutoverlaps, + NULL, NULL, NULL); typeIds = palloc_array(Oid, numberOfAttributes); collationIds = palloc_array(Oid, numberOfAttributes); @@ -3989,10 +3991,11 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein tablespaceid = indexRel->rd_rel->reltablespace; /* Create new index definition based on given index */ - newIndexId = index_concurrently_create_copy(heapRel, - idx->indexId, - tablespaceid, - concurrentName); + newIndexId = index_create_copy(heapRel, + true, + idx->indexId, + tablespaceid, + concurrentName); /* * Now open the relation of the new index, a session-level lock is diff --git a/src/backend/nodes/makefuncs.c b/src/backend/nodes/makefuncs.c index 3cd35c5c457..8d23aa917e5 100644 --- a/src/backend/nodes/makefuncs.c +++ b/src/backend/nodes/makefuncs.c @@ -834,7 +834,8 @@ IndexInfo * makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, List *predicates, bool unique, bool nulls_not_distinct, bool isready, bool concurrent, bool summarizing, - bool withoutoverlaps) + bool withoutoverlaps, Oid *exclusion_ops, Oid *exclusion_procs, + uint16 *exclusion_strats) { IndexInfo *n = makeNode(IndexInfo); @@ -863,9 +864,9 @@ makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, n->ii_PredicateState = NULL; /* exclusion constraints */ - n->ii_ExclusionOps = NULL; - n->ii_ExclusionProcs = NULL; - n->ii_ExclusionStrats = NULL; + n->ii_ExclusionOps = exclusion_ops; + n->ii_ExclusionProcs = exclusion_procs; + n->ii_ExclusionStrats = exclusion_strats; /* speculative inserts */ n->ii_UniqueOps = NULL; diff --git a/src/include/catalog/index.h b/src/include/catalog/index.h index a38e95bc0eb..ed9e4c37d27 100644 --- a/src/include/catalog/index.h +++ b/src/include/catalog/index.h @@ -101,10 +101,9 @@ extern Oid index_create(Relation heapRelation, #define INDEX_CONSTR_CREATE_REMOVE_OLD_DEPS (1 << 4) #define INDEX_CONSTR_CREATE_WITHOUT_OVERLAPS (1 << 5) -extern Oid index_concurrently_create_copy(Relation heapRelation, - Oid oldIndexId, - Oid tablespaceOid, - const char *newName); +extern Oid index_create_copy(Relation heapRelation, bool concurrently, + Oid oldIndexId, Oid tablespaceOid, + const char *newName); extern void index_concurrently_build(Oid heapRelationId, Oid indexRelationId); diff --git a/src/include/nodes/makefuncs.h b/src/include/nodes/makefuncs.h index bf54d39feb0..40ec249a7a1 100644 --- a/src/include/nodes/makefuncs.h +++ b/src/include/nodes/makefuncs.h @@ -99,7 +99,9 @@ extern IndexInfo *makeIndexInfo(int numattrs, int numkeyattrs, Oid amoid, List *expressions, List *predicates, bool unique, bool nulls_not_distinct, bool isready, bool concurrent, - bool summarizing, bool withoutoverlaps); + bool summarizing, bool withoutoverlaps, + Oid *exclusion_ops, Oid *exclusion_procs, + uint16 *exclusion_strats); extern Node *makeStringConst(char *str, int location); extern DefElem *makeDefElem(char *name, Node *arg, int location); -- 2.47.3 --2fxmamo6mu2qbxgv Content-Type: text/x-diff; charset=utf-8 Content-Disposition: attachment; filename="v47-0002-give-options-bitmask-to-table_delete-table_updat.patch" ^ permalink raw reply [nested|flat] 9+ messages in thread
end of thread, other threads:[~2026-03-24 18:02 UTC | newest] Thread overview: 9+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2023-07-04 07:33 Re: Check lateral references within PHVs for memoize cache keys Richard Guo <[email protected]> 2023-07-08 05:24 ` Paul A Jungwirth <[email protected]> 2023-07-09 04:17 ` David Rowley <[email protected]> 2023-07-13 09:29 ` Richard Guo <[email protected]> 2023-07-08 17:28 ` Tom Lane <[email protected]> 2023-07-09 01:11 ` David Rowley <[email protected]> 2023-07-13 07:12 ` Richard Guo <[email protected]> 2023-07-13 09:21 ` Richard Guo <[email protected]> 2026-03-24 18:02 [PATCH v47 1/9] Make index_concurrently_create_copy more general Álvaro Herrera <[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