From 8c2b0c25c47a60924345879a788434cde1cf4125 Mon Sep 17 00:00:00 2001 From: jian he Date: Mon, 26 Oct 2026 20:05:08 +0800 Subject: [PATCH v2 1/1] Fix SQL/JSON error handling after a soft error has occurred 1. Stop evaluating after a soft error Once a soft error has occurred, the result is going to be discarded, but evaluation continued on a dummy NULL. Checking a domain's constraints on that NULL is wrong: a CHECK expression could throw a hard error, and a NOT NULL check would overwrite the saved error. ExecInitCoerceToDomain now emits, before each constraint evaluation, an EEOP_JUMP_IF_NULL step based on the context's error_occurred flag that jumps past all remaining constraints. Likewise, json_populate_type() stops as soon as a soft error is reported: populate_domain() no longer checks the domain after the base type's conversion fails, and populate_record() no longer populates the remaining columns. And ExecEvalJsonCoercion() no longer coerces the dummy result of an ON ERROR or ON EMPTY expression that has already failed. 2. Always rethrow soft errors from ON ERROR and ON EMPTY expressions The DEFAULT expression of an SQL/JSON ON ERROR or ON EMPTY clause is always compiled in an error-safe manner, so that its errors can be rethrown later. Hence, unconditionally check whether evaluation of the preceding DEFAULT expression failed. Discussion: https://postgr.es/m/CACJufxHDK39zF+1zZF6prH_wtn1SDt74_mrqYk1Oo82cCu44ng@mail.gmail.com Commitfest: https://commitfest.postgresql.org/patch/7355 --- src/backend/executor/execExpr.c | 66 ++++++++++------- src/backend/executor/execExprInterp.c | 7 ++ src/backend/utils/adt/jsonfuncs.c | 15 ++++ .../regress/expected/sqljson_jsontable.out | 13 ++++ .../regress/expected/sqljson_queryfuncs.out | 71 +++++++++++++++++++ src/test/regress/sql/sqljson_jsontable.sql | 5 ++ src/test/regress/sql/sqljson_queryfuncs.sql | 26 +++++++ 7 files changed, 178 insertions(+), 25 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 3f063b79f4d..ba23d1046e0 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -3520,6 +3520,7 @@ ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest, Datum *domainval = NULL; bool *domainnull = NULL; ListCell *l; + List *skipjumps = NIL; scratch->d.domaincheck.resulttype = ctest->resulttype; /* we'll allocate workspace only if needed */ @@ -3570,6 +3571,24 @@ ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest, scratch->d.domaincheck.constraintname = con->name; + /* + * CoerceToDomain may be evaluated softly, so the argument or an + * earlier constraint may already have failed, leaving a dummy NULL. + * In that case skip the remaining constraints: a CHECK expression + * could throw a hard error on that NULL, and a NOT NULL constraint + * would report it as a violation, replacing the saved error. + */ + if (state->escontext != NULL) + { + ExprEvalStep jump = {0}; + + jump.opcode = EEOP_JUMP_IF_NULL; + jump.resnull = &state->escontext->error_occurred; + jump.d.jump.jumpdone = -1; /* set below */ + skipjumps = lappend_int(skipjumps, state->steps_len); + ExprEvalPushStep(state, &jump); + } + switch (con->constrainttype) { case DOM_CONSTRAINT_NOTNULL: @@ -3650,6 +3669,8 @@ ExecInitCoerceToDomain(ExprEvalStep *scratch, CoerceToDomain *ctest, break; } } + foreach_int(jumpno, skipjumps) + state->steps[jumpno].d.jump.jumpdone = state->steps_len; } /* @@ -4948,19 +4969,17 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, resv, resnull); /* - * Add a COERCION_FINISH step to check for errors that may occur when - * coercing and rethrow them. + * Add a COERCION_FINISH step to rethrow errors reported softly. This + * must be done unconditionally, since not only the coercion but also + * the ON ERROR expression itself is evaluated with escontext. + * + * XXX: maybe ExecInitJsonCoercion() should push this step itself. */ - if (jsexpr->on_error->coerce || - IsA(jsexpr->on_error->expr, CoerceViaIO) || - IsA(jsexpr->on_error->expr, CoerceToDomain)) - { - scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; - scratch->resvalue = resv; - scratch->resnull = resnull; - scratch->d.jsonexpr.jsestate = jsestate; - ExprEvalPushStep(state, scratch); - } + scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; + scratch->resvalue = resv; + scratch->resnull = resnull; + scratch->d.jsonexpr.jsestate = jsestate; + ExprEvalPushStep(state, scratch); /* JUMP to end to skip the ON EMPTY steps added below. */ jumps_to_end = lappend_int(jumps_to_end, state->steps_len); @@ -5011,20 +5030,17 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, resv, resnull); /* - * Add a COERCION_FINISH step to check for errors that may occur when - * coercing and rethrow them. + * Add a COERCION_FINISH step to rethrow errors reported softly. This + * must be done unconditionally, since not only the coercion but also + * the ON EMPTY expression itself is evaluated with escontext. + * + * XXX: maybe ExecInitJsonCoercion() should push this step itself. */ - if (jsexpr->on_empty->coerce || - IsA(jsexpr->on_empty->expr, CoerceViaIO) || - IsA(jsexpr->on_empty->expr, CoerceToDomain)) - { - - scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; - scratch->resvalue = resv; - scratch->resnull = resnull; - scratch->d.jsonexpr.jsestate = jsestate; - ExprEvalPushStep(state, scratch); - } + scratch->opcode = EEOP_JSONEXPR_COERCION_FINISH; + scratch->resvalue = resv; + scratch->resnull = resnull; + scratch->d.jsonexpr.jsestate = jsestate; + ExprEvalPushStep(state, scratch); } foreach(lc, jumps_to_end) diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 397219f7a3a..91d1043ecf6 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -5188,6 +5188,13 @@ ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op, { ErrorSaveContext *escontext = op->d.jsonexpr_coercion.escontext; + if (SOFT_ERROR_OCCURRED(escontext)) + { + *op->resnull = true; + *op->resvalue = (Datum) 0; + return; + } + /* * Prepare to call json_populate_type() to coerce the boolean result of * JSON_EXISTS_OP to the target type. If the target type is integer or a diff --git a/src/backend/utils/adt/jsonfuncs.c b/src/backend/utils/adt/jsonfuncs.c index 274ed9d3184..bef8707cc6a 100644 --- a/src/backend/utils/adt/jsonfuncs.c +++ b/src/backend/utils/adt/jsonfuncs.c @@ -3233,6 +3233,17 @@ populate_domain(DomainIOData *io, colname, mcxt, PointerGetDatum(NULL), jsv, isnull, escontext, omit_quotes); Assert(!*isnull || SOFT_ERROR_OCCURRED(escontext)); + + /* + * If the base type's conversion failed softly, don't check the + * domain's constraints on the dummy result: a CHECK expression could + * throw a hard error on it. + */ + if (SOFT_ERROR_OCCURRED(escontext)) + { + *isnull = true; + return (Datum) 0; + } } if (!domain_check_safe(res, *isnull, typid, &io->domain_info, mcxt, @@ -3616,6 +3627,10 @@ populate_record(TupleDesc tupdesc, &nulls[i], escontext, false); + + /* Nothing more to do once a column failed softly. */ + if (SOFT_ERROR_OCCURRED(escontext)) + break; } res = heap_form_tuple(tupdesc, values, nulls); diff --git a/src/test/regress/expected/sqljson_jsontable.out b/src/test/regress/expected/sqljson_jsontable.out index ae64dbed303..d6a22ba8205 100644 --- a/src/test/regress/expected/sqljson_jsontable.out +++ b/src/test/regress/expected/sqljson_jsontable.out @@ -672,6 +672,19 @@ SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS 1 | t (1 row) +-- soft errors in the ON EMPTY expression must be rethrown +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' DEFAULT (0::dint4_0 + 0) ON EMPTY)); +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: value for domain dint4_0 violates check constraint "dint4_0_check" +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' KEEP QUOTES DEFAULT (0::dint4_0 + 0) ON EMPTY)); +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: value for domain dint4_0 violates check constraint "dint4_0_check" +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' DEFAULT (('a' || (random() * 0)::int)::int + 0) ON EMPTY)); +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: invalid input syntax for type integer: "a0" +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' KEEP QUOTES DEFAULT (('a' || (random() * 0)::int)::int + 0) ON EMPTY)); +ERROR: could not coerce ON EMPTY expression (DEFAULT) to the RETURNING type +DETAIL: invalid input syntax for type integer: "a0" DROP DOMAIN dint4, dint4_0; -- JSON_TABLE: WRAPPER/QUOTES clauses on scalar columns SELECT * FROM JSON_TABLE(jsonb '"world"', '$' COLUMNS (item text PATH '$' KEEP QUOTES ON SCALAR STRING)); diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out index fe8aa927b5e..209fb3382c3 100644 --- a/src/test/regress/expected/sqljson_queryfuncs.out +++ b/src/test/regress/expected/sqljson_queryfuncs.out @@ -1515,6 +1515,77 @@ ERROR: invalid ON ERROR behavior LINE 1: SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR); ^ DETAIL: Only ERROR, NULL, EMPTY ARRAY, EMPTY OBJECT, or DEFAULT expression is allowed in ON ERROR for JSON_QUERY(). +-- soft errors raised by the ON ERROR or ON EMPTY DEFAULT expression must always +-- be rethrown +SELECT JSON_VALUE('"a"', '$' RETURNING sqljsonb_int_not_null DEFAULT NULL ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: domain sqljsonb_int_not_null does not allow null values +SELECT JSON_VALUE('"a"', '$' RETURNING int DEFAULT (NULL::sqljsonb_int_not_null + 0) ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: domain sqljsonb_int_not_null does not allow null values +SELECT JSON_QUERY('"a"', '$' RETURNING int DEFAULT (NULL::sqljsonb_int_not_null + 0) ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: domain sqljsonb_int_not_null does not allow null values +SELECT JSON_VALUE('"a"', '$' RETURNING int DEFAULT (('a' || (random() * 0)::int)::int + 0) ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: invalid input syntax for type integer: "a0" +SELECT JSON_QUERY('"a"', '$' RETURNING int DEFAULT (('a' || (random() * 0)::int)::int + 0) ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: invalid input syntax for type integer: "a0" +-- error. should report that the DEFAULT expression cannot be coerced to the +-- RETURNING type, not a NOT NULL violation +SELECT JSON_VALUE(jsonb '{"a": 1}', '$' RETURNING sqljsonb_int_not_null DEFAULT concat('y') ON ERROR); +ERROR: could not coerce ON ERROR expression (DEFAULT) to the RETURNING type +DETAIL: invalid input syntax for type integer: "y" +-- domain constraints must not be checked once a soft error has already occurred +CREATE DOMAIN queryfuncs_int AS int CHECK (1 / coalesce(VALUE, 0) = 0); +SELECT JSON_VALUE(jsonb '"x"', '$' RETURNING int DEFAULT 11 ON ERROR); + json_value +------------ + 11 +(1 row) + +SELECT JSON_QUERY(jsonb '["x"]', '$' RETURNING int[] DEFAULT '{11}' ON ERROR); + json_query +------------ + {11} +(1 row) + +SELECT JSON_VALUE(jsonb '"x"', '$' RETURNING queryfuncs_int DEFAULT 11 ON ERROR); + json_value +------------ + 11 +(1 row) + +SELECT JSON_QUERY(jsonb '["x"]', '$' RETURNING queryfuncs_int[] DEFAULT '{11}' ON ERROR); + json_query +------------ + {11} +(1 row) + +CREATE TYPE compcoalesce AS (a int, b queryfuncs_int); +SELECT JSON_QUERY(jsonb '{"a": "x"}', '$' RETURNING compcoalesce); + json_query +------------ + +(1 row) + +SELECT JSON_QUERY(jsonb '{"a": 1, "b": 11}', '$' RETURNING compcoalesce); + json_query +------------ + (1,11) +(1 row) + +SELECT JSON_QUERY(jsonb '{"a": 1, "b": "B"}', '$' RETURNING compcoalesce); + json_query +------------ + +(1 row) + +SELECT JSON_QUERY(jsonb '{"a": 1, "b": 0}', '$' RETURNING compcoalesce); -- error +ERROR: division by zero +DROP TYPE compcoalesce; +DROP DOMAIN queryfuncs_int; -- Test implicit coercion to a domain over fixed-length type specified in -- RETURNING CREATE DOMAIN queryfuncs_char2 AS char(2); diff --git a/src/test/regress/sql/sqljson_jsontable.sql b/src/test/regress/sql/sqljson_jsontable.sql index 2a33aaec57f..7f0b54b1121 100644 --- a/src/test/regress/sql/sqljson_jsontable.sql +++ b/src/test/regress/sql/sqljson_jsontable.sql @@ -311,6 +311,11 @@ SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' ERROR ON ERROR)); SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' FALSE ON ERROR)); SELECT a, a::bool FROM JSON_TABLE(jsonb '{"a":1}', '$' COLUMNS (a dint4_0 EXISTS PATH '$.b' TRUE ON ERROR)); +-- soft errors in the ON EMPTY expression must be rethrown +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' DEFAULT (0::dint4_0 + 0) ON EMPTY)); +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' KEEP QUOTES DEFAULT (0::dint4_0 + 0) ON EMPTY)); +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' DEFAULT (('a' || (random() * 0)::int)::int + 0) ON EMPTY)); +SELECT * FROM JSON_TABLE('{}', '$' COLUMNS (a int PATH '$.a' KEEP QUOTES DEFAULT (('a' || (random() * 0)::int)::int + 0) ON EMPTY)); DROP DOMAIN dint4, dint4_0; -- JSON_TABLE: WRAPPER/QUOTES clauses on scalar columns diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql index 0767db9cfff..d4ba4c710db 100644 --- a/src/test/regress/sql/sqljson_queryfuncs.sql +++ b/src/test/regress/sql/sqljson_queryfuncs.sql @@ -509,6 +509,32 @@ SELECT JSON_EXISTS(jsonb '1', '$' DEFAULT 1 ON ERROR); SELECT JSON_VALUE(jsonb '1', '$' EMPTY ON ERROR); SELECT JSON_QUERY(jsonb '1', '$' TRUE ON ERROR); +-- soft errors raised by the ON ERROR or ON EMPTY DEFAULT expression must always +-- be rethrown +SELECT JSON_VALUE('"a"', '$' RETURNING sqljsonb_int_not_null DEFAULT NULL ON ERROR); +SELECT JSON_VALUE('"a"', '$' RETURNING int DEFAULT (NULL::sqljsonb_int_not_null + 0) ON ERROR); +SELECT JSON_QUERY('"a"', '$' RETURNING int DEFAULT (NULL::sqljsonb_int_not_null + 0) ON ERROR); +SELECT JSON_VALUE('"a"', '$' RETURNING int DEFAULT (('a' || (random() * 0)::int)::int + 0) ON ERROR); +SELECT JSON_QUERY('"a"', '$' RETURNING int DEFAULT (('a' || (random() * 0)::int)::int + 0) ON ERROR); +-- error. should report that the DEFAULT expression cannot be coerced to the +-- RETURNING type, not a NOT NULL violation +SELECT JSON_VALUE(jsonb '{"a": 1}', '$' RETURNING sqljsonb_int_not_null DEFAULT concat('y') ON ERROR); + +-- domain constraints must not be checked once a soft error has already occurred +CREATE DOMAIN queryfuncs_int AS int CHECK (1 / coalesce(VALUE, 0) = 0); +SELECT JSON_VALUE(jsonb '"x"', '$' RETURNING int DEFAULT 11 ON ERROR); +SELECT JSON_QUERY(jsonb '["x"]', '$' RETURNING int[] DEFAULT '{11}' ON ERROR); +SELECT JSON_VALUE(jsonb '"x"', '$' RETURNING queryfuncs_int DEFAULT 11 ON ERROR); +SELECT JSON_QUERY(jsonb '["x"]', '$' RETURNING queryfuncs_int[] DEFAULT '{11}' ON ERROR); + +CREATE TYPE compcoalesce AS (a int, b queryfuncs_int); +SELECT JSON_QUERY(jsonb '{"a": "x"}', '$' RETURNING compcoalesce); +SELECT JSON_QUERY(jsonb '{"a": 1, "b": 11}', '$' RETURNING compcoalesce); +SELECT JSON_QUERY(jsonb '{"a": 1, "b": "B"}', '$' RETURNING compcoalesce); +SELECT JSON_QUERY(jsonb '{"a": 1, "b": 0}', '$' RETURNING compcoalesce); -- error +DROP TYPE compcoalesce; +DROP DOMAIN queryfuncs_int; + -- Test implicit coercion to a domain over fixed-length type specified in -- RETURNING CREATE DOMAIN queryfuncs_char2 AS char(2); base-commit: fcfd2456078db0a7591fd53c87c67f717ac73fba -- 2.34.1