Re: Assert failure in parallel query due to SubPlan double visit for hash join

> 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)
>

Just realized that for CTEs it's different. The CTE is only stored in
initPlan and planstate_tree_walker() doesn't descend into
CTEScanState::cteplanstate. So every CTEScanState is only visited once.

But the example you found is a good one. Each PlanState only has a
single reference but in total there's more than a single reference to
the same PlanState.

So it seems like we cannot avoid this case by being smart about what we
store per node. In that case I would prefer if planstate_tree_walker()
would take care not further descending.

Do you know if there's any code that relies on nodes being visited
multiple times? If not, I would change planstate_tree_walker() such that
it visits every node only once. If there's such code, I would make
planstate_tree_walker() have two modes.

We could use Bitmapset on the plan_node_id to check if a node has been
visited already. That should be pretty cheap. Only question is how to
pass this through the recursion levels in a nice way.

>> 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 would think in almost all code it's pretty easy to check for "is
initialized" but it makes the code hard to read and it's easy to forget
without having such corner-case tests in place.

I would prefer a solution that avoids the problem in the first place.

>> 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.

That would naturally work with a solution that doesn't visit already
visited children, does it?

>> 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.
OK.

--
David Geier