Re: SQL/JSON DEFAULT ON ERROR/ON EMPTY evaluation fail should rethrow error unconditionally

Hi,

On Wed, Oct 28, 2026 at 2:38 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> On Tue, Oct 27, 2026 at 9:22 PM Amit Langote <amitlangote09(at)gmail(dot)com> 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.

You're correct on all counts. I've fixed that in the attached patch.
Thanks for checking.

--
Thanks, Amit Langote