Re: [PATCH]Fix planner's strictness check for JsonExpr

Hi Ethan,

Thanks for the thorough review, and for the +1.

> The conservative check prevents the safe inlining of a context-only
wrapper:
> ...
> With 100,000 rows, median tested execution time increased
> from 34.9 ms to 137.3 ms, about 3.93x on macOS with JIT and parallelism
> off.

Thanks for measuring this, I overlooked the performance issue with inlining,
and can reproduce in my local env.

In v1 I wrote that distinguishing the strict cases "hardly seems worth
the trouble", but your numbers show otherwise, and since this is meant
to be back-patched to v17, I'd rather not have people's existing
context-only wrappers slow down by ~4x in a minor release. So rather
than leaving it as a follow-up, the attached v2 makes the check precise.

`ExecInitJsonExpr()` returns NULL as soon as the context item or the
path specification evaluates to NULL, before PASSING arguments, the
jsonpath evaluation or the ON EMPTY / ON ERROR behaviors are even
looked at. So a JsonExpr is strict with respect to those two inputs,
and the non-strictness can only come from:

1. PASSING arguments, and
2. inputs of the ON EMPTY / ON ERROR expressions (the parser rejects
column references in DEFAULT, but not Params, so e.g.
"DEFAULT $1 + 0 ON EMPTY" inside a SQL function body is accepted).

v2 therefore treats a `JsonExpr` as non-strict if it has any PASSING
argument or a non-Const ON EMPTY / ON ERROR expression, and otherwise
checks only formatted_expr and path_spec. The Const test is a bit
conservative (a DEFAULT that needs a coercion is not a bare Const), but
the default behaviors are, which covers the common case. Note that we
can't just let expression_tree_walker() descend into the JsonExpr, as
it would then see the PASSING Vars/Params as perfectly strict inputs.

> Nit: could you add a STRICT-inlining regression test alongside the
> outer-join test?

Done, using your example. v2 now has these tests:

- outer-join pull-up with PASSING (as in v1)
- STRICT SQL function with PASSING $1 (your example)
- STRICT SQL function with DEFAULT $1 + 0 ON EMPTY
- STRICT SQL function that only uses its argument as the context item:
EXPLAIN VERBOSE shows it is inlined, plus a check of the results

The first three return a wrong non-NULL value on unpatched master and
NULL with v2. With v2, value_ctx() is inlined again, so the plan is the
same as on unpatched master. Likewise, a context-only JSON_VALUE() in a
subquery pulled up under an outer join no longer gets an unnecessary
PlaceHolderVar, which v1 added.

Greg, since v2 changes the approach from "always non-strict" to the
precise check, another look would be welcome if you have time.

Regards,
Zhenglong Li

<ethantran(at)quwin(dot)dev> 于2026年10月29日周四 10:47写道：

>
> Hi Zhenglong,
>
> This is another review of CF 7186 v1 [1].
>
> Contents & Purpose
>
> ==================
>
> The patch treats JsonExpr as non-strict so that outer-join pull-up and
>
> STRICT SQL-function inlining preserves NULL semantics. It includes a
>
> regression test for the outer-join case.
>
> Initial Run
>
> ==================
>
> The patch applies cleanly to HEAD. Baseline and patched
>
> builds each pass all 239 core tests, and the patched build passes all 582
>
> make installcheck-world tests. TAP was disabled; PL/Perl, PL/Python and
>
> SSL was not configured.
>
> Performance
>
> ==================
>
> The conservative check prevents the safe inlining of a context-only
> wrapper:
>
> CREATE FUNCTION value_ctx(text) RETURNS text
>
> LANGUAGE sql IMMUTABLE STRICT
>
> AS SELECTJSONVALUE($1::jsonb,'$');
>
> SELECT sum(length(value_ctx(doc))) FROM perf;
>
> With 100,000 rows, median tested execution time increased
>
> from 34.9 ms to 137.3 ms, about 3.93x on macOS with JIT and parallelism
>
> off.
>
> Nitpicking & Conclusion
>
> ==================
>
> Nit: could you add a STRICT-inlining regression test alongside the
> outer-join
>
> test? For example:
>
> CREATE TEMP TABLE args(x integer);
>
> INSERT INTO args VALUES (NULL), (1);
>
> CREATE FUNCTION pg_temp.f(integer) RETURNS text
>
> LANGUAGE sql IMMUTABLE STRICT
>
> AS SELECTJSONVALUE('1','$'PASSING$1ASp);
>
> SELECT x, pg_temp.f(x) FROM args;
>
> For the NULL row, unpatched master returns '1'; v1 correctly returns NULL.
>
> This approach looks appropriate for the correctness fix.
>
> Since this is a correctness fix, preserving safe context-only inlining
> could be a follow-up optimization.
>
> +1 from me on the approach and the patch.
>
> References
>
> ==================
>
> [1] CF 7186, 2026-08-24, [PATCH]Fix planner's strictness check for JsonExpr
>
>
> https://www.postgresql.org/message-id/CAD=-kXbK=GGAAK62N-r6tKuU2dVmtJ4GvHTr=CJJGHczuuUGCQ@mail.gmail.com
>
>