Re: BUG #19851: Adding a tautological CHECK(true) to a domain changes JSON_VALUE's result (silent NULL, value/type c

On Fri, Nov 13, 2026 at 6:32 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> PG Bug reporting form <noreply(at)postgresql(dot)org> writes:
> > Two domains with the *identical value set* — one declared without
> > constraints, one with only a tautological `CHECK (true)` that can never
> > reject anything — give *different results* for the same `JSON_VALUE`
> > expression. `CHECK(true)` is a no-op semantically, so the two results must
> > agree, and both must agree with the base type.
>
> Yeah, this is a mess. The entire design of JsonExpr in this area
> is a disaster, starting here:
>
> /*
> * Information about converting the result of jsonpath functions
> * JsonPathQuery() and JsonPathValue() to the RETURNING type.
> */
> bool use_io_coercion;
> bool use_json_coercion;
>
> Possibly, if these flags hadn't been so utterly undocumented,
> transformJsonFuncExpr wouldn't be so confused about what it needs
> to do with domains. But there are worse problems. The broken
> logic is in parse_expr.c:
>
> if (get_typtype(jsexpr->returning->typid) == TYPTYPE_DOMAIN &&
> DomainHasConstraints(jsexpr->returning->typid, NULL))
> jsexpr->use_json_coercion = true;
> else
> jsexpr->use_io_coercion = true;
>
> Quite aside from whether this is the right thing, testing
> DomainHasConstraints *at parse time* is insane. The situation
> could be completely different by the time we execute the expression.
> At the earliest this decision should be made in the planner; but
> probably doing it at executor startup would be better.

Yeah, I should have noticed that the result of a parse-time
DomainHasConstraints() test gets frozen into the stored JsonExpr,
while the domain's constraints can change at any time after that.
Those flags are derivable from the rest of the node and shouldn't have
been stored in it to begin with.

> Another reason for doing it later is that we cannot fix the behavior
> in released versions given this structure: even if we end up still
> using exactly two booleans, any stored JsonExpr will already have
> locked down the values of these flags. What I'm contemplating as a
> potential fix design is to remove these flags in master, leave them
> present-but-unused in back branches, and adjust the runtime
> expression representation instead.

I've started working on a patch, which I'll post in a couple of days.
The main change is that the choice of coercion method will depend only
on which function it is (JSON_VALUE, JSON_QUERY or JSON_EXISTS) and
the RETURNING type, not on whether the domain has constraints, so a
domain gets the same result as its base type. The constraint test was
there so that a NULL result would still get the domain's constraints
checked; the patch will pass the NULL to domain_in() instead.

With that, nothing in the decision can change after parsing, but I'd
still make it at executor startup, because the flags already stored in
back-branch JsonExprs were computed by the old rule and have to be
ignored.

In master, as you say, the patch will remove the two flags from
JsonExpr and also the parser code (in transformJsonFuncExpr()) that
sets them, while leaving all of that in place in the back branches.

> I'm afraid now to look through the rest of the SQL/JSON logic for
> similar errors.

I'll scan the code to look for more. From a quick look, I don't see
another place where something that can change later is tested at parse
time and the result stored in the node, but there are a few places in
SQL/JSON code in parse_expr.c where the type is compared against
json/jsonb/bytea without looking through domains, so a domain over one
of those behaves differently from the base type. I'll go through
those more carefully. I'm also looking at some other reported
SQL/JSON bugs in the meantime.

--
Thanks, Amit Langote