Hi, On Wed, Oct 28, 2026 at 2:38 PM jian he <[email protected]> wrote: > > 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.
You're correct on all counts. I've fixed that in the attached patch. Thanks for checking. -- Thanks, Amit Langote
v4-0002-SQL-JSON-fix-error-handling-when-coercing-to-a-do.patch
Description: Binary data
v4-0001-SQL-JSON-always-rethrow-soft-errors-from-DEFAULT-.patch
Description: Binary data
