BUG #19742: `INTERSECT` under a `UNION ALL` with an empty arm fails with "could not find pathkey item t"

Hi Tom,

I suspect that that commit just allowed reaching some pre-existing
mistake, but I've not dug into it.

Yes, I think the mistake is older.

The "WHERE false" path is removed, so the UNION ALL has one child
left, the INTERSECT. For an Append with one child,
create_append_path() copies the child's pathkeys. Later
create_append_plan() looks for those sort columns in the Append's
own targetlist. It can't find them, and we get the error.

It can't find them because the SetOp's pathkeys are wrong. A sorted
SetOp reuses the pathkeys of its left input. Here "a = 1" makes the
subquery skip its own sort , so we add a Sort on top of the subquery.
That Sort's pathkeys are built from the subquery's columns, not from
the SetOp's output columns.

There is a second way to get wrong pathkeys, with no SetOp at all.
When the column types differ, recurse_set_operations() adds a
projection but keeps the old pathkeys

(select a from d union select a from d)
union all select a::numeric from d where false;

So I did not fix the SetOp. 0001 makes these one-child Appends drop
the child's pathkeys, which covers both cases. The planner adds a
Sort above if it needs the order. 0002 adds tests for the three
queries.

This is the small fix, meant for 19 and master.

My first try, v1, only fixed the SetOp's pathkeys. It fixed the
reported query but changed many plans, so I think it is too much for
19.

I think a better fix would make the pathkeys right in both places,
so the Append can keep them. I can work on that for master if you
like.

Thanks,
Shihao

On Sun, 4 Oct 2026 at 14:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:

and the reason it doesn't fail is that the AppendPath has nil pathkeys
in this case. So (I speculate that) we never attached pathkeys to a
UNION ALL AppendPath before, and the reason we're trying to now has
something to do with having reduced the child list to a singleton.

It looks like a bug in add_setop_child_rel_equivalences(). It wrongly
assumes that setop_pathkeys will contain a PathKey for each tlist
entry. The problem query has a redundant PathKey due to the WHERE a =
1.

I think the fix needs to be either don't remove redundant pathkeys for
setop_pathkeys or use some other method to figure out which
expressions to add in add_child_eq_member().

David

I wrote:

My own thoughts were along the lines of "don't ever assign pathkeys to
an AppendPath"; not sure if that's equivalent to your first idea.

Concretely, the attached fixes the given test case. There are other
calls to create_append_path in prepunion.c, and I think they may
all need to do likewise, but I didn't analyze them.

It'd be nominally cleaner to add a flag to create_append_path telling
it whether it's allowed to override the given pathkeys. I didn't do
that here because it seems like this is a localized problem that
should eventually be fixed inside prepunion.c, but there's room to
argue differently.

regards, tom lane

Hi Tom,

There are other calls to create_append_path in prepunion.c, and I
think they may all need to do likewise, but I didn't analyze them.

The EXCEPT ALL one needs it too. With only your patch this still
fails:
set enable_hashagg = off;
(select a from d where a = 1 intersect select a from d where a = 1)
except all select a from d where false;

The v2-0001 I posted upthread changes both places. The partial
Append looks safe, its children always have NIL pathkeys.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

Hi Tom,

There are other calls to create_append_path in prepunion.c, and I
think they may all need to do likewise, but I didn't analyze them.

The EXCEPT ALL one needs it too. With only your patch this still
fails:
set enable_hashagg = off;
(select a from d where a = 1 intersect select a from d where a = 1)
except all select a from d where false;

Ah. I'd been trying to make a test case for that one, but I didn't
realize that two levels of setop are required. Something like this
doesn't fail:

explain
select * from
((select * from int8_tbl i81 order by q1)
except all (select * from int8_tbl i81b where false)) ss1,
((select * from int8_tbl i82 order by q1)
except all (select * from int8_tbl i82b where false)) ss2
where ss1.q1 = ss2.q1
;

However, digging into the guts of that doesn't leave a warm feeling
either. Unpatched, we end up with the same situation where a
single-child AppendPath has a tlist containing varno-0 Vars and a
pathkey, and create_append_plan tries to compute sort column info from
that. The reason it fails to fail is that *the equivalence classes
contain varno-0 Vars too*. I've not entirely figured out why this
is different from the original test case --- well, okay, UNION ALL
at the top level is different because it doesn't make any pathkeys,
but if you change that to UNION the test case still fails on unpatched
code, and in that case the pathkey-slinging sure looks the same.

Regardless of the detailed reason for that, having varno-0 Vars in
equivalence classes scares the dickens out of me. Each setop node
is going to have its own varno-0 Vars, and if they match on type then
the equivalence class machinery can't tell them apart, so it sure
seems like we are at risk of drawing false conclusions about whether
different setop outputs are sorted alike. In the above example I was
trying to break it by having it falsely deduce that the outer WHERE
clause could be thrown away or reduced to an IS NOT NULL test.
I failed, which turns out to be because the questionable eclasses are
not at top level but within the two subroots associated with the two
setop nests. So I think the potential bad effects are limited to
maybe mistakenly planning a single setop nest, and so far we've
escaped issues mainly because we don't do that much optimization of
non-UNION-ALL nests. But it's really past time to get rid of the
varno-0 representation. For now, one reason I like forcing these
paths' pathkeys to nil is that it limits the amount of damage that
could be done by false equivalence-class reasoning. In particular,
I wonder whether v19/HEAD are at risk of such bugs in cases that
couldn't occur before we started eliding dummy child setops.

