| From: | Richard Guo <guofenglinux(at)gmail(dot)com> |
|---|---|
| To: | Yilin Zhang <jiezhilove(at)126(dot)com> |
| Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, Jacob Brazeal <jacob(dot)brazeal(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: bug: query returns different result with and without memoization. |
| Date: | 2026-10-28 02:23:43 |
| Message-ID: | CAMbWs49RL3=hAr_RgeJZk6eK9GHN_m9=MBNz22k9=LEFLGEaUQ@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Fri, Sep 11, 2026 at 4:10 PM Yilin Zhang <jiezhilove(at)126(dot)com> wrote:
> 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.
Good point. I agree that we should not give up Memoize only because a
buried Param is not hashable. For such a Param, purging the cache
whenever it changes is enough to get correct results.
But I think we don't need to track the unhashable Params separately.
I think it's simpler to put into keyparamids only the Params that we
know are determined by the cache keys. These are the nestloop Params
that replace the outer rel's Vars in the keys, because the inner scan
can only reference the outer rel through the keys. Also, in binary
mode, a Param that is a cache key by itself. Any other Param will
purge the cache when it changes.
While working on this, I found another wrong-results issue.
pull_paramids() also collects PARAM_EXTERN Params, so in a generic
plan an external Param in the cache key can be mistaken for the
executor Param with the same number, and then the cache is not purged
when that executor Param changes. The above change fixes this too.
Attached is v3, which is split into two patches.
0001 is the fix and is intended for back-patching. It makes each
Param buried inside a cache key expression a cache key by itself, so
the cache entries can still be reused across rescans of the enclosing
plan. If a buried Param is not hashable, we just leave it to the
purging. It also includes the keyparamids changes above.
0002 is for master only. It also makes cache keys of the upper-level
Params used in the inner rel's base quals, targetlist and the inner
side of its join clauses, which currently purge the cache whenever
they change. This is a trade-off: instead of purging the cache on
each change, we keep the entries and reuse them if the same Param
value comes back. If the values do not repeat across rescans, the old
entries just take up memory until they are evicted, and the planner
cannot tell the difference since it costs Memoize per rescan with an
empty cache.
I plan to push them (or at least 0001, back-patched to 14) in a few
days if there are no objections, since the next minor release is just
around the corner.
- Richard
| Attachment | Content-Type | Size |
|---|---|---|
| v3-0001-Fix-wrong-results-from-Memoize-with-a-Param-insid.patch | application/octet-stream | 19.8 KB |
| v3-0002-Make-cache-keys-of-other-Params-a-Memoize-subplan.patch | application/octet-stream | 6.0 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Ashutosh Bapat | 2026-10-28 02:50:09 | Re: [PATCH] regression test: view and rule definitions must round trip |
| Previous Message | shihao zhong | 2026-10-28 02:16:47 | Re: Deadlock detector fails to activate on a hot standby replica |