Re: BUG #17606: There is still some glitch in 3f7323cbb fixing failure of MULTIEXPR_SUBLINK

The following bug has been logged on the website:

Bug reference: 17606
Logged by: Andre Lin
Email address: 857348270(at)qq(dot)com
PostgreSQL version: 13.8
Operating system: Linux x86_64 GNU/Linux
Description:

3f7323cbb regenerates its Param for each SubPlan by traversing the
targetlist. But ignore one point: initplan is not in targetlist.
This will result in a failed assertion, or an error like "unexpected
PARAM_MULTIEXPR ID: 131074"

reproduce it is simple, change the regress sql in inherit.sql to

explain (verbose, costs off)
update inhpar set (f1, f2[1]) = (select 1, '1' from int4_tbl limit 1)
from onek p2 where inhpar.f1 = p2.unique1;

On Mon, Sep 5, 2022 at 3:33 PM PG Bug reporting form <noreply(at)postgresql(dot)org>
wrote:

> 3f7323cbb regenerates its Param for each SubPlan by traversing the
> targetlist. But ignore one point: initplan is not in targetlist.
> This will result in a failed assertion, or an error like "unexpected
> PARAM_MULTIEXPR ID: 131074"

Since initplan SubPlans do not have args lists, I think it's OK for them
to share output parameters. So maybe we do not need to do the
SS_make_multiexprs_unique trick for initplan SubPlans?

Thanks
Richard

On Mon, Sep 5, 2022 at 10:25 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:

>
> On Mon, Sep 5, 2022 at 3:33 PM PG Bug reporting form <
> noreply(at)postgresql(dot)org> wrote:
>
>> 3f7323cbb regenerates its Param for each SubPlan by traversing the
>> targetlist. But ignore one point: initplan is not in targetlist.
>> This will result in a failed assertion, or an error like "unexpected
>> PARAM_MULTIEXPR ID: 131074"
>
>
> Since initplan SubPlans do not have args lists, I think it's OK for them
> to share output parameters. So maybe we do not need to do the
> SS_make_multiexprs_unique trick for initplan SubPlans?
>

If I consider it correctly, can we fix the initplan SubPlan issue simply
as below?

--- a/src/backend/optimizer/plan/subselect.c
+++ b/src/backend/optimizer/plan/subselect.c
@@ -911,6 +911,13 @@ SS_make_multiexprs_unique(PlannerInfo *root,
PlannerInfo *subroot)
new_multiexpr_params = lappend(new_multiexpr_params,
params);
}

+ /*
+ * It's OK for initplan SubPlans to share output parameters, so we
do not
+ * need to generate new Param nodes for them.
+ */
+ if (new_multiexpr_params == NIL)
+ return;
+
/*
* Now we must find the Param nodes that reference the MULTIEXPR
outputs
* and update their sublink IDs so they'll reference the new
outputs.

Thanks
Richard

Richard Guo <guofenglinux(at)gmail(dot)com> writes:
> On Mon, Sep 5, 2022 at 10:25 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>> On Mon, Sep 5, 2022 at 3:33 PM PG Bug reporting form <
>> noreply(at)postgresql(dot)org> wrote:
>>> 3f7323cbb regenerates its Param for each SubPlan by traversing the
>>> targetlist. But ignore one point: initplan is not in targetlist.
>>> This will result in a failed assertion, or an error like "unexpected
>>> PARAM_MULTIEXPR ID: 131074"

>> Since initplan SubPlans do not have args lists, I think it's OK for them
>> to share output parameters. So maybe we do not need to do the
>> SS_make_multiexprs_unique trick for initplan SubPlans?

Yeah, I agree. The initial loop will do nothing anyway since the initplan
will no longer have a SubPlan in the tlist. The problem is to get the
second loop to not adjust the numbers of the associated output Params.

> If I consider it correctly, can we fix the initplan SubPlan issue simply
> as below?

No, that'll malfunction if there's a mix of initplan and subplan
MULTIEXPRs. I think we're going to have to bite the bullet and find a way
to discover the SubLinkIds of the non-initplan MULTIEXPRs, so that the
second loop can identify which Params to change and which not to.

AFAIR that info's no longer available by the time we get here, so
I imagine that a fix will require recording some more data during
SubLink-to-SubPlan conversion.

I'm sure glad that this isn't stuff we're going to need to carry
into the future...

regards, tom lane

I wrote:
> No, that'll malfunction if there's a mix of initplan and subplan
> MULTIEXPRs. I think we're going to have to bite the bullet and find a way
> to discover the SubLinkIds of the non-initplan MULTIEXPRs, so that the
> second loop can identify which Params to change and which not to.
> AFAIR that info's no longer available by the time we get here, so
> I imagine that a fix will require recording some more data during
> SubLink-to-SubPlan conversion.

After contemplating that for awhile, by far the least painful way
seems to be to add the subLinkId to struct SubPlan. We can get
away with that in the back branches as long as we add it at the
end, since SubPlans don't get recorded in the catalogs.

regards, tom lane

&gt; After contemplating that for awhile, by far the least painful way
&gt; seems to be to add the subLinkId to struct SubPlan.&nbsp; We can get
&gt; away with that in the back branches as long as we add it at the
&gt; end, since SubPlans don't get recorded in the catalogs.

But let's assume that we manage to verify initplan/subplan by adding
subLinkId to&nbsp;SubPlan, we still have problem. In short, the exact
subqueryid of the new MULTIEXPR&nbsp;Param will be pain in the ass...

Consider the UPDATE sql containing sublinks by order like (subplan,
initplan, subplan, ...)

The existing code "offset = list_length(root-&gt;multiexpr_params)"
will count initplan into "offset", which is correct when first time here,
but it gives wrong subqueryid after...(when the place of initplan should
be "skipped")

"=?ISO-8859-1?B?QW5kcmUgTGlu?=" <857348270(at)qq(dot)com> writes:
> &gt; After contemplating that for awhile, by far the least painful way
> &gt; seems to be to add the subLinkId to struct SubPlan.&nbsp; We can get
> &gt; away with that in the back branches as long as we add it at the
> &gt; end, since SubPlans don't get recorded in the catalogs.

> But let's assume that we manage to verify initplan/subplan by adding
> subLinkId to&nbsp;SubPlan, we still have problem. In short, the exact
> subqueryid of the new MULTIEXPR&nbsp;Param will be pain in the ass...

Well, yeah, that data structure would need to change.

regards, tom lane

I wrote:
> After contemplating that for awhile, by far the least painful way
> seems to be to add the subLinkId to struct SubPlan. We can get
> away with that in the back branches as long as we add it at the
> end, since SubPlans don't get recorded in the catalogs.

The attached seems to fix it. While writing a test case I discovered
that the code I'd stolen from ruleutils.c is inadequate for the purpose.
That code was only designed to deal with pre-planning query trees,
so it only expects one Param per targetlist entry --- but cases like
multiple assignments to the same array column can get rolled up into
nests containing several Params (cf. rewriteTargetListIU). So I abandoned
that tactic in favor of just writing a tree walker. Andre's gut feeling
that we needed one was right.

regards, tom lane