From: Tom Lane <tgl@sss.pgh.pa.us>
To: exclusion@gmail.com
cc: pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18722: Processing arrays with plpgsql raises errors
In-reply-to: <18722-fd9e645448cc78b4@postgresql.org>
References: <18722-fd9e645448cc78b4@postgresql.org>
Comments: In-reply-to PG Bug reporting form <noreply@postgresql.org>
	message dated "Sat, 23 Nov 2024 10:00:01 +0000"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <940891.1732412489.0@sss.pgh.pa.us>
Date: Sat, 23 Nov 2024 20:42:00 -0500
Message-ID: <940942.1732412520@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/940942.1732412520%40sss.pgh.pa.us>
Precedence: bulk

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

PG Bug reporting form <noreply@postgresql.org> writes:
> The following script:
> CREATE FUNCTION make_ia() RETURNS int[] LANGUAGE plpgsql AS
>   'declare x int[]; begin x := array[0]; return x; end';
> CREATE FUNCTION ia_eq(int[], int[]) RETURNS boolean LANGUAGE plpgsql AS
> 'begin return array_eq($1, $2); end'; 
> CREATE OPERATOR = (procedure = ia_eq, leftarg = int[], rightarg = int[]);
> SELECT NULLIF(make_ia(), array[1]::int[]);

> fails with:
> ERROR:  cache lookup failed for type 2139062143

Nice catch!  What is happening here is that make_ia returns a
read/write pointer to an expanded array object.  The EEOP_NULLIF
code passes that pointer straight on to the equality function.
Which in this case is a plpgsql function that will suppose it
can take ownership of the expanded object, resulting in said
object being freed before return.  (Neither function has done
anything wrong.)  The problem is that EEOP_NULLIF then returns
the original Datum pointer, which is now pointing at garbage.
The different failures you get depending on what is done next
with the Datum are not too surprising.

What EEOP_NULLIF needs to do is pass a read-only pointer to the
equality function, so that the object is not modified and remains
available to return if we want to do so.

Attached is a quick WIP patch to handle that.  It is missing a test
case, but the real omission is that llvm_compile_expr()'s EEOP_NULLIF
handling also needs to be fixed, and I'm pretty unsure how to do that.

I'm wondering now if any of our other conditional expressions have
similar bugs ...

			regards, tom lane


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

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execEx=
pr.c
index 8f7a534005..69d36f70b3 100644
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1191,6 +1191,14 @@ ExecInitExprRec(Expr *node, ExprState *state,
 							 op->args, op->opfuncid, op->inputcollid,
 							 state);
 =

+				/*
+				 * If first argument is of varlena type, we'll need to ensure
+				 * that the value passed to the comparison function is a
+				 * read-only pointer.
+				 */
+				scratch.d.func.make_ro =3D
+					(get_typlen(exprType((Node *) linitial(op->args))) =3D=3D -1);
+
 				/*
 				 * Change opcode of call instruction to EEOP_NULLIF.
 				 *
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/=
execExprInterp.c
index 30c5a19aad..42da962178 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -1308,12 +1308,24 @@ ExecInterpExpr(ExprState *state, ExprContext *econ=
text, bool *isnull)
 			 * The arguments are already evaluated into fcinfo->args.
 			 */
 			FunctionCallInfo fcinfo =3D op->d.func.fcinfo_data;
+			Datum		save_arg0 =3D fcinfo->args[0].value;
 =

 			/* if either argument is NULL they can't be equal */
 			if (!fcinfo->args[0].isnull && !fcinfo->args[1].isnull)
 			{
 				Datum		result;
 =

+				/*
+				 * If first argument is of varlena type, it might be an
+				 * expanded datum.  We need to ensure that the value passed to
+				 * the comparison function is a read-only pointer.  However,
+				 * if we end by returning the first argument, that will be the
+				 * original read-write pointer if it was read-write.
+				 */
+				if (op->d.func.make_ro)
+					fcinfo->args[0].value =3D
+						MakeExpandedObjectReadOnlyInternal(save_arg0);
+
 				fcinfo->isnull =3D false;
 				result =3D op->d.func.fn_addr(fcinfo);
 =

@@ -1328,7 +1340,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econte=
xt, bool *isnull)
 			}
 =

 			/* Arguments aren't equal, so return the first one */
-			*op->resvalue =3D fcinfo->args[0].value;
+			*op->resvalue =3D save_arg0;
 			*op->resnull =3D fcinfo->args[0].isnull;
 =

 			EEO_NEXT();
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execEx=
pr.h
index cd97dfa062..56fb0d0adb 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -365,6 +365,7 @@ typedef struct ExprEvalStep
 			/* faster to access without additional indirection: */
 			PGFunction	fn_addr;	/* actual call address */
 			int			nargs;	/* number of arguments */
+			bool		make_ro;	/* make arg0 R/O (used only for NULLIF) */
 		}			func;
 =

 		/* for EEOP_BOOL_*_STEP */

------- =_aaaaaaaaaa0--