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 <[email protected]> 于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=cjjghczuuu...@mail.gmail.com > >
v2-0001-Fix-planner-s-strictness-check-for-JsonExpr.patch
Description: Binary data
