bug: query returns different result with and without memoization.

On Sat, Jul 25, 2026 at 5:04 AM Jacob Brazeal <jacob.brazeal@gmail.com> wrote:

--setup
CREATE TABLE tenk1 AS
SELECT g AS unique1, g%2 AS two, g%10 AS ten, g%20 AS twenty, g%100 AS hundred
FROM generate_series(0,9999) g;
CREATE INDEX tenk1_unique1 ON tenk1(unique1);
CREATE INDEX tenk1_hundred ON tenk1(hundred);
ANALYZE tenk1;

SET enable_seqscan=off; SET enable_mergejoin=off; SET work_mem='64kB';

Now return the query below with each of these settings:

SET enable_memoize = on; -- 82000 (incorrect)
SET enable_memoize = off; -- 100000 (correct)

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;

Thanks for the report! ExecReScanMemoize() purges the whole cache
only when a parameter that is not part of the cache key changes. It
assumes that a change to a cache-key parameter is handled by looking
up a different cache entry. But that assumption fails when a
parameter is buried inside a larger cache-key expression and also
appears elsewhere in the subplan.

In the reported case, there is a cache key of "t2.hundred + t0.ten"
and a separate filter "t1.twenty = t0.ten". The problem is that
different (t2.hundred, t0.ten) pairs can produce the same cache-key
value while the filter selects different rows.

I think we can fix it by dropping from keyparamids any parameter that
also appears outside the cache-key expressions. See the attached
patch.

- Richard

On Mon, 27 Jul 2026 at 20:46, Richard Guo <guofenglinux@gmail.com> wrote:

In the reported case, there is a cache key of "t2.hundred + t0.ten"
and a separate filter "t1.twenty = t0.ten". The problem is that
different (t2.hundred, t0.ten) pairs can produce the same cache-key
value while the filter selects different rows.

I think we can fix it by dropping from keyparamids any parameter that
also appears outside the cache-key expressions. See the attached
patch.

I'm not that keen on that being the fix for this. I think the correct
way to fix is to add the extra cache key. That way, we have at least
got a chance at costing Memoize correctly. I feel your method puts us
at risk of Memoize nodes suddenly having a terrible cache hit ratio in
the back branches. The alternative method of adding the extra key
does risk that Memoize is no longer chosen and some other (perhaps)
more suitable join method is selected instead. None of which is
particularly great in the back-branches, but I'd be quite nervous
about your method of flushing the cache each time a parameter like
this changes, as in the worst case scenario, Memoize might never get a
cache hit, and that won't be accounted for in the costs.

For the reported test case, with your patch I get:

-> Memoize (cost=0.30..0.36 rows=1 width=4) (actual
rows=0.05 loops=2000000)
Cache Key: (t2.hundred + t0.ten)
Cache Mode: logical
Estimates: capacity=100 distinct keys=100
lookups=10000 hit percent=99.00%
Hits: 1980000 Misses: 20000 Evictions: 19900
Overflows: 0 Memory Usage: 7kB

Whereas with my method:

-> Memoize (cost=0.30..0.36 rows=1 width=4) (actual
rows=0.05 loops=2000000)
Cache Key: (t2.hundred + t0.ten), t0.ten
Cache Mode: logical
Estimates: capacity=100 distinct keys=100
lookups=10000 hit percent=99.00%
Hits: 1999000 Misses: 1000 Evictions: 0
Overflows: 0 Memory Usage: 73kB

For reference, I was experimenting with the attached. What do you think?

David

On Mon, Jul 27, 2026 at 9:46 PM David Rowley <dgrowleyml@gmail.com> wrote:

For reference, I was experimenting with the attached. What do you think?

I looked over your patch, but I'm afraid it's not correct.

IIUC, your patch inspects innerrel->baserestrictinfo and only collects
the outer arg of a two-arg OpExpr.

First, this can pick the wrong arg. If the Param isn't a whole side
of the qual, we end up adding a bogus cache key while still missing
the Param itself. Consider the qual "t1.twenty - t0.ten = 0" in this
query:

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 = 0) AS c
FROM tenk1 t0 WHERE t0.unique1 < 200) s;

With your patch I get:

