On Tue, Oct 27, 2026 at 9:22 PM Amit Langote <[email protected]> wrote:
>
> I’ve looked at the patch and it seems to be fixing the following two bugs:
>
> 1. Lack of a COERCION_FINISH step after ON ERROR/EMPTY expression
> evaluation in some cases meant the soft error saved during that
> evaluation was never rethrown and the query silently returned NULL.
>
Hi. in ExecInitJsonExpr, first we have:
escontext = jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR ?
&jsestate->escontext : NULL;
then
if (jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
(!(IsA(jsexpr->on_error->expr, Const) &&
((Const *) jsexpr->on_error->expr)->constisnull) ||
returning_domain))
{
}
Within the IF branch above, escontext is never NULL, so the `if
(escontext)` check can be removed.
The comment there needs updating accordingly.
For ON EMPTY clause
if (jsexpr->on_empty != NULL &&
jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
(!(IsA(jsexpr->on_empty->expr, Const) &&
((Const *) jsexpr->on_empty->expr)->constisnull) ||
returning_domain))
{
}
escontext can be NULL. So, the comment
/*
* Steps to evaluate the ON EMPTY expression; handle errors softly to
* rethrow them in COERCION_FINISH step that will be added later.
*/
is not fully accurate. We can change it to:
/*
* Steps to evaluate the ON EMPTY expression. Unless ON ERROR is
* ERROR, errors are handled softly, to be rethrown by the
* COERCION_FINISH step added below; otherwise they are thrown
* directly.
*/
> 2. Once a soft error has occurred, the dummy NULL it leaves behind
> should not be processed any further, but in a few places
> (CoerceToDomain, populate_domain(), populate_record(),
> ExecEvalJsonCoercion() called on the DEFAULT result) it currently is.
>
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -5188,6 +5188,21 @@ ExecEvalJsonCoercion(ExprState *state, ExprEvalStep *op,
{
ErrorSaveContext *escontext = op->d.jsonexpr_coercion.escontext;
+ /*
+ * An ON ERROR or ON EMPTY DEFAULT expression is evaluated with escontext
+ * in effect, so if it failed, the error has been saved in escontext and
+ * the value we were given to coerce is a dummy NULL. Return it as is;
+ * the COERCION_FINISH step that follows will throw the saved error, and
+ * running the coercion on the NULL might instead throw a hard error or
+ * overwrite that saved error.
+ */
+ if (SOFT_ERROR_OCCURRED(escontext))
+ {
+ *op->resnull = true;
+ *op->resvalue = (Datum) 0;
+ return;
+ }
+
The first sentence of the comment is not entirely accurate.
The value in op->resvalue is not guaranteed to be a dummy NULL.
For example, ExecInitCoerceToDomain, writes the result of domain CHECK
constraint check into
scratch->d.domaincheck.checkvalue rather than op->resvalue, so after a soft
CHECK violation op->resvalue still holds the original value.
--
jian
https://www.enterprisedb.com/
