"Imai, Yoshikazu" <imai.yoshikazu@jp.fujitsu.com> writes:
> I changed the state of this patch to "Ready for Committer".
I pushed this with a bit of extra tweaking --- notably, I added an
Assert to ExecFindMatchingSubPlans to guard against the possibility
that someone calls it when the remapping hasn't been done.
I think that the main win here is the recognition that we only need
remapping when both do_initial_prune and do_exec_prune are true.
I suspect that's an unusual case, so that we'll usually get to skip
this code altogether.
I am not really convinced that the original idea of replacing the
loop-over-subplans with a bms_next_member() loop is a good one.
Yeah, it wins in the best case where there are a lot of subplans
and initial pruning got rid of nearly all of them --- but how often
is that true, given the context that both do_initial_prune and
do_exec_prune are needed? And does it still win if most of the
subplans *didn't* get pruned here? bms_next_member() is not cheap
compared to an increment-and-compare. I am distressed by the fact
that your performance testing seems to have examined only this
best case. IMO performance testing should usually worry more about
the possible downside in the worst case.
Rather than arguing about that, though, I think we should focus
on something else: I'd be a lot happier if this remapping logic
just disappeared completely. I doubt it's a performance issue
now that we run it only when initial and exec pruning are both
needed. But it seems like a source of complexity and bugs, most
especially the latter now that common cases won't run it. Why
is it that we can't fix things so that ExecFindMatchingSubPlans
just uses the original planner output data?
regards, tom lane