From: Tom Lane <tgl@sss.pgh.pa.us>
To: luis.roberto@siscobra.com.br
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>
Subject: Re: segfault with incremental sort
In-reply-to: 
 <1779929796.39206557.1604407053112.JavaMail.zimbra@siscobra.com.br>
References: <622580997.37108180.1604080457319.JavaMail.zimbra@siscobra.com.br>
 <102089.1604082122@sss.pgh.pa.us>
 <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>
Comments: In-reply-to luis.roberto@siscobra.com.br
	message dated "Tue, 03 Nov 2020 09:37:33 -0300"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <924084.1604422138.0@sss.pgh.pa.us>
Date: Tue, 03 Nov 2020 11:52:06 -0500
Message-ID: <924226.1604422326@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <924084.1604422138.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

luis.roberto@siscobra.com.br writes:
> It took me some time to make it the same... I managed to simplify the er=
ror. It appears to be something related to a subplan with a distinct claus=
e and another subplan... =


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

[1] https://www.postgresql.org/message-id/flat/CAJGNTeNaxpXgBVcRhJX%2B2vSb=
q%2BF2kJqGBcvompmpvXb7pq%2BoFA%40mail.gmail.com


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="more-paranoia-in-ExecInitSubPlan.patch";
	charset="us-ascii"
Content-ID: <924084.1604422138.2@sss.pgh.pa.us>
Content-Description: more-paranoia-in-ExecInitSubPlan.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/executor/nodeSubplan.c b/src/backend/executor/nod=
eSubplan.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 =3D (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 =3D=3D NULL)
+		elog(ERROR, "subplan \"%s\" was not initialized",
+			 subplan->plan_name);
+
+	/* Link to parent's state, too */
 	sstate->parent =3D parent;
 =

 	/* Initialize subexpressions */

------- =_aaaaaaaaaa0--