From a2b91e0c4117415ee743eafdf565fa365f49ebce Mon Sep 17 00:00:00 2001 From: YiLin Zhang Date: Fri, 11 Sep 2026 14:30:41 +0800 Subject: [PATCH v2] Fix wrong results from Memoize with a Param inside a cache key ExecReScanMemoize() purges the whole cache only when a Param changes which is not part of the cache key; a change to a cache-key Param is assumed to be handled by looking up a different cache entry. That assumption holds only when the Param's entire effect on the subplan's output is captured by the cache key. It fails when a Param is buried inside a larger cache-key expression and also appears elsewhere in the subplan. With a cache key of "t2.hundred + t0.ten" and a filter "t1.twenty = t0.ten", different (t2.hundred, t0.ten) pairs produce the same cache-key value while the filter selects different rows, so one cache entry wrongly serves both. Since t0.ten was included in keyparamids, the cache was never flushed when it changed. Fix by making every Param appearing in a cache key expression a cache key in its own right, so that matching the keys implies matching the Param. While here, do the same for the Params which the inner scan uses outside of the cache keys: those in the memoized relation's base quals and targetlist, and on the inner side of the join clauses. Such a Param was handled correctly already, but only by flushing the whole cache whenever it changed, so for a correlated subquery's Param nothing survived from one invocation of the subplan to the next. This part is only an optimization. Making every buried Param a cache key requires each of them to be hashable. A Param of a type with no hash opclass, e.g. point or box, previously caused the Memoize path to be abandoned entirely. That plan regression hit queries which were never broken: a Param used only within the key expression needs no extra handling, and even when it is used elsewhere, the cache can simply be flushed when it changes. Instead of giving up on Memoize, record such Params in the new MemoizePath.unhashable_params field and exclude them from keyparamids in create_memoize_plan(). The executor then purges the cache whenever their values change, which is less efficient than including them as keys, but correct and keeps the Memoize node. For Params of hashable types nothing changes. Measurements with point, box and domain-over-point Params in a correlated subquery show the previous approach to be 5-12x slower than master when the Param changes infrequently, and ~8x slower when it changes on every outer row. This patch restores master-level performance while keeping the results correct in collision scenarios where master returns wrong results. Add regression tests for the reported query and for point Params buried inside cache key expressions, with and without additional uses of the Param, including the collision case that produces wrong results on master. --- src/backend/optimizer/path/joinpath.c | 172 +++++++++++++++--- src/backend/optimizer/plan/createplan.c | 10 ++ src/backend/optimizer/util/clauses.c | 6 +- src/backend/optimizer/util/pathnode.c | 7 +- src/include/nodes/pathnodes.h | 5 + src/include/optimizer/pathnode.h | 3 +- src/test/regress/expected/memoize.out | 227 +++++++++++++++++++++++- src/test/regress/sql/memoize.sql | 130 +++++++++++++- 8 files changed, 520 insertions(+), 40 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index dfd08e7aeb1..2b61df96a15 100644 --- a/src/backend/optimizer/path/joinpath.c +++ b/src/backend/optimizer/path/joinpath.c @@ -94,6 +94,9 @@ static void generate_mergejoin_paths(PlannerInfo *root, Path *inner_cheapest_total, List *merge_pathkeys, bool is_partial); +static bool pull_exec_params_walker(Node *node, List **params); +static bool memoize_add_cache_key(Node *expr, List **param_exprs, + List **operators, bool *binary_mode); /* @@ -479,10 +482,12 @@ static bool paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, RelOptInfo *outerrel, RelOptInfo *innerrel, List *ph_lateral_vars, List **param_exprs, - List **operators, bool *binary_mode) + List **operators, bool *binary_mode, + Bitmapset **unhashable_params) { List *lateral_vars; + List *params; ListCell *lc; *param_exprs = NIL; @@ -567,7 +572,6 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, foreach(lc, lateral_vars) { Node *expr = (Node *) lfirst(lc); - TypeCacheEntry *typentry; /* Reject if there are any volatile functions in lateral vars */ if (contain_volatile_functions(expr)) @@ -577,44 +581,155 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, return false; } - typentry = lookup_type_cache(exprType(expr), - TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR); - - /* can't use memoize without a valid hash proc and equals operator */ - if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr)) + if (!memoize_add_cache_key(expr, param_exprs, operators, binary_mode)) { list_free(*operators); list_free(*param_exprs); return false; } + } - /* - * 'expr' may already exist as a parameter from the ppi_clauses. No - * need to include it again, however we'd better ensure we do switch - * into binary mode. - */ - if (!list_member(*param_exprs, expr)) + /* + * Now make a cache key of every Param which appears inside one of the + * cache key expressions. nodeMemoize.c only flushes the cache for Params + * which aren't part of the cache key, assuming the key determines the + * Param's value. That doesn't hold for a Param buried inside a larger + * expression, as two Param values can produce the same key value while + * affecting the results in some other way, say through a base qual. + */ + params = NIL; + (void) pull_exec_params_walker((Node *) *param_exprs, ¶ms); + + foreach(lc, params) + { + if (!memoize_add_cache_key((Node *) lfirst(lc), param_exprs, operators, + binary_mode)) { - *operators = lappend_oid(*operators, typentry->eq_opr); - *param_exprs = lappend(*param_exprs, expr); + Param *param = (Param *) lfirst(lc); + + /* + * We can't make this Param a cache key (e.g. its type has no + * hash opclass). Rather than abandoning Memoize altogether, + * record the Param ID so that create_memoize_plan can exclude + * it from keyparamids. The executor will then purge the cache + * whenever its value changes, which is less efficient than + * including it as a key but still correct. + */ + *unhashable_params = bms_add_member(*unhashable_params, + param->paramid); } + } - /* - * We must go into binary mode as we don't have too much of an idea of - * how these lateral Vars are being used. See comment above when we - * set *binary_mode for the non-lateral Var case. This could be - * relaxed a bit if we had the RestrictInfos and knew the operators - * being used, however for cases like Vars that are arguments to - * functions we must operate in binary mode as we don't have - * visibility into what the function is doing with the Vars. - */ - *binary_mode = true; + /* + * Params which affect the inner scan without being part of a cache key + * are handled correctly already, as the cache just gets flushed whenever + * one of them changes. Since such Params commonly change on every inner + * scan, we're better off making cache keys of them too so that the cached + * results survive. Unlike above this is only an optimization, so we + * needn't track down every last one of them, nor refuse to memoize when + * one isn't hashable. + */ + params = NIL; + foreach(lc, innerrel->baserestrictinfo) + { + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); + + (void) pull_exec_params_walker((Node *) rinfo->clause, ¶ms); + } + (void) pull_exec_params_walker((Node *) innerrel->reltarget->exprs, ¶ms); + + /* + * Only the outer side of each ppi_clause became a cache key above, so the + * inner sides must be examined here. + */ + if (param_info != NULL) + { + foreach(lc, param_info->ppi_clauses) + { + RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc); + OpExpr *opexpr = (OpExpr *) rinfo->clause; + + (void) pull_exec_params_walker(rinfo->outer_is_left ? + (Node *) lsecond(opexpr->args) : + (Node *) linitial(opexpr->args), + ¶ms); + } } + foreach(lc, params) + (void) memoize_add_cache_key((Node *) lfirst(lc), param_exprs, + operators, binary_mode); + /* We're okay to use memoize */ return true; } +/* + * memoize_add_cache_key + * Add 'expr' to the Memoize cache keys collected in *param_exprs and + * *operators, unless it's a cache key already. + * + * Returns false if 'expr' has a type which Memoize can't hash. It's up to the + * caller to decide whether that's fatal. + */ +static bool +memoize_add_cache_key(Node *expr, List **param_exprs, List **operators, + bool *binary_mode) +{ + /* + * 'expr' may already have been added as a cache key, in which case it has + * been through the checks below already. + */ + if (!list_member(*param_exprs, expr)) + { + TypeCacheEntry *typentry; + + typentry = lookup_type_cache(exprType(expr), + TYPECACHE_HASH_PROC | TYPECACHE_EQ_OPR); + + /* can't use memoize without a valid hash proc and equals operator */ + if (!OidIsValid(typentry->hash_proc) || !OidIsValid(typentry->eq_opr)) + return false; + + *operators = lappend_oid(*operators, typentry->eq_opr); + *param_exprs = lappend(*param_exprs, expr); + } + + /* + * We must go into binary mode as we don't have too much of an idea of how + * 'expr' is being used. See the comment above where we set *binary_mode + * for the join clause case. This could be relaxed a bit if we knew the + * operators being used, however for cases like Vars that are arguments to + * functions, or Params appearing in quals we've not examined, we must + * operate in binary mode as we've no visibility into what's done with the + * value. + */ + *binary_mode = true; + + return true; +} + +/* + * pull_exec_params_walker + * Collect the distinct PARAM_EXEC Params found in 'node' into *params. + */ +static bool +pull_exec_params_walker(Node *node, List **params) +{ + if (node == NULL) + return false; + if (IsA(node, Param)) + { + Param *param = (Param *) node; + + if (param->paramkind == PARAM_EXEC && + !list_member(*params, param)) + *params = lappend(*params, param); + return false; + } + return expression_tree_walker(node, pull_exec_params_walker, params); +} + /* * extract_lateral_vars_from_PHVs * Extract lateral references within PlaceHolderVars that are due to be @@ -722,6 +837,7 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, ListCell *lc; bool binary_mode; List *ph_lateral_vars; + Bitmapset *unhashable_params = NULL; /* Obviously not if it's disabled */ if ((extra->pgs_mask & PGS_NESTLOOP_MEMOIZE) == 0) @@ -857,7 +973,8 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, ph_lateral_vars, ¶m_exprs, &hash_operators, - &binary_mode)) + &binary_mode, + &unhashable_params)) { return (Path *) create_memoize_path(root, innerrel, @@ -866,7 +983,8 @@ get_memoize_path(PlannerInfo *root, RelOptInfo *innerrel, hash_operators, extra->inner_unique, binary_mode, - outer_path->rows); + outer_path->rows, + unhashable_params); } return NULL; diff --git a/src/backend/optimizer/plan/createplan.c b/src/backend/optimizer/plan/createplan.c index 430f6557f1f..8874f9bbc0b 100644 --- a/src/backend/optimizer/plan/createplan.c +++ b/src/backend/optimizer/plan/createplan.c @@ -1752,6 +1752,16 @@ create_memoize_plan(PlannerInfo *root, MemoizePath *best_path, int flags) keyparamids = pull_paramids((Expr *) param_exprs); + /* + * Params that appear in the key expressions but could not be made cache + * keys must not be included in keyparamids. The executor purges the + * cache when such a Param changes, so that stale entries are never + * reused. + */ + if (best_path->unhashable_params) + keyparamids = bms_del_members(keyparamids, + best_path->unhashable_params); + plan = make_memoize(subplan, operators, collations, param_exprs, best_path->singlerow, best_path->binary_mode, best_path->est_entries, keyparamids, best_path->est_calls, diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index 55cebe4a74b..702c13ddac3 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -6368,7 +6368,8 @@ substitute_actual_parameters_in_from_mutator(Node *node, /* * pull_paramids - * Returns a Bitmapset containing the paramids of all Params in 'expr'. + * Returns a Bitmapset containing the paramids of all PARAM_EXEC Params + * in 'expr'. */ Bitmapset * pull_paramids(Expr *expr) @@ -6389,7 +6390,8 @@ pull_paramids_walker(Node *node, Bitmapset **context) { Param *param = (Param *) node; - *context = bms_add_member(*context, param->paramid); + if (param->paramkind == PARAM_EXEC) + *context = bms_add_member(*context, param->paramid); return false; } return expression_tree_walker(node, pull_paramids_walker, context); diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 2ba31765ca5..8af0036ac77 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -1747,7 +1747,8 @@ create_material_path(RelOptInfo *rel, Path *subpath, bool enabled) MemoizePath * create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, List *param_exprs, List *hash_operators, - bool singlerow, bool binary_mode, Cardinality est_calls) + bool singlerow, bool binary_mode, Cardinality est_calls, + Bitmapset *unhashable_params) { MemoizePath *pathnode = makeNode(MemoizePath); @@ -1768,6 +1769,7 @@ create_memoize_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, pathnode->param_exprs = param_exprs; pathnode->singlerow = singlerow; pathnode->binary_mode = binary_mode; + pathnode->unhashable_params = unhashable_params; /* * For now we set est_entries to 0. cost_memoize_rescan() does all the @@ -4074,7 +4076,8 @@ reparameterize_path(PlannerInfo *root, Path *path, mpath->hash_operators, mpath->singlerow, mpath->binary_mode, - mpath->est_calls); + mpath->est_calls, + mpath->unhashable_params); } default: break; diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index 460c4f4d8dc..2b0c034052d 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -2359,6 +2359,11 @@ typedef struct MemoizePath Cardinality est_calls; /* expected number of rescans */ Cardinality est_unique_keys; /* estimated unique keys, for EXPLAIN */ double est_hit_ratio; /* estimated cache hit ratio, for EXPLAIN */ + Bitmapset *unhashable_params; /* paramids of Params appearing in the key + * expressions that couldn't be made cache + * keys (e.g. their types have no hash + * opclass); excluded from keyparamids so + * that changes flush the cache */ } MemoizePath; /* diff --git a/src/include/optimizer/pathnode.h b/src/include/optimizer/pathnode.h index e8db321f92b..ae37ebaf523 100644 --- a/src/include/optimizer/pathnode.h +++ b/src/include/optimizer/pathnode.h @@ -123,7 +123,8 @@ extern MemoizePath *create_memoize_path(PlannerInfo *root, List *hash_operators, bool singlerow, bool binary_mode, - Cardinality est_calls); + Cardinality est_calls, + Bitmapset *unhashable_params); extern GatherPath *create_gather_path(PlannerInfo *root, RelOptInfo *rel, Path *subpath, PathTarget *target, Relids required_outer, double *rows); diff --git a/src/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out index 2d24f4480c4..bddf63173e8 100644 --- a/src/test/regress/expected/memoize.out +++ b/src/test/regress/expected/memoize.out @@ -415,9 +415,9 @@ ON t1.a = t2.a;', false); DROP TABLE prt; RESET enable_partitionwise_join; --- Exercise Memoize code that flushes the cache when a parameter changes which --- is not part of the cache key. --- Ensure we get a Memoize plan +-- Ensure Params which are part of the base quals are also added as a cache +-- key. +-- Ensure we get a Memoize plan with the Param as a cache key EXPLAIN (COSTS OFF) SELECT unique1 FROM tenk1 t0 WHERE unique1 < 3 @@ -435,8 +435,8 @@ WHERE unique1 < 3 -> Index Scan using tenk1_hundred on tenk1 t2 Filter: (t0.two <> four) -> Memoize - Cache Key: t2.hundred - Cache Mode: logical + Cache Key: t2.hundred, t0.ten + Cache Mode: binary -> Index Scan using tenk1_unique1 on tenk1 t1 Index Cond: (unique1 = t2.hundred) Filter: (t0.ten = twenty) @@ -454,6 +454,43 @@ WHERE unique1 < 3 2 (1 row) +-- Ensure a Param which is buried inside a larger cache key expression is made +-- a cache key by itself too. +-- Ensure we get a Memoize plan with both cache keys +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM tenk1 t1 + INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 2) s; + QUERY PLAN +--------------------------------------------------------------------------- + Aggregate + -> Index Scan using tenk1_unique1 on tenk1 t0 + Index Cond: (unique1 < 2) + SubPlan expr_1 + -> Aggregate + -> Nested Loop + -> Index Only Scan using tenk1_hundred on tenk1 t2 + -> Memoize + Cache Key: (t2.hundred + t0.ten), t0.ten + Cache Mode: binary + -> Index Scan using tenk1_unique1 on tenk1 t1 + Index Cond: (unique1 = (t2.hundred + t0.ten)) + Filter: (twenty = t0.ten) +(13 rows) + +-- Ensure the above query returns the correct result +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM tenk1 t1 + INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 2) s; + sum +------ + 1000 +(1 row) + RESET enable_seqscan; RESET enable_material; RESET enable_mergejoin; @@ -461,6 +498,186 @@ RESET work_mem; RESET hash_mem_multiplier; RESET enable_bitmapscan; RESET enable_hashjoin; +-- Test the original report "bug: query returns different result with and +-- without memoization". The result must be the same with Memoize on and off. +SET enable_seqscan TO off; +SET enable_material TO off; +SET enable_mergejoin TO off; +SET enable_hashjoin TO off; +SET work_mem TO '64kB'; +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; + QUERY PLAN +--------------------------------------------------------------------------- + Aggregate + -> Bitmap Heap Scan on tenk1 t0 + Recheck Cond: (unique1 < 200) + -> Bitmap Index Scan on tenk1_unique1 + Index Cond: (unique1 < 200) + SubPlan expr_1 + -> Aggregate + -> Nested Loop + -> Index Only Scan using tenk1_hundred on tenk1 t2 + -> Memoize + Cache Key: (t2.hundred + t0.ten), t0.ten + Cache Mode: binary + -> Index Scan using tenk1_unique1 on tenk1 t1 + Index Cond: (unique1 = (t2.hundred + t0.ten)) + Filter: (twenty = t0.ten) +(15 rows) + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; + sum +-------- + 100000 +(1 row) + +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; + sum +-------- + 100000 +(1 row) + +RESET enable_seqscan; +RESET enable_material; +RESET enable_mergejoin; +RESET enable_hashjoin; +RESET work_mem; +-- Test Params of types with no hash opclass (point) buried inside cache key +-- expressions. Such Params can't be made cache keys; instead they must not +-- be part of keyparamids, so that the cache is flushed when their values +-- change. +CREATE TABLE mp0 (p point, id int); +CREATE TABLE mpa (n int, k int); +CREATE TABLE mpb (n int, k int); +CREATE INDEX mpa_n ON mpa(n); +CREATE INDEX mpb_n ON mpb(n); +INSERT INTO mp0 SELECT point(d, d), 1 FROM generate_series(1, 5) d; +INSERT INTO mpa SELECT g % 20, g % 10 FROM generate_series(1, 1000) g; +INSERT INTO mpb SELECT g % 20, g % 10 FROM generate_series(1, 1000) g; +ANALYZE mp0; ANALYZE mpa; ANALYZE mpb; +SET enable_seqscan TO off; +SET enable_material TO off; +SET enable_mergejoin TO off; +SET enable_hashjoin TO off; +-- buried unhashable Param also used in a base qual; without the fix the +-- cache would serve stale entries +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------- + Aggregate + -> Seq Scan on mp0 + Disabled: true + SubPlan expr_1 + -> Aggregate + -> Nested Loop + -> Seq Scan on mpa + Disabled: true + -> Memoize + Cache Key: (mpa.k + (((mp0.p <-> '(0,0)'::point) * '3'::double precision))::integer) + Cache Mode: logical + -> Index Scan using mpb_n on mpb + Index Cond: (n = (mpa.k + (((mp0.p <-> '(0,0)'::point) * '3'::double precision))::integer)) + Filter: (k = ((((mp0.p <-> '(0,0)'::point) * '9'::double precision))::integer % 10)) +(14 rows) + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; + sum +------- + 15000 +(1 row) + +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; + sum +------- + 15000 +(1 row) + +-- buried unhashable Param used only in the key expression; the plan must +-- keep the Memoize node +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; + QUERY PLAN +------------------------------------------------------------------------------------------------------------------------- + Aggregate + -> Seq Scan on mp0 + Disabled: true + SubPlan expr_1 + -> Aggregate + -> Nested Loop + -> Seq Scan on mpa + Disabled: true + -> Memoize + Cache Key: (mpa.k + (((mp0.p <-> '(0,0)'::point) * '3'::double precision))::integer) + Cache Mode: logical + -> Index Scan using mpb_n on mpb + Index Cond: (n = (mpa.k + (((mp0.p <-> '(0,0)'::point) * '3'::double precision))::integer)) + Filter: (k = 1) +(14 rows) + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; + sum +------- + 10000 +(1 row) + +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; + sum +------- + 10000 +(1 row) + +RESET enable_seqscan; +RESET enable_material; +RESET enable_mergejoin; +RESET enable_hashjoin; +DROP TABLE mp0, mpa, mpb; -- Test parallel plans with Memoize SET min_parallel_table_scan_size TO 0; SET parallel_setup_cost TO 0; diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql index c02a0d51af4..e87df64bbb8 100644 --- a/src/test/regress/sql/memoize.sql +++ b/src/test/regress/sql/memoize.sql @@ -198,10 +198,10 @@ DROP TABLE prt; RESET enable_partitionwise_join; --- Exercise Memoize code that flushes the cache when a parameter changes which --- is not part of the cache key. +-- Ensure Params which are part of the base quals are also added as a cache +-- key. --- Ensure we get a Memoize plan +-- Ensure we get a Memoize plan with the Param as a cache key EXPLAIN (COSTS OFF) SELECT unique1 FROM tenk1 t0 WHERE unique1 < 3 @@ -218,6 +218,24 @@ WHERE unique1 < 3 INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred WHERE t0.ten = t1.twenty AND t0.two <> t2.four OFFSET 0); +-- Ensure a Param which is buried inside a larger cache key expression is made +-- a cache key by itself too. + +-- Ensure we get a Memoize plan with both cache keys +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM tenk1 t1 + INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 2) s; + +-- Ensure the above query returns the correct result +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM tenk1 t1 + INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 2) s; + RESET enable_seqscan; RESET enable_material; RESET enable_mergejoin; @@ -226,6 +244,112 @@ RESET hash_mem_multiplier; RESET enable_bitmapscan; RESET enable_hashjoin; +-- Test the original report "bug: query returns different result with and +-- without memoization". The result must be the same with Memoize on and off. +SET enable_seqscan TO off; +SET enable_material TO off; +SET enable_mergejoin TO off; +SET enable_hashjoin TO off; +SET work_mem TO '64kB'; + +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT t0.unique1, + (SELECT count(*) FROM tenk1 t2 JOIN tenk1 t1 + ON t1.unique1 = t2.hundred + t0.ten + WHERE t1.twenty = t0.ten) AS c + FROM tenk1 t0 WHERE t0.unique1 < 200) s; + +RESET enable_seqscan; +RESET enable_material; +RESET enable_mergejoin; +RESET enable_hashjoin; +RESET work_mem; + +-- Test Params of types with no hash opclass (point) buried inside cache key +-- expressions. Such Params can't be made cache keys; instead they must not +-- be part of keyparamids, so that the cache is flushed when their values +-- change. +CREATE TABLE mp0 (p point, id int); +CREATE TABLE mpa (n int, k int); +CREATE TABLE mpb (n int, k int); +CREATE INDEX mpa_n ON mpa(n); +CREATE INDEX mpb_n ON mpb(n); +INSERT INTO mp0 SELECT point(d, d), 1 FROM generate_series(1, 5) d; +INSERT INTO mpa SELECT g % 20, g % 10 FROM generate_series(1, 1000) g; +INSERT INTO mpb SELECT g % 20, g % 10 FROM generate_series(1, 1000) g; +ANALYZE mp0; ANALYZE mpa; ANALYZE mpb; + +SET enable_seqscan TO off; +SET enable_material TO off; +SET enable_mergejoin TO off; +SET enable_hashjoin TO off; + +-- buried unhashable Param also used in a base qual; without the fix the +-- cache would serve stale entries +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = ((mp0.p <-> point '(0,0)') * 9)::int % 10) AS c + FROM mp0) s; + +-- buried unhashable Param used only in the key expression; the plan must +-- keep the Memoize node +EXPLAIN (COSTS OFF) +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; + +SET enable_memoize = off; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; +SET enable_memoize = on; +SELECT sum(c) FROM ( + SELECT (SELECT count(*) FROM mpa JOIN mpb + ON mpb.n = mpa.k + ((mp0.p <-> point '(0,0)') * 3)::int + WHERE mpb.k = 1) AS c + FROM mp0) s; + +RESET enable_seqscan; +RESET enable_material; +RESET enable_mergejoin; +RESET enable_hashjoin; +DROP TABLE mp0, mpa, mpb; + -- Test parallel plans with Memoize SET min_parallel_table_scan_size TO 0; SET parallel_setup_cost TO 0; -- 2.43.0