From d552eaa3e0da1a892cdf3a1e05c100c502b011d8 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Wed, 4 Nov 2026 19:02:18 +0900 Subject: [PATCH v4] SQL/JSON: Fix ON EMPTY/ERROR state leakage on NULL input ExecInitJsonExpr() emitted JUMP_IF_NULL steps to skip the EEOP_JSONEXPR_PATH step when either the context item or the path expression was NULL, landing on a CONST step that returned NULL. However, ExecEvalJsonExprPath(), which implements EEOP_JSONEXPR_PATH, was also the only place where JsonExprState's empty and error flags got reset for a new row. Skipping it left the flags as the previous row had set them, and since execution then fell through the ON EMPTY / ON ERROR steps that check those flags, a NULL-input row returned the previous row's ON EMPTY / ON ERROR expression instead of NULL, making the result depend on the order of the input rows. This could only happen when ExecInitJsonExpr() had emitted the ON EMPTY / ON ERROR steps for the NULL-input row to fall through, which it does unless the corresponding expression is a NULL constant and the RETURNING type is not a domain. So JSON_VALUE and JSON_QUERY with their default NULL ON EMPTY / NULL ON ERROR behaviors were not affected, but JSON_EXISTS, whose default is FALSE ON ERROR, was. Fix by pointing those JUMP_IF_NULL steps at EEOP_JSONEXPR_PATH itself and dropping the CONST NULL step. ExecEvalJsonExprPath() now returns NULL for NULL input after resetting the flags, skipping jsonpath evaluation and the ON EMPTY / ON ERROR steps as before, but still jumping to the coercion steps when RETURNING a domain type so that its constraints are checked. Oversight in commit 6185c9737cf, so back-patch to 17. Bug: #19621 Reported-by: Suyang Zhong Author: jian he Author: Andrey Rachitskiy Reviewed-by: Srinath Reddy Sadipiralla Tested-by: Manu Discussion: https://postgr.es/m/19621-0a480d2dc74e6bd5@postgresql.org Backpatch-through: 17 --- src/backend/executor/execExpr.c | 24 +++--- src/backend/executor/execExprInterp.c | 33 +++++++-- .../regress/expected/sqljson_queryfuncs.out | 73 +++++++++++++++++++ src/test/regress/sql/sqljson_queryfuncs.sql | 29 ++++++++ 4 files changed, 140 insertions(+), 19 deletions(-) diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c index 35cec43fb3d..fd216c4978d 100644 --- a/src/backend/executor/execExpr.c +++ b/src/backend/executor/execExpr.c @@ -4830,17 +4830,12 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, jsestate->args = lappend(jsestate->args, var); } - /* Step for jsonpath evaluation; see ExecEvalJsonExprPath(). */ - scratch->opcode = EEOP_JSONEXPR_PATH; - scratch->resvalue = resv; - scratch->resnull = resnull; - scratch->d.jsonexpr.jsestate = jsestate; - ExprEvalPushStep(state, scratch); - /* - * Step to return NULL after jumping to skip the EEOP_JSONEXPR_PATH step - * when either formatted_expr or pathspec is NULL. Adjust jump target - * addresses of JUMPs that we added above. + * Adjust jump target addresses of JUMPs that we added above to point to + * the EEOP_JSONEXPR_PATH step. That step itself handles the case where + * either formatted_expr or pathspec is NULL by returning NULL, which + * ensures that JsonExprState's empty/error flags are reset for every row + * before the ON EMPTY / ON ERROR steps below look at them. */ foreach(lc, jumps_return_null) { @@ -4848,11 +4843,12 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, as->d.jump.jumpdone = state->steps_len; } - scratch->opcode = EEOP_CONST; + + /* Step for jsonpath evaluation; see ExecEvalJsonExprPath(). */ + scratch->opcode = EEOP_JSONEXPR_PATH; scratch->resvalue = resv; scratch->resnull = resnull; - scratch->d.constval.value = (Datum) 0; - scratch->d.constval.isnull = true; + scratch->d.jsonexpr.jsestate = jsestate; ExprEvalPushStep(state, scratch); escontext = jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR ? @@ -4867,7 +4863,7 @@ ExecInitJsonExpr(JsonExpr *jsexpr, ExprState *state, /* * Steps to coerce the result value computed by EEOP_JSONEXPR_PATH or the - * NULL returned on NULL input as described above. + * NULL that it returns on NULL input. */ jsestate->jump_eval_coercion = -1; if (jsexpr->use_json_coercion) diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index d0d70be13e1..2391ac41e4c 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -4891,8 +4891,15 @@ ExecEvalJsonIsPredicate(ExprState *state, ExprEvalStep *op) } /* - * Evaluate a jsonpath against a document, both of which must have been - * evaluated and their values saved in op->d.jsonexpr.jsestate. + * Evaluate a jsonpath against a document, whose values (if not NULL) have been + * saved in op->d.jsonexpr.jsestate. + * + * This step is reached even if the document or the jsonpath is NULL, so that + * JsonExprState.error and JsonExprState.empty are reset for every row before + * the ON ERROR / ON EMPTY steps look at them. In that case, the result is + * NULL and jsonpath evaluation is skipped, as are the ON ERROR / ON EMPTY + * steps; the NULL is still passed through the coercion steps, if any, so that + * the constraints of a RETURNING domain type are checked. * * If an error occurs during JsonPath* evaluation or when coercing its result * to the RETURNING type, JsonExprState.error is set to true, provided the @@ -4919,9 +4926,6 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, int jump_eval_coercion = jsestate->jump_eval_coercion; char *val_string = NULL; - item = jsestate->formatted_expr.value; - path = DatumGetJsonPathP(jsestate->pathspec.value); - /* Set error/empty to false. */ memset(&jsestate->error, 0, sizeof(NullableDatum)); memset(&jsestate->empty, 0, sizeof(NullableDatum)); @@ -4934,6 +4938,25 @@ ExecEvalJsonExprPath(ExprState *state, ExprEvalStep *op, } jsestate->escontext.error_occurred = false; + /* + * Return NULL if formatted_expr or pathspec is NULL, skipping the ON ERROR + * and ON EMPTY steps. If the RETURNING type is a domain with constraints, + * the NULL must still be coerced to it so that the constraints are checked. + */ + if (jsestate->formatted_expr.isnull || jsestate->pathspec.isnull) + { + *op->resvalue = (Datum) 0; + *op->resnull = true; + + if (jump_eval_coercion >= 0) + return jump_eval_coercion; + else + return jsestate->jump_end; + } + + item = jsestate->formatted_expr.value; + path = DatumGetJsonPathP(jsestate->pathspec.value); + switch (jsexpr->op) { case JSON_EXISTS_OP: diff --git a/src/test/regress/expected/sqljson_queryfuncs.out b/src/test/regress/expected/sqljson_queryfuncs.out index 209fb3382c3..230c7a6f2b1 100644 --- a/src/test/regress/expected/sqljson_queryfuncs.out +++ b/src/test/regress/expected/sqljson_queryfuncs.out @@ -337,6 +337,16 @@ SELECT JSON_VALUE('"purple"'::jsonb, 'lax $[*]' RETURNING rgb); SELECT JSON_VALUE('"purple"'::jsonb, 'lax $[*]' RETURNING rgb ERROR ON ERROR); ERROR: value for domain rgb violates check constraint "rgb_check" +SELECT x IS NULL AS is_null, + json_value(x, 'strict $.a' RETURNING sqljsonb_int_not_null DEFAULT 1 ON ERROR) AS jv +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); + is_null | jv +---------+---- + f | 1 + t | 1 + f | 1 +(3 rows) + SELECT JSON_VALUE(jsonb '[]', '$'); json_value ------------ @@ -475,6 +485,69 @@ FROM 2 | -1 (3 rows) +-- Check that a NULL context item or path doesn't return a previous row's +-- ON EMPTY / ON ERROR result (bug #19621). +SELECT x IS NULL AS is_null, + json_value(x, '$.a' RETURNING int DEFAULT 42 ON EMPTY) AS jv +FROM (VALUES ('{}'), (NULL), ('{}')) v(x); + is_null | jv +---------+---- + f | 42 + t | + f | 42 +(3 rows) + +SELECT x IS NULL AS is_null, + json_value(x, 'strict $.a' RETURNING int DEFAULT 42 ON ERROR) AS jv +FROM (VALUES ('1'), (NULL), ('1')) v(x); + is_null | jv +---------+---- + f | 42 + t | + f | 42 +(3 rows) + +SELECT p IS NULL AS is_null, + json_value('{}', p RETURNING int DEFAULT 42 ON EMPTY) AS jv +FROM (VALUES ('$.a'::jsonpath), (NULL), ('$.a'::jsonpath)) v(p); + is_null | jv +---------+---- + f | 42 + t | + f | 42 +(3 rows) + +SELECT x IS NULL AS is_null, + json_query(x, '$.a' DEFAULT '"empty"' ON EMPTY) AS jq +FROM (VALUES ('{}'::jsonb), (NULL), ('{}'::jsonb)) v(x); + is_null | jq +---------+--------- + f | "empty" + t | + f | "empty" +(3 rows) + +SELECT x IS NULL AS is_null, + json_exists(x, 'strict $.a' TRUE ON ERROR) AS je +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); + is_null | je +---------+---- + f | t + t | + f | t +(3 rows) + +-- the default FALSE ON ERROR is affected too +SELECT x IS NULL AS is_null, + json_exists(x, 'strict $.a') AS je +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); + is_null | je +---------+---- + f | f + t | + f | f +(3 rows) + SELECT JSON_VALUE(jsonb 'null', '$a' PASSING point ' (1, 2 )' AS a); json_value ------------ diff --git a/src/test/regress/sql/sqljson_queryfuncs.sql b/src/test/regress/sql/sqljson_queryfuncs.sql index d4ba4c710db..a3339a45604 100644 --- a/src/test/regress/sql/sqljson_queryfuncs.sql +++ b/src/test/regress/sql/sqljson_queryfuncs.sql @@ -80,6 +80,9 @@ CREATE TYPE rainbow AS ENUM ('red', 'orange', 'yellow', 'green', 'blue', 'purple CREATE DOMAIN rgb AS rainbow CHECK (VALUE IN ('red', 'green', 'blue')); SELECT JSON_VALUE('"purple"'::jsonb, 'lax $[*]' RETURNING rgb); SELECT JSON_VALUE('"purple"'::jsonb, 'lax $[*]' RETURNING rgb ERROR ON ERROR); +SELECT x IS NULL AS is_null, + json_value(x, 'strict $.a' RETURNING sqljsonb_int_not_null DEFAULT 1 ON ERROR) AS jv +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); SELECT JSON_VALUE(jsonb '[]', '$'); SELECT JSON_VALUE(jsonb '[]', '$' ERROR ON ERROR); @@ -128,6 +131,32 @@ SELECT FROM generate_series(0, 2) x; +-- Check that a NULL context item or path doesn't return a previous row's +-- ON EMPTY / ON ERROR result (bug #19621). +SELECT x IS NULL AS is_null, + json_value(x, '$.a' RETURNING int DEFAULT 42 ON EMPTY) AS jv +FROM (VALUES ('{}'), (NULL), ('{}')) v(x); + +SELECT x IS NULL AS is_null, + json_value(x, 'strict $.a' RETURNING int DEFAULT 42 ON ERROR) AS jv +FROM (VALUES ('1'), (NULL), ('1')) v(x); + +SELECT p IS NULL AS is_null, + json_value('{}', p RETURNING int DEFAULT 42 ON EMPTY) AS jv +FROM (VALUES ('$.a'::jsonpath), (NULL), ('$.a'::jsonpath)) v(p); + +SELECT x IS NULL AS is_null, + json_query(x, '$.a' DEFAULT '"empty"' ON EMPTY) AS jq +FROM (VALUES ('{}'::jsonb), (NULL), ('{}'::jsonb)) v(x); + +SELECT x IS NULL AS is_null, + json_exists(x, 'strict $.a' TRUE ON ERROR) AS je +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); +-- the default FALSE ON ERROR is affected too +SELECT x IS NULL AS is_null, + json_exists(x, 'strict $.a') AS je +FROM (VALUES ('1'::jsonb), (NULL), ('1'::jsonb)) v(x); + SELECT JSON_VALUE(jsonb 'null', '$a' PASSING point ' (1, 2 )' AS a); SELECT JSON_VALUE(jsonb 'null', '$a' PASSING point ' (1, 2 )' AS a RETURNING point); SELECT JSON_VALUE(jsonb 'null', '$a' PASSING point ' (1, 2 )' AS a RETURNING point ERROR ON ERROR); -- 2.47.3