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
Date: Sat, 23 Nov 2024 20:42:00 -0500
Message-ID: <940942.1732412520@sss.pgh.pa.us> (raw)
In-Reply-To: <18722-fd9e645448cc78b4@postgresql.org>
References: <18722-fd9e645448cc78b4@postgresql.org>
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
Attachments:bug-18722-fix-wip.patch (text/x-diff, 2.6K) downloaddiff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.cindex 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 =+ (get_typlen(exprType((Node *) linitial(op->args))) == -1);+
/*
* Change opcode of call instruction to EEOP_NULLIF.
*
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.cindex 30c5a19aad..42da962178 100644--- a/src/backend/executor/execExprInterp.c+++ b/src/backend/executor/execExprInterp.c@@ -1308,12 +1308,24 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
* The arguments are already evaluated into fcinfo->args.
*/
FunctionCallInfo fcinfo = op->d.func.fcinfo_data;
+ Datum save_arg0 = 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 =+ MakeExpandedObjectReadOnlyInternal(save_arg0);+
fcinfo->isnull = false;
result = op->d.func.fn_addr(fcinfo);
@@ -1328,7 +1340,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
}
/* Arguments aren't equal, so return the first one */
- *op->resvalue = fcinfo->args[0].value;+ *op->resvalue = save_arg0;
*op->resnull = fcinfo->args[0].isnull;
EEO_NEXT();
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.hindex 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 */
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-bugs@postgresql.org
Cc: tgl@sss.pgh.pa.us, exclusion@gmail.com, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #18722: Processing arrays with plpgsql raises errors
In-Reply-To: <940942.1732412520@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox