Wrong results from a parameterized Append
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254098psql -h localhost -U postgresBuilt from patchset v3 (message #3), October 22, 2026 at 04:45 AM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254098_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254098_3 && git checkout t254098_3Patchset v3 (message #3) is on t254098_3
I ran into a wrong-result issue on master (also reproducible on v16
and later, I believe).
create table t1 (a int, b int);
create table t2 (a int, b int);
insert into t1 values (1, 0), (2, 0), (3, 0);
insert into t2 values (1, 1);
analyze t1, t2;
select * from t1 where t1.a in
(select s.a + t1.b
from (select a from t2 where t2.b > t1.b
union all
select a from t2 where t2.b > t1.b) s, t1 t3);
a | b
---+---
1 | 0
2 | 0
3 | 0
(3 rows)
Only the first row should be returned. Let's look at its plan:
Nested Loop Semi Join
-> Seq Scan on t1
-> Nested Loop
-> Seq Scan on t1 t3
-> Materialize
-> Append
-> Seq Scan on t2
Filter: (b > t1.b)
-> Seq Scan on t2 t2_1
Filter: (b > t1.b)
So where does the join filter for "t1.a = s.a + t1.b" go?
The clause is EC-derived, and the UNION ALL members get no child
version of the member "s.a + t1.b" since it references t1, so their
parameterized scans cannot enforce it. But create_append_path()
builds the Append's ParamPathInfo with get_baserel_parampathinfo(),
which generates the clause for the parent as if the Append could
enforce it. get_param_path_clause_serials() knows not to trust that
and intersects the children of an AppendPath, but for a Material path
on top of the Append it falls into the "baserel path" branch and
returns the parent's ppi_serials. create_nestloop_path() then drops
the clause from the semijoin as already enforced below.
I believe the same happens with any path that inherits its subpath's
ParamPathInfo: Memoize, Projection, Sort, IncrementalSort, Unique, Agg
and GroupingSets, although I don't have proof. Attached is a patch
that makes get_param_path_clause_serials() look through those paths,
and uses it in get_memoize_path() too, which reads ppi_serials
directly.
Thoughts?
- Richard
Richard Guo <guofenglinux@gmail.com> writes:
I ran into a wrong-result issue on master (also reproducible on v16
and later, I believe).
FWIW, this example reproduces for me on v17 and up, but not v16.
It might be worth determining why. A quick bisect run says that it
broke at
9f133763961e280d8ba692bcad0b061b861e9138 is the first bad commit
commit 9f133763961e280d8ba692bcad0b061b861e9138
Author: Alexander Korotkov <akorotkov@postgresql.org>
Date: Thu Feb 15 12:05:52 2024 +0200
Pull up ANY-SUBLINK with the necessary lateral support.
but I've not dug further than that.
I believe the same happens with any path that inherits its subpath's
ParamPathInfo: Memoize, Projection, Sort, IncrementalSort, Unique, Agg
and GroupingSets, although I don't have proof. Attached is a patch
that makes get_param_path_clause_serials() look through those paths,
and uses it in get_memoize_path() too, which reads ppi_serials
directly.
I think you're on to something, but this patch has a hasty look to
it. AFAICS, the root of the problem is that the wrapper-path-making
functions in pathnode.c just do
pathnode->path.param_info = subpath->param_info;
and we now see that that isn't good enough. I wonder if we should try
harder. At the very least I think it'd be advisable to make all of
those look like, say,
pathnode->path.param_info = get_wrapper_parampathinfo(subpath);
Even if get_wrapper_parampathinfo doesn't do anything beyond handing
back the subpath's param_info, it would provide a place for a comment
discussing what it's doing and why.
However, I feel like we should consider a redesign rather than just
a minimal fix. In particular, it took me a bit to reconstruct why
the code looks like it does now. I think it's this:
(1) Joins need special handling in get_param_path_clause_serials
because different join input pairs for the same result join may have
different joinrestrictinfo lists and thus enforce different sets of
rinfo serials; so we can't compute a ppi_serials value in advance.
(2) AppendPath needs special handling in case there are joins under
it, which would only occur for partitionwise joining. Otherwise
we could have precomputed the correct ppi_serials to begin with.
(I didn't check the code history, but I wonder if somebody rewrote
this for partitionwise joining and did not bother to add a comment
explaining that.)
But if that is the situation, then what about a wrapper path that
wraps a join? Seems like we need to be able to recurse and
compute a context-dependent set of ppi_serials for that too.
Now we could just recurse always, which is what your patch does, but
that feels annoyingly inefficient. I am wondering about a redesign
that would allow marking ParamPathInfos as context-dependent (if they
are for a join or something with a join under it) or not (everything
else). Then we only need to recurse when the hard cases occur,
and otherwise we can just believe the ppi_serials field.
Also, as a more nitpicky objection, I'd be inclined to refactor
get_param_path_clause_serials so that the whole thing is one big
switch over nodeTag(path), rather than the messy structure your
v1 patch would leave behind.
regards, tom lane
Here's a v2 patch incorporating the cosmetic changes I suggested,
and substantially enlarging the comments around the area.
I don't think we're ready to commit though, because I see a number
of loose ends.
I looked through the code and confirmed that ppi_rows is used only
as per the new comments, and that the mistake you found in
get_memoize_path() is the only unsafe use of ppi_serials.
However, there are a number of places in the memoize logic that
think they can use ppi_clauses, which seems about as unsafe.
I think that'll at worst cause some missed optimization chances,
but I'm not entirely certain.
Another interesting point is that Gather and GatherMerge are treated
like baserel paths, although what's under them might be a join or
append. This might be okay but I'm not quite convinced there either.
At least we probably should document this somewhere.
The fact that create_append_path sometimes uses
get_baserel_parampathinfo isn't reflected in my new comments either,
and I wonder if the fact that it does isn't breaking
get_param_path_clause_serials.
regards, tom lane
Hi Tom,
v2 looks right to me. Here is a case that fails on v16 too, using
plain LATERAL. With Richard's tables it gives 18 rows on 16 to
master, and 6 with v2:
set from_collapse_limit = 1;
set enable_hashjoin = off;
set enable_mergejoin = off;
select t1.*, ss.a from t1, lateral
(select s.a from t1 t3,
(select a from t2 where t2.b > t1.b
union all
select a from t2 where t2.b > t1.b) s) ss
where t1.a = ss.a + t1.b
The original SQL not work for 16 because v16 keeps a correlated I
N sublink as a SubPlan and never makes the semijoin. So we can
use Plain LATERAL to repro it.
Thanks,
Shihao