regards, tom lane

Hi Tom,

In particular, I wonder whether v19/HEAD are at risk of such bugs in
cases that couldn't occur before we started eliding dummy child
setops.

I looked for one with random queries. The attached script builds
nested setops with empty arms and mixed int and numeric columns,
puts an ORDER BY, a join or a second setop nest above them, and
checks the output against an answer worked out in Python. Obviously
I worked with Cladue to get that script generated.

I ran 20000 queries, each with seven sets of planner settings. On
HEAD 2516 of the 140000 runs hit the pathkey error. With both
Appends forced to NIL pathkeys none did. There were no wrong
answers in either build.

It did find one more varno 0 problem, in EXPLAIN only. 18 is fine.
create table d1 (a int);
create table d2 (b numeric);
explain (verbose, costs off)
select b from d2 except
(select a from d1 where false except all select a from d1);
ERROR: bogus varno: 0

928df067d1e handles a plain Var in the dummy Result's tlist. Here
the Var is under an int to numeric cast, so set_plan_refs() misses
it.

The attached patch walks the whole expression. It keeps the
varno 1 rewrite, so for a nested setop the name shown can come from
another child, as in the new test's output.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

928df067d1e handles a plain Var in the dummy Result's tlist. Here
the Var is under an int to numeric cast, so set_plan_refs() misses
it.
The attached patch walks the whole expression. It keeps the
varno 1 rewrite, so for a nested setop the name shown can come from
another child, as in the new test's output.

Good catch, but there's another problem here. I wondered why the
Result is claiming to output "two", when that is not either of
the columns being output by the removed setop leaf queries.
This same code is at fault: it's injecting varno "1" without
regard for which of the leaf queries are actually represented.
Fortunately, now that we have Result.relids, it's pretty easy
to discover which leaf queries are represented and choose the
leftmost one. Hence, v2 attached.

By the way, I'm still not super happy about

+ Replaces: Aggregate on unnamed_subquery, unnamed_subquery_1

when there is no aggregation going on anywhere. But that's
because Robert took shortcuts: show_result_replacement_info
does

case RESULT_TYPE_UPPER:
/* a small white lie */
replacement_type = "Aggregate";
break;

without regard for the actual reason the Result got injected.
I recall complaining about that and Robert not wanting to add
yet more complexity to what he was doing. Which is fair,
but I still think we're gonna get bug reports about this.

regards, tom lane

Hi Tom,

Fortunately, now that we have Result.relids, it's pretty easy
to discover which leaf queries are represented and choose the
leftmost one. Hence, v2 attached.

Thanks, v2 looks good to me. With it, EXPLAIN works on all of the
same 20000 random queries.

With the dummy setop inside a subquery, under a JOIN or GROUP BY,
the Result shows the columns of its own left-most leaf.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> writes:

This is the small fix, meant for 19 and master.

Sorry for having overlooked this email earlier. It's substantially
the same fix I came up with, so I credited you as co-author.
I've pushed these two fixes and marked the open item as done.

I think a better fix would make the pathkeys right in both places,
so the Append can keep them. I can work on that for master if you
like.

If you're interested in working on a long-term fix, I think the path
forward ought to be to get rid of these "varno 0" Vars in favor of
using ordinary Vars that reference real RangeTblEntrys. Right now,
a Query level that represents a set-op nest only has RTE_SUBQUERY
RTEs for the leaf queries. I'm imagining inventing a new RTEKind,
say RTE_SETOP, and building one of those for each set operation
in the nest. Then the Vars representing the output columns of that
set operation could carry that RTE's index, and everything gets a
lot less magic. I'm not sure that there would be any large reduction
in total lines of code, but it'd be cleaner, and there are some things
such as tlist width estimation that would work better.

While we could move much of what's in SetOperationStmt into such
RTEs, I'd be inclined not to, because additional fields in
RangeTblEntry would just be bloat for non-SETOP RTEs. So my
druthers would be to add no new fields to RangeTblEntry, just
re-use whatever ones are there that are useful. SetOperationStmt
probably needs to gain a field for the index of the associated
RTE, though.

IIRC, there are XXX comments in prepunion.c whining about how
building the setop tlists ought to be done at parse time, so
that's something we could look into at the same time, or as a
follow-on patch.

regards, tom lane

Hi Tom,

Thanks for pushing these, and for the credit.

I'm imagining inventing a new RTEKind, say RTE_SETOP, and building
one of those for each set operation in the nest.

Yes, I would like to work on that. I will start with the RTE and the
Vars, and leave building the setop tlists at parse time for a
follow-on patch.

I will post the first version in a new thread on hackers.

Thanks,
Shihao