Re: v12: ERROR: subplan "InitPlan 2 (returns $4)" was not initialized

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Wed, Apr 12, 2023 at 3:59 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> The v1 patch attached is enough to fix the immediate issue,
>> but there's another thing not to like, which is that we're also
>> discarding the costs associated with the initplans. That's
>> strictly cosmetic given that all the planning decisions are
>> already made, but it still seems potentially annoying if you're
>> trying to understand EXPLAIN output. So I'm inclined to instead
>> do something like v2 attached, which deals with that as well.
>> (On the other hand, we aren't bothering to fix up costs when
>> we move initplans around in materialize_finished_plan or
>> standard_planner ... so maybe that should be left for a patch
>> that fixes those things too.)

> +1 to the v2 patch.

Thanks for looking at this. After sleeping on it, I'm inclined
to use the v1 patch in the back branches and do the cost fixups
only in HEAD.

> * Should we likewise set the parallel_safe flag for topmost plan in
> SS_attach_initplans?

SS_attach_initplans is assuming that costs and parallel safety
already got dealt with, either by SS_charge_for_initplans or by
equivalent processing during create_plan. I did have an Assert
there about parallel_safe already being off in a draft version
of this patch, but dropped it after realizing that it'd have to
go away anyway when we fix things to allow parallel-safe initplans.
(I have a draft patch for that that I'll post separately.)

We could improve the comment for SS_attach_initplans to explicitly
mention parallel safety, though. Also, I'd better go look at the
create_plan code paths to make sure they are indeed accounting
for this.

> * In standard_planner around line 443, we move any initPlans from
> top_plan to the new added Gather node. But since we know that the
> top_plan is parallel_safe here, shouldn't it have no initPlans?

Right. Again, I did have such an Assert there in a draft version,
then decided it wasn't useful to change temporarily. However, the
follow-on patch removes that stanza altogether, and I suppose it
might as well remove an Assert as what's there now. I'll make it so.

regards, tom lane