Re: BUG #18722: Processing arrays with plpgsql raises errors - Mailing list pgsql-bugs

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

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.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 =
+                    (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.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 *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.c
index 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.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 */
diff --git a/src/test/regress/expected/case.out b/src/test/regress/expected/case.out
index 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.sql
index 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)