Add soft error handling to some expression nodes

This adjusts the code for CoerceViaIO and CoerceToDomain expression
nodes to handle errors softly.

For CoerceViaIo, this adds a new ExprEvalStep opcode
EEOP_IOCOERCE_SAFE, which is implemented in the new accompanying
function ExecEvalCoerceViaIOSafe().  The only difference from
EEOP_IOCOERCE's inline implementation is that the input function
receives an ErrorSaveContext via the function's
FunctionCallInfo.context, which it can use to handle errors softly.

For CoerceToDomain, this simply entails replacing the ereport() in
ExecEvalConstraintNotNull() and ExecEvalConstraintCheck() by
errsave() passing it the ErrorSaveContext passed in the expression's
ExprEvalStep.

In both cases, the ErrorSaveContext to be used is passed by setting
ExprState.escontext to point to it before calling ExecInitExprRec()
on the expression tree whose errors are to be handled softly.

Note that there's no functional change as of this commit as no call
site of ExecInitExprRec() has been changed.  This is intended for
implementing new SQL/JSON expression nodes in future commits.

Extracted from a much larger patch to add SQL/JSON query functions.

Author: Nikita Glukhov <n.gluhov@postgrespro.ru>
Author: Teodor Sigaev <teodor@sigaev.ru>
Author: Oleg Bartunov <obartunov@gmail.com>
Author: Alexander Korotkov <aekorotkov@gmail.com>
Author: Andrew Dunstan <andrew@dunslane.net>
Author: Amit Langote <amitlangote09@gmail.com>

Reviewers have included (in no particular order) Andres Freund,
Alexander Korotkov, Pavel Stehule, Andrew Alsup, Erik Rijkers,
Zihong Yu, Himanshu Upadhyaya, Daniel Gustafsson, Justin Pryzby,
Álvaro Herrera, Jian He, Peter Eisentraut

Discussion: https://postgr.es/m/cd0bb935-0158-78a7-08b5-904886deac4b@postgrespro.ru
Discussion: https://postgr.es/m/20220616233130.rparivafipt6doj3@alap3.anarazel.de
Discussion: https://postgr.es/m/abd9b83b-aa66-f230-3d6d-734817f0995d%40postgresql.org
Discussion: https://postgr.es/m/CA+HiwqHROpf9e644D8BRqYvaAPmgBZVup-xKMDPk-nd4EpgzHw@mail.gmail.com
Discussion: https://postgr.es/m/CA+HiwqE4XTdfb1nW=Ojoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg@mail.gmail.com

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 91df2009bee7b5ff33712650086ecef88d8ca8c4..3181b1136a22472554344411c49b749f68443453 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -1560,7 +1560,10 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                 * We don't check permissions here as a type's input/output
                                 * function are assumed to be executable by everyone.
                                 */
-                               scratch.opcode = EEOP_IOCOERCE;
+                               if (state->escontext == NULL)
+                                       scratch.opcode = EEOP_IOCOERCE;
+                               else
+                                       scratch.opcode = EEOP_IOCOERCE_SAFE;
 
                                /* lookup the source type's output function */
                                scratch.d.iocoerce.finfo_out = palloc0(sizeof(FmgrInfo));
@@ -1596,6 +1599,8 @@ ExecInitExprRec(Expr *node, ExprState *state,
                                fcinfo_in->args[2].value = Int32GetDatum(-1);
                                fcinfo_in->args[2].isnull = false;
 
+                               fcinfo_in->context = (Node *) state->escontext;
+
                                ExprEvalPushStep(state, &scratch);
                                break;
                        }
@@ -3303,6 +3308,7 @@ ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest,
        /* we'll allocate workspace only if needed */
        scratch->d.domaincheck.checkvalue = NULL;
        scratch->d.domaincheck.checknull = NULL;
