Hi David,
On Thu, Oct 29, 2026 at 1:26 AM David Geier <[email protected]> wrote:
> > I believe this is an issue since b8d7f053c5c (Postgres 10, CCing
> > Andres as the author of that one) changed how ExecInitHashJoin calls
> > ExecInitExpr, creating the second SubPlanState. More digging also
> > found the very old a64846f3ada that talked about a seemingly related
> > problem, but that must have happened before parallel query was in the
> > picture.
>
> (gdb) p *planstate
> $17 = {
> type = T_HashJoinState,
> plan = 0x5a9ef8df0040,
> state = 0x5a9ef8ddfed0,
> ...
> (gdb) p ((SubPlanState *)planstate->subPlan->elements[0])->planstate
> $15 = (PlanState *) 0x5a9ef8de01e0
> (gdb) p ((SubPlanState *)planstate->subPlan->elements[1])->planstate
> $16 = (PlanState *) 0x5a9ef8de01e0
>
> This happens in ExecInitSubPlanExpr() which adds the sort PlanState
> twice to the subPlan list.
Yep, exactly - thanks for adding those details.
> > To fix this, the least convoluted way I could think of is to invent a
> > new mechanism to collect a deduplicated list of plan states, and then
> > call the existing logic on each list member. This also allows some
> > refactoring to remove logic for counting how many nodes we've visited.
>
> While I like generally like your fix and the resulting code
> simplifications, I'm wondering if it's the right fix.
>
> It seems to me that the PlanState graph structure is broken for the case
> you encountered. While it's generally possible that a PlanState is
> referenced more than once (e.g. multiple CteScanStates reference the
> same CteScanState::cteplanstate), it's weird that the same PlanState
> references another PlanState more than once.
Right - I agree.
>
> Beyond that, I'm wondering how much other code is broken or can break if
> we don't change this structurally in the graph. You found already
> another one in ExecShutdownNode().
>
> A simple alternative fix would to ensure inExecInitSubPlanExpr() that
> the subPlan list is always unique.
Yeah, that would be elegant, but I don't think it works. The problem
is that we can also have two different nodes reference the same
PlanState, like in this example with the same SubPlan being referenced
by different nodes underneath an Append (courtesy of Claude, FWIW):
create table t2s as
select i % 1000 as thousand, i % 2000 as twothousand
from generate_series(0, 999) i;
alter table t2s set (parallel_workers = 0);
-- Hash-partitioned table. The WHERE clause below is copied to every
-- partition scan, each with its own SubPlanState for the same subplan.
create table tp (unique1 int, hundred int) partition by hash (unique1);
create table tp0 partition of tp for values with (modulus 3, remainder 0);
create table tp1 partition of tp for values with (modulus 3, remainder 1);
create table tp2 partition of tp for values with (modulus 3, remainder 2);
insert into tp select i, i % 100 from generate_series(0, 299999) i;
vacuum analyze t2s;
vacuum analyze tp;
-- Make a parallel plan attractive on a small test table.
set parallel_leader_participation = off;
set parallel_setup_cost = 0;
set parallel_tuple_cost = 0;
set min_parallel_table_scan_size = 0;
set max_parallel_workers_per_gather = 2;
explain (analyze, costs off, timing off, summary off, buffers off)
select count(*)
from tp
where hundred = case when 5 not in
(select thousand from t2s order by twothousand limit 50)
then 1 else 2 end;
On master this shows in a non-assert enabled build (note the missing
Sort Method in the SubPlan):
Finalize Aggregate
-> Gather
Workers Planned: 2
-> Partial Aggregate
-> Parallel Append
-> Parallel Seq Scan on tp0 tp_1
Filter: (hundred = CASE WHEN (NOT (ANY (5
= (hashed SubPlan any_1).col1))) THEN 1 ELSE 2 END)
SubPlan any_1
-> Limit
-> Sort
Sort Key: t2s.twothousand
-> Seq Scan on t2s
-> Parallel Seq Scan on tp1 tp_2
Filter: (hundred = CASE WHEN (NOT (ANY (5
= (hashed SubPlan any_1).col1))) THEN 1 ELSE 2 END)
-> Parallel Seq Scan on tp2 tp_3
Filter: (hundred = CASE WHEN (NOT (ANY (5
= (hashed SubPlan any_1).col1))) THEN 1 ELSE 2 END)
>
> > With the attached patch, the instrumentation now shows correctly, and
> > the added regression test no longer fails with an assertion failure.
> >
> > ...
> > SubPlan any_1
> > -> Sort (actual rows=1000.00 loops=4)
> > Sort Key: o.f
> > Worker 0: Sort Method: quicksort Memory:
> > 48kB
> > Worker 1: Sort Method: quicksort Memory:
> > 48kB
> > -> Seq Scan on other o (actual
> > rows=1000.00 loops=4)
> >
> > We could alternatively deal with this in the existing walkers, but
> > then we'd have to modify the walker context in four different places
> > to carry the identities of already visited plan states.
>
> Really? Can't you check based on state you have already decide if it's
> already initialized and if so bail? For example in ExecSortInitializeDSM():
>
> if (node->shared_info != NULL)
> return;
I suppose, but that becomes a bit tedious since we're reinventing the
wheel on how to avoid duplicates in a customized way for each walker.
>
> > I also assume
> > that changing how ExecInitHashJoin works is off the table, but I admit
> > that logic goes a bit over my head.
>
> It's these two calls that cause the sort PlanState to be added twice:
>
> hjstate->hj_OuterHash =
> ExecBuildHash32Expr(hjstate->js.ps.ps_ResultTupleDesc,
> hjstate->js.ps.resultops,
> outer_hashfuncid,
> node->hashcollations,
> node->hashkeys,
> hash_strict,
> &hjstate->js.ps,
> 0);
>
> and
>
> hjstate->hashclauses =
> ExecInitQual(node->hashclauses, (PlanState *) hjstate);
>
> Both, the hash clauses and the qual, reference the sub-plan. How would
> you avoid this? Also, it's too easy for other plan nodes to do the same.
> Hence, i think it's better to fix it structurally.
Yeah, that's fair - I was mainly thinking "I don't see if there is a
way to solve it at that level, maybe someone else (Andres?) does", but
it seemed unlikely to me too.
> > Note that technically the ExecShutdownNode_walker suffers from the
> > same problem, but it doesn't appear to be a practical problem with
> > in-core code today, and it's a bit more complicated to fix with the
> > current nested InstrStartNode assumptions (that I intend to fix
> > separately in the stack-based instrumentation series).
>
> Can you give some more details why it's not a problem in-core and what
> the problem with InstrStart/StopNode() is?
The problem is that InstrStart/StopNode expect to be called in a
nested manner, so that if shutdown activity occurs in a node, the
parent node also gets its instrumentation updated. If we were to
process each plan state one by one in a list, that nested aspect gets
lost, and parent nodes would no longer carry the instrumentation
information (we'd have to invent a mechanism to add it back, which is
a bunch of code). This gets a lot easier in a world where we are more
explicit about accumulation of instrumentation data (as is the case in
the stack-based instrumentation patch series), but otherwise seems a
bit overkill.
> Any node sitting below the Sort node, which doesn't like having its
> shutdown function called twice, might cause problems. Especially,
> CustomScanStates that we don't have full control over could be problematic.
Yeah, that's a fair point - custom scans could certainly get
unexpected failures here.
Thanks,
Lukas
--
Lukas Fittl
