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

> Thanks for confirming - I do agree that if we can make
> planstate_tree_walker do this consistently that would be good - I'm
> mainly not sure how awkward we make the API with that.

I think it won't be too bad. There are also only very fw call sites for
planstate_tree_walker().

>> 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.
>
> From my read I don't think any of the callers expect it to visit the
> same node multiple times, but if we go this way we should review one
> more time.

- ExplainPreScanNode() is fine either way.
- ExecShutdownNode_walker() should be fixed as discussed.
- The remaining calls are all from execParallel.c and should also be
fixed, shouldn't they?

These are the only call sites I can see. Based on that we could change
planstate_tree_walker() to visit every node only once.

>> 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.
>
> Yeah, exactly - I think there are two ways I can think of to solve it:
>
> 1) Make context not be generic, but rather a common base context
> struct (with the Bitmapset) that is embedded as the first field in the
> other context structs

That sounds good. Maybe it's enough to require that the first member of
any context is a Bitmapset. Bitmapset is a node so we could assert on
that as well.

> 2) Pass an extra argument to the walker functions and callback that
> contains the Bitmapset

That requires then always two state variables at the call sites. I
prefer (1).

> Preferences on these / can you think of a different way?

An alternative to the Bitmapset would be storing traversal state inside
struct PlanState. The advantage of that is performance. A "has been
visited" check would be as cheap as comparing two variables.

A visited flag has the disadvantage that it needs to be reset after the
traversal has completed. What usually works quite nicely is a "traversal
generation" counter that is incremented prior to each traversal. A node
is considered visited if that node's counter is equal to the current
traversal generation (which means it has been incremented already during
the current traversal). The counter could be as small as 1 bit. We can
rely on wrap around to be different for each two successive traversals.

I'm leaning towards the less intrusive Bitmapset variant. The
is-member-of checks should be fast enough and likely be negligible
compared to all the cache misses when traversing the graph. We can
profile that.

>>> 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?
>
> Yes, it would. For me it was mostly an argument to not invent a custom
> way of solving it there.

Cool!

--
David Geier