From 0c445c979d2529547b0be56720d1c7a5f06008b4 Mon Sep 17 00:00:00 2001 From: Richard Guo Date: Tue, 28 Jul 2026 16:51:51 +0900 Subject: [PATCH v1] 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, hence a Param we cannot hash is simply left to the existing flushing behavior. --- src/backend/optimizer/path/joinpath.c | 153 ++++++++++++++++++++++---- src/backend/optimizer/util/clauses.c | 6 +- src/test/regress/expected/memoize.out | 47 +++++++- src/test/regress/sql/memoize.sql | 24 +++- 4 files changed, 196 insertions(+), 34 deletions(-) diff --git a/src/backend/optimizer/path/joinpath.c b/src/backend/optimizer/path/joinpath.c index 713283a73aa..4b82f7fe368 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); /* @@ -480,6 +483,7 @@ paraminfo_get_equal_hashops(PlannerInfo *root, ParamPathInfo *param_info, { List *lateral_vars; + List *params; ListCell *lc; *param_exprs = NIL; @@ -564,7 +568,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)) @@ -574,44 +577,146 @@ 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); + list_free(*operators); + list_free(*param_exprs); + return false; } + } - /* - * 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 diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index aa8886ec210..c560a9ce769 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -6253,7 +6253,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) @@ -6274,7 +6275,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/test/regress/expected/memoize.out b/src/test/regress/expected/memoize.out index 218972dfab8..912e15ba918 100644 --- a/src/test/regress/expected/memoize.out +++ b/src/test/regress/expected/memoize.out @@ -416,9 +416,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 @@ -436,8 +436,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) @@ -455,6 +455,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; diff --git a/src/test/regress/sql/memoize.sql b/src/test/regress/sql/memoize.sql index e39bbb65391..776340f7851 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; -- 2.39.5 (Apple Git-154)