MIME-Version: 1.0
References: <1129530.1681317832@sss.pgh.pa.us>
 <CAMbWs4_7hFTDjYzJs6sYpceY_0MFRA+DhdKxAMOREP084vr2pA@mail.gmail.com>
 <1315906.1681394451@sss.pgh.pa.us>
 <CAMbWs48p-WpnLdR9ZQ4QsHZP_a-P0rktAYo4Z3uOHUAkH3fjQg@mail.gmail.com>
In-Reply-To: 
 <CAMbWs48p-WpnLdR9ZQ4QsHZP_a-P0rktAYo4Z3uOHUAkH3fjQg@mail.gmail.com>
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 17 Apr 2023 15:26:46 +0800
Message-ID: 
 <CAMbWs49RCcbqjwuiEobCsE4MU9qB0Me+dextApkwCmXe1GZroA@mail.gmail.com>
Subject: Re: Allowing parallel-safe initplans
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: multipart/alternative; boundary="000000000000b2925405f9831db8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAMbWs49RCcbqjwuiEobCsE4MU9qB0Me%2BdextApkwCmXe1GZroA%40mail.gmail.com>
Precedence: bulk

--000000000000b2925405f9831db8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Apr 17, 2023 at 10:57=E2=80=AFAM Richard Guo <guofenglinux@gmail.co=
m> wrote:

> The initPlan has been moved from the Result node to the Gather node.  As
> a result, when doing tuple projection for the Result node, we'd get a
> ParamExecData entry with NULL execPlan.  So the initPlan does not get
> chance to be executed.  And we'd get the output as the default value
> from the ParamExecData entry, which is zero as shown.
>
> So now I begin to wonder if this wrong result issue is possible to exist
> in other places where we move initPlans.  But I haven't tried hard to
> verify that.
>

I looked further into this issue and I believe other places are good.
The problem with this query is that the es/ecxt_param_exec_vals used to
store info about the initplan is not the same one as in the Result
node's expression context for projection, because we've forked a new
process for the parallel worker and then created and initialized a new
EState node, and allocated a new es_param_exec_vals array for the new
EState.  When doing projection for the Result node, the current code
just goes ahead and accesses the new es_param_exec_vals, thus fails to
retrieve the info about the initplan.  Hmm, I doubt this is sensible.

So now it seems that the breakage of regression tests is more severe
than being cosmetic.  I wonder if we need to update the comments to
indicate the potential wrong results issue if we move the initPlans to
the Gather node.

Thanks
Richard

--000000000000b2925405f9831db8
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><div class=3D"gmail_quote"><div=
 dir=3D"ltr" class=3D"gmail_attr">On Mon, Apr 17, 2023 at 10:57=E2=80=AFAM =
Richard Guo &lt;<a href=3D"mailto:guofenglinux@gmail.com">guofenglinux@gmai=
l.com</a>&gt; wrote:</div><blockquote class=3D"gmail_quote" style=3D"margin=
:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"=
><div dir=3D"ltr"><div class=3D"gmail_quote"><div>The initPlan has been mov=
ed from the Result node to the Gather node.=C2=A0 As<br>a result, when doin=
g tuple projection for the Result node, we&#39;d get a<br>ParamExecData ent=
ry with NULL execPlan.=C2=A0 So the initPlan does not get<br>chance to be e=
xecuted.=C2=A0 And we&#39;d get the output as the default value<br>from the=
 ParamExecData entry, which is zero as shown.<br><br>So now I begin to wond=
er if this wrong result issue is possible to exist<br>in other places where=
 we move initPlans.=C2=A0 But I haven&#39;t tried hard to<br>verify that.</=
div></div></div></blockquote><div><br></div><div>I looked further into this=
 issue and I believe other places are good.<br>The problem with this query =
is that the es/ecxt_param_exec_vals used to<br>store info about the initpla=
n is not the same one as in the Result<br>node&#39;s expression context for=
 projection, because we&#39;ve forked a new<br>process for the parallel wor=
ker and then created and initialized a new<br>EState node, and allocated a =
new es_param_exec_vals array for the new<br>EState.=C2=A0 When doing projec=
tion for the Result node, the current code<br>just goes ahead and accesses =
the new es_param_exec_vals, thus fails to<br>retrieve the info about the in=
itplan.=C2=A0 Hmm, I doubt this is sensible.<br><br>So now it seems that th=
e breakage of regression tests is more severe<br>than being cosmetic.=C2=A0=
 I wonder if we need to update the comments to<br>indicate the potential wr=
ong results issue if we move the initPlans to<br>the Gather node.<br><br>Th=
anks<br>Richard<br></div></div></div>

--000000000000b2925405f9831db8--