+       scratch->d.domaincheck.escontext = state->escontext;
 
        /*
         * Evaluate argument - it's fine to directly store it into resv/resnull,

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 3c17cc6b1e17d603f309b106b99ee0cf38689d43..3f20f1dd31484b2335307e9aed1426082826d335 100644 (file)
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -63,6 +63,7 @@
 #include "executor/nodeSubplan.h"
 #include "funcapi.h"
 #include "miscadmin.h"
+#include "nodes/miscnodes.h"
 #include "nodes/nodeFuncs.h"
 #include "parser/parsetree.h"
 #include "pgstat.h"
@@ -452,6 +453,7 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
                &&CASE_EEOP_CASE_TESTVAL,
                &&CASE_EEOP_MAKE_READONLY,
                &&CASE_EEOP_IOCOERCE,
+               &&CASE_EEOP_IOCOERCE_SAFE,
                &&CASE_EEOP_DISTINCT,
                &&CASE_EEOP_NOT_DISTINCT,
                &&CASE_EEOP_NULLIF,
@@ -1150,6 +1152,9 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
                         * Evaluate a CoerceViaIO node.  This can be quite a hot path, so
                         * inline as much work as possible.  The source value is in our
                         * result variable.
+                        *
+                        * Also look at ExecEvalCoerceViaIOSafe() if you change anything
+                        * here.
                         */
                        char       *str;
 
@@ -1205,6 +1210,12 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
                        EEO_NEXT();
                }
 
+               EEO_CASE(EEOP_IOCOERCE_SAFE)
+               {
+                       ExecEvalCoerceViaIOSafe(state, op);
+                       EEO_NEXT();
+               }
+
                EEO_CASE(EEOP_DISTINCT)
                {
                        /*
@@ -2510,6 +2521,71 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
                         errmsg("no value found for parameter %d", paramId)));
 }
 
+/*
+ * Evaluate a CoerceViaIO node in soft-error mode.
+ *
+ * The source value is in op's result variable.
+ *
+ * Note: This implements EEOP_IOCOERCE_SAFE. If you change anything here,
+ * also look at the inline code for EEOP_IOCOERCE.
+ */
+void
+ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op)
+{
+       char       *str;
+
+       /* call output function (similar to OutputFunctionCall) */
+       if (*op->resnull)
+       {
+               /* output functions are not called on nulls */
+               str = NULL;
+       }
+       else
+       {
+               FunctionCallInfo fcinfo_out;
+
+               fcinfo_out = op->d.iocoerce.fcinfo_data_out;
+               fcinfo_out->args[0].value = *op->resvalue;
+               fcinfo_out->args[0].isnull = false;
+
+               fcinfo_out->isnull = false;
+               str = DatumGetCString(FunctionCallInvoke(fcinfo_out));
+
+               /* OutputFunctionCall assumes result isn't null */
+               Assert(!fcinfo_out->isnull);
+       }
+
+       /* call input function (similar to InputFunctionCallSafe) */
+       if (!op->d.iocoerce.finfo_in->fn_strict || str != NULL)
+       {
+               FunctionCallInfo fcinfo_in;
+
+               fcinfo_in = op->d.iocoerce.fcinfo_data_in;
+               fcinfo_in->args[0].value = PointerGetDatum(str);
+               fcinfo_in->args[0].isnull = *op->resnull;
+               /* second and third arguments are already set up */
+
+               /* ErrorSaveContext must be present. */
+               Assert(IsA(fcinfo_in->context, ErrorSaveContext));
+
+               fcinfo_in->isnull = false;
+               *op->resvalue = FunctionCallInvoke(fcinfo_in);
+
+               if (SOFT_ERROR_OCCURRED(fcinfo_in->context))
+               {
+                       *op->resnull = true;
+                       *op->resvalue = (Datum) 0;
+                       return;
+               }
+
+               /* Should get null result if and only if str is NULL */
+               if (str == NULL)
+                       Assert(*op->resnull);
+               else
+                       Assert(!*op->resnull);
+       }
+}
+
 /*
  * Evaluate a SQLValueFunction expression.
  */