-> Memoize
Cache Key: (t2.hundred + t0.ten), 0
Cache Mode: logical
-> Index Scan using tenk1_unique1 on tenk1 t1
Index Cond: (unique1 = (t2.hundred + t0.ten))
Filter: ((twenty - t0.ten) = 0)

That is, the constant 0 becomes a cache key, while t0.ten is still
missing from the keys.

And running this query I get wrong result:

sum
--------
148000

where the correct result is 100000.

Second, I'm not convinced that base quals are the only place such a
Param can appear. It can also appear on the inner side of a join
clause, as in "t1.hundred + t0.ten = t2.hundred", where only the outer
side becomes a cache key. It could also be in innerrel->reltarget, or
inside a subquery RTE that wasn't pulled up, and no amount of base
qual scanning will reach those.

That is what worries me most about this approach: it requires us to
enumerate every place a Param might be used, and if we ever miss one,
we are back to the same kind of wrong results.

Besides correctness, I'm a bit concerned that returning false discards
the Memoize path entirely, and that a qual only has to contain a Param
to reach that test. Taking the same query with a plain join clause
and varying only the qual on t1:

SELECT sum(c) FROM (
SELECT (SELECT count(*) FROM tenk1 t1
INNER JOIN tenk1 t2 ON t1.unique1 = t2.hundred
WHERE <qual>) AS c
FROM tenk1 t0 WHERE t0.unique1 < 2) s;

none of these can use Memoize any more with your patch, though none of
them is broken today:

WHERE t1.twenty = t1.four + t0.ten -- sides don't match
WHERE t1.twenty IS DISTINCT FROM t0.ten -- not an OpExpr
WHERE t1.twenty = t0.ten OR t1.four = t0.two -- not an OpExpr

along with any Param whose type has no hash opclass. Today each of
those gets a working Memoize node that simply flushes the cache when
the Param changes. So I'm concerned about the plan regression in
cases that never had a problem.

So I'd like to propose the attached patch. Rather than looking for
every place a Param might be used, it requires that every PARAM_EXEC
Param appearing in a cache key expression is also a cache key in its
own right. Then matching the keys implies matching the Param, and the
set we have to examine is bounded, because it's just the key list we
already built.

The patch then also makes cache keys of the Params used outside the
cache keys: in the relation's base quals, its targetlist, and on the
inner side of the join clauses. It's what stops the cache being
purged on every invocation of a correlated subplan. It's only an
optimization, so it's ok if we miss a place a Param might be used.

- Richard

At 2026-07-28 17:53:00, "Richard Guo" <guofenglinux@gmail.com> wrote:

along with any Param whose type has no hash opclass. Today each of
those gets a working Memoize node that simply flushes the cache when
the Param changes. So I'm concerned about the plan regression in
cases that never had a problem.
...
So I'd like to propose the attached patch. Rather than looking for
every place a Param might be used, it requires that every PARAM_EXEC
Param appearing in a cache key expression is also a cache key in its
own right. Then matching the keys implies matching the Param, and the
set we have to examine is bounded, because it's just the key list we
already built.

The patch then also makes cache keys of the Params used outside the
cache keys: in the relation's base quals, its targetlist, and on the
inner side of the join clauses. It's what stops the cache being
purged on every invocation of a correlated subplan. It's only an
optimization, so it's ok if we miss a place a Param might be used.

- Richard

Hi,
Thank you for working on this bug.

+	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;
 		}
+	}

From the earlier discussion we learned that when a Param's type has no
hash opclass (geometric types such as point and box, domains over such
types, and custom types), memoize_add_cache_key() fails and the Memoize
path is abandoned entirely. Among types commonly used in production,
point has no hash or btree opclass at all, which can increase the scan
cost severalfold.

Building on your v1-0001-Fix-wrong-results-from-Memoize-with-a-Param-insid.patch,
I made some changes:

For unhashable buried Params, keep the attempt to add them as bare cache
keys; when that fails, record them in unhashable_params and exclude them
from keyparamids at createplan time, so that when such a Param changes it
falls into the executor's existing bms_nonempty_difference(chgParam,keyparamids)
difference set and triggers cache_purge_all().

I tested some types without hash opclass; the performance is basically
unchanged while the results remain correct.

Best regards,

--

Yilin Zhang