Date: Tue, 3 Nov 2020 14:06:08 -0300 (BRT)
From: luis.roberto@siscobra.com.br
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-bugs <pgsql-bugs@lists.postgresql.org>,
	"alan.formagi" <alan.formagi@siscobra.com.br>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	James Coleman <jtc331@gmail.com>
Message-ID: 
 <1332064613.40286998.1604423168236.JavaMail.zimbra@siscobra.com.br>
In-Reply-To: <924226.1604422326@sss.pgh.pa.us>
References: <622580997.37108180.1604080457319.JavaMail.zimbra@siscobra.com.br>
 <541187747.37582276.1604086719237.JavaMail.zimbra@siscobra.com.br>
 <107029.1604087483@sss.pgh.pa.us>
 <1673261949.37800156.1604090102715.JavaMail.zimbra@siscobra.com.br>
 <1807748588.37862034.1604091799640.JavaMail.zimbra@siscobra.com.br>
 <136656.1604092024@sss.pgh.pa.us>
 <1779929796.39206557.1604407053112.JavaMail.zimbra@siscobra.com.br>
 <924226.1604422326@sss.pgh.pa.us>
Subject: Re: segfault with incremental sort
MIME-Version: 1.0
Content-Type: multipart/alternative;
	boundary="=_52efb5da-6e0d-481e-81f3-c059b8bf550c"
Thread-Topic: segfault with incremental sort
Thread-Index: k39v9e8WFPXK0qtBEHcGyVqFBW0Asg==
Precedence: bulk

--=_52efb5da-6e0d-481e-81f3-c059b8bf550c
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


Thanks for the test case! It looks like this issue is somewhat related 
to the "enable_incremental_sort changes query behavior" thread [1]. 
What I see happening is 

1. The SELECT DISTINCT gives rise to a sort key expression that 
contains non-parallel-safe SubPlans. (It's not immediately apparent 
to me why we don't consider these particular subqueries parallel safe, 
but they aren't. Anyway such a situation surely has to be allowed for.) 

2. The planner ignores the fact that the sort key isn't parallel-safe 
and makes a plan with IncrementalSort below Gather anyway. (I'm not 
certain that this bug is specific to IncrementalSort; but given the lack 
of previous complaints, I'm guessing we avoid this somehow for regular 
sorts.) 

3. ExecSerializePlan notes that the subplans aren't parallel-safe and 
doesn't send them to the workers. 

4. In the workers, nodeSubplan.c dumps core because the planstate it's 
expecting is not there. 

So, not only do we need to be thinking about volatility while checking 
whether IncrementalSort is possible, but also parallel-safety. 

In the meantime, now that I've seen this I don't have a lot of confidence 
that we'll never inject similar bugs in future. I'm thinking of 
committing the attached to at least reduce the stakes from "core dump" 
to "weird error message". 

regards, tom lane 


Thanks for confirming it. For now, disabling incremental sort will 'solve' the issue. I'll be keeping an eye on that thread. 

--=_52efb5da-6e0d-481e-81f3-c059b8bf550c
Content-Type: text/html; charset=utf-8
Content-Transfer-Encoding: quoted-printable

<html><body><div style=3D"font-family: arial, helvetica, sans-serif; font-s=
ize: 11pt; color: #000000"><blockquote><div data-marker=3D"__QUOTED_TEXT__"=
>Thanks for the test case! &nbsp;It looks like this issue is somewhat relat=
ed<br>to the "enable_incremental_sort changes query behavior" thread [1].<b=
r>What I see happening is<br><br>1. The SELECT DISTINCT gives rise to a sor=
t key expression that<br>contains non-parallel-safe SubPlans. &nbsp;(It's n=
ot immediately apparent<br>to me why we don't consider these particular sub=
queries parallel safe,<br>but they aren't. &nbsp;Anyway such a situation su=
rely has to be allowed for.)<br><br>2. The planner ignores the fact that th=
e sort key isn't parallel-safe<br>and makes a plan with IncrementalSort bel=
ow Gather anyway. &nbsp;(I'm not<br>certain that this bug is specific to In=
crementalSort; but given the lack<br>of previous complaints, I'm guessing w=
e avoid this somehow for regular<br>sorts.)<br><br>3. ExecSerializePlan not=
es that the subplans aren't parallel-safe and<br>doesn't send them to the w=
orkers.<br><br>4. In the workers, nodeSubplan.c dumps core because the plan=
state it's<br>expecting is not there.<br><br>So, not only do we need to be =
thinking about volatility while checking<br>whether IncrementalSort is poss=
ible, but also parallel-safety.<br><br>In the meantime, now that I've seen =
this I don't have a lot of confidence<br>that we'll never inject similar bu=
gs in future. &nbsp;I'm thinking of<br>committing the attached to at least =
reduce the stakes from "core dump"<br>to "weird error message".<br><br>&nbs=
p;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&=
nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;regards, t=
om lane<br><br></div></blockquote><div data-marker=3D"__QUOTED_TEXT__">Than=
ks for confirming it. For now, disabling incremental sort will 'solve' the =
issue. I'll be keeping an eye on that thread.&nbsp;</div></div></body></htm=
l>
--=_52efb5da-6e0d-481e-81f3-c059b8bf550c
Content-Type: text/x-diff; charset=utf-8
Content-Transfer-Encoding: 7bit

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nodeSubplan.c
index 9a706df5f0..152c7ae7eb 100644
--- a/src/backend/executor/nodeSubplan.c
+++ b/src/backend/executor/nodeSubplan.c
@@ -797,7 +797,15 @@ ExecInitSubPlan(SubPlan *subplan, PlanState *parent)
 	sstate->planstate = (PlanState *) list_nth(estate->es_subplanstates,
 											   subplan->plan_id - 1);
 
-	/* ... and to its parent's state */
+	/*
+	 * This check can fail if the planner mistakenly puts a parallel-unsafe
+	 * subplan into a parallelized subquery; see ExecSerializePlan.
+	 */
+	if (sstate->planstate == NULL)
+		elog(ERROR, "subplan \"%s\" was not initialized",
+			 subplan->plan_name);
+
+	/* Link to parent's state, too */
 	sstate->parent = parent;
 
 	/* Initialize subexpressions */

--=_52efb5da-6e0d-481e-81f3-c059b8bf550c--