@@ -3730,7 +3806,7 @@ void
 ExecEvalConstraintNotNull(ExprState *state, ExprEvalStep *op)
 {
        if (*op->resnull)
-               ereport(ERROR,
+               errsave((Node *) op->d.domaincheck.escontext,
                                (errcode(ERRCODE_NOT_NULL_VIOLATION),
                                 errmsg("domain %s does not allow null values",
                                                format_type_be(op->d.domaincheck.resulttype)),
@@ -3745,7 +3821,7 @@ ExecEvalConstraintCheck(ExprState *state, ExprEvalStep *op)
 {
        if (!*op->d.domaincheck.checknull &&
                !DatumGetBool(*op->d.domaincheck.checkvalue))
-               ereport(ERROR,
+               errsave((Node *) op->d.domaincheck.escontext,
                                (errcode(ERRCODE_CHECK_VIOLATION),
                                 errmsg("value for domain %s violates check constraint \"%s\"",
                                                format_type_be(op->d.domaincheck.resulttype),

diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index 33161d812f27fdf95b824c23d0214b942101c00b..09994503b15b07ce8f65bf26503d719187e651d1 100644 (file)
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -1431,6 +1431,12 @@ llvm_compile_expr(ExprState *state)
                                        break;
                                }
 
+                       case EEOP_IOCOERCE_SAFE:
+                               build_EvalXFunc(b, mod, "ExecEvalCoerceViaIOSafe",
+                                                               v_state, op);
+                               LLVMBuildBr(b, opblocks[opno + 1]);
+                               break;
+
                        case EEOP_DISTINCT:
                        case EEOP_NOT_DISTINCT:
                                {

diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index 5212f529c84150d5d0996cf2d10e015065077229..47c9daf40230bb76af301109429711427e653c80 100644 (file)
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -162,6 +162,7 @@ void           *referenced_functions[] =
        ExecEvalRow,
        ExecEvalRowNotNull,
        ExecEvalRowNull,
+       ExecEvalCoerceViaIOSafe,
        ExecEvalSQLValueFunction,
        ExecEvalScalarArrayOp,
        ExecEvalHashedScalarArrayOp,

diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index a20c539a25304799dcec3057b1f1546db3a47e2d..a28ddcdd771472f3406691583e529f2a18e3e827 100644 (file)
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -16,6 +16,7 @@
 
 #include "executor/nodeAgg.h"
 #include "nodes/execnodes.h"
+#include "nodes/miscnodes.h"
 
 /* forward references to avoid circularity */
 struct ExprEvalStep;
@@ -168,6 +169,7 @@ typedef enum ExprEvalOp
 
        /* evaluate assorted special-purpose expression types */
        EEOP_IOCOERCE,
+       EEOP_IOCOERCE_SAFE,
        EEOP_DISTINCT,
        EEOP_NOT_DISTINCT,
        EEOP_NULLIF,
@@ -547,6 +549,7 @@ typedef struct ExprEvalStep
                        bool       *checknull;
                        /* OID of domain type */
                        Oid                     resulttype;
+                       ErrorSaveContext *escontext;
                }                       domaincheck;
 
                /* for EEOP_CONVERT_ROWTYPE */
@@ -776,6 +779,7 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
                                                          ExprContext *econtext);
 extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
                                                                ExprContext *econtext);
+extern void ExecEvalCoerceViaIOSafe(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalCurrentOfExpr(ExprState *state, ExprEvalStep *op);
 extern void ExecEvalNextValueExpr(ExprState *state, ExprEvalStep *op);

diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index 561fdd98f1b56fb23e167ade3ddaab38405bc3f4..444a5f0fd5786a0b213e7d8905b1bb786a4f7989 100644 (file)
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -34,6 +34,7 @@
 #include "fmgr.h"
 #include "lib/ilist.h"
 #include "lib/pairingheap.h"
+#include "nodes/miscnodes.h"
 #include "nodes/params.h"
 #include "nodes/plannodes.h"
 #include "nodes/tidbitmap.h"
@@ -129,6 +130,12 @@ typedef struct ExprState
 
        Datum      *innermost_domainval;
        bool       *innermost_domainnull;
+
+       /*
+        * For expression nodes that support soft errors.  Should be set to NULL
+        * before calling ExecInitExprRec() if the caller wants errors thrown.
+        */
+       ErrorSaveContext *escontext;
 } ExprState;