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

On Mon, Nov 2, 2026 at 12:18 PM David Geier <geidav(dot)pg(at)gmail(dot)com> wrote:
> 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.

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.

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

> 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

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

Preferences on these / can you think of a different 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.

Ack.

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

Yes, it would. For me it was mostly an argument to not invent a custom
way of solving it there.

Thanks,
Lukas

--
Lukas Fittl