From: Tom Lane <tgl@sss.pgh.pa.us>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
cc: bernddorn@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #17227: segmentation fault with jsonb_to_recordset
In-reply-to: <676464.1634158492@sss.pgh.pa.us>
References: <202110131359.a6m35hpudkg7@alvherre.pgsql>
 <649678.1634135335@sss.pgh.pa.us> <675396.1634157283@sss.pgh.pa.us>
 <676464.1634158492@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 13 Oct 2021 16:54:52 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <709376.1634165860.0@sss.pgh.pa.us>
Date: Wed, 13 Oct 2021 18:58:10 -0400
Message-ID: <709405.1634165890@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/709405.1634165890%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <709376.1634165860.1@sss.pgh.pa.us>

I wrote:
> Apparently this idea failed to account for subexpressions pulled up
> during later processing.  Back to the drawing board.

After further study of 7266d0997, it seems we could fix this by
the expedient of repeating eval_const_expressions on a function
RTE whenever it contains LATERAL references.  The code already
understood that lateral join aliases were trouble, but it didn't
account for the case of subexpressions inserted by subquery
pullup (which'd have to have been lateral refs originally).

The whole thing makes me itch a little bit, because this logic is
assuming that eval_const_expressions is idempotent, something
we can't test very well and (AFAIR) are not assuming anywhere else.
If somebody made some sub-case not idempotent, we'd not notice until
that case got used in a lateral function RTE.  That could be a long
time, as evidenced by the fact that this bug went undetected for a
year since v13 release.  However, we had that assumption in the code
7266d0997 replaced too, so this isn't adding any real new risk.

I looked briefly at whether the planner's preprocessing steps
could be re-ordered to eliminate the need for sometimes reprocessing
function RTEs.  It looks like it'd be a mess though, a conclusion
I think I also reached before committing 7266d0997.

Anyway, unless somebody has a better idea, I'll flesh out the
attached with a test case and push it.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="lateral-function-pullup-bug-wip.patch";
	charset="us-ascii"
Content-ID: <709376.1634165860.2@sss.pgh.pa.us>
Content-Description: lateral-function-pullup-bug-wip.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/=
plan/planner.c
index 1e42d75465..56722616a3 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -1085,7 +1085,8 @@ preprocess_expression(PlannerInfo *root, Node *expr,=
 int kind)
 	/*
 	 * Simplify constant expressions.  For function RTEs, this was already
 	 * done by preprocess_function_rtes ... but we have to do it again if th=
e
-	 * RTE is LATERAL and might have contained join alias variables.
+	 * RTE is LATERAL, as it might contain un-simplified subexpressions
+	 * inserted by flattening of subqueries or join alias variables.
 	 *
 	 * Note: an essential effect of this is to convert named-argument functi=
on
 	 * calls to positional notation and insert the current actual values of
@@ -1099,8 +1100,7 @@ preprocess_expression(PlannerInfo *root, Node *expr,=
 int kind)
 	 * careful to maintain AND/OR flatness --- that is, do not generate a tr=
ee
 	 * with AND directly under AND, nor OR directly under OR.
 	 */
-	if (!(kind =3D=3D EXPRKIND_RTFUNC ||
-		  (kind =3D=3D EXPRKIND_RTFUNC_LATERAL && !root->hasJoinRTEs)))
+	if (kind !=3D EXPRKIND_RTFUNC)
 		expr =3D eval_const_expressions(root, expr);
 =

 	/*

------- =_aaaaaaaaaa0--