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: Sun, 24 Nov 2024 17:49:12 -0500
Message-ID: <1211277.1732488552@sss.pgh.pa.us> (raw)
In-Reply-To: <940942.1732412520@sss.pgh.pa.us>
References: <18722-fd9e645448cc78b4@postgresql.org>
<940942.1732412520@sss.pgh.pa.us>
I wrote:
> 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.
Here's a fleshed-out patch with a test case and JIT support. This
is about the first time I've messed with LLVM, so I wouldn't mind
some review of what I did in llvmjit_expr.c. In particular, do I
correctly understand that "l_funcvalue(b, v_fcinfo, 0)" produces
a reference to a copy of the initial value of args[0].value?
It seems to work in testing --- the original R/W pointer is returned
out of the expression --- but if we returned the R/O pointer instead
it'd be mighty difficult to detect the resulting inefficiency.
> I'm wondering now if any of our other conditional expressions have
> similar bugs ...
I was amused to discover that case.sql already had infrastructure
suitable for testing this, thanks to bug #14472.
regards, tom lane
Attachments:bug-18722-fix-v1.patch (text/x-diff, 5.9K) 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/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.cindex 9cec17544b..c533f55254 100644--- a/src/backend/jit/llvm/llvmjit_expr.c+++ b/src/backend/jit/llvm/llvmjit_expr.c@@ -1559,6 +1559,9 @@ llvm_compile_expr(ExprState *state)
v_fcinfo = l_ptr_const(fcinfo, l_ptr(StructFunctionCallInfoData));
+ /* save original arg[0] */+ v_arg0 = l_funcvalue(b, v_fcinfo, 0);+
/* if either argument is NULL they can't be equal */
v_argnull0 = l_funcnull(b, v_fcinfo, 0);
v_argnull1 = l_funcnull(b, v_fcinfo, 1);
@@ -1575,7 +1578,6 @@ llvm_compile_expr(ExprState *state)
/* one (or both) of the arguments are null, return arg[0] */
LLVMPositionBuilderAtEnd(b, b_hasnull);
- v_arg0 = l_funcvalue(b, v_fcinfo, 0);
LLVMBuildStore(b, v_argnull0, v_resnullp);
LLVMBuildStore(b, v_arg0, v_resvaluep);
LLVMBuildBr(b, opblocks[opno + 1]);
@@ -1583,12 +1585,35 @@ llvm_compile_expr(ExprState *state)
/* build block to invoke function and check result */
LLVMPositionBuilderAtEnd(b, b_nonull);
+ /*+ * 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)+ {+ LLVMValueRef v_params[1];+ LLVMValueRef v_arg0_ro;++ v_params[0] = v_arg0;+ v_arg0_ro =+ l_call(b,+ llvm_pg_var_func_type("MakeExpandedObjectReadOnlyInternal"),+ llvm_pg_func(mod, "MakeExpandedObjectReadOnlyInternal"),+ v_params, lengthof(v_params), "");+ LLVMBuildStore(b, v_arg0_ro,+ l_funcvaluep(b, v_fcinfo, 0));+ }+
v_retval = BuildV1Call(context, b, mod, fcinfo, &v_fcinfo_isnull);
/*
- * If result not null, and arguments are equal return null- * (same result as if there'd been NULLs, hence reuse- * b_hasnull).+ * If result not null and arguments are equal return null,+ * else return arg[0] (same result as if there'd been+ * NULLs, hence reuse b_hasnull).
*/
v_argsequal = LLVMBuildAnd(b,
LLVMBuildICmp(b, LLVMIntEQ,
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 */
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.outindex f5136c17ab..efee7fc431 100644--- a/src/test/regress/expected/case.out+++ b/src/test/regress/expected/case.out@@ -397,6 +397,14 @@ SELECT CASE make_ad(1,2)
right
(1 row)
+-- While we're here, also test handling of a NULLIF arg that is a read/write+-- object (bug #18722)+SELECT NULLIF(make_ad(1,2), array[2,3]::arrdomain);+ nullif +--------+ {1,2}+(1 row)+
ROLLBACK;
-- Test interaction of CASE with ArrayCoerceExpr (bug #15471)
BEGIN;
diff --git a/src/test/regress/sql/case.sql b/src/test/regress/sql/case.sqlindex 83fe43be6b..388d4c6f52 100644--- a/src/test/regress/sql/case.sql+++ b/src/test/regress/sql/case.sql@@ -242,6 +242,11 @@ SELECT CASE make_ad(1,2)
WHEN array[1,2]::arrdomain THEN 'right'
END;
+-- While we're here, also test handling of a NULLIF arg that is a read/write+-- object (bug #18722)++SELECT NULLIF(make_ad(1,2), array[2,3]::arrdomain);+
ROLLBACK;
-- Test interaction of CASE with ArrayCoerceExpr (bug #15471)
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: <1211277.1732488552@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