Re: SQL/JSON json_table plan clause

Hi Nikita!

On Fri, May 15, 2026 at 2:53 PM Nikita Malakhov <hukutoc(at)gmail(dot)com> wrote:
> Again, Alexander and Amit, thanks for the review. I've rebased the patch and made
> some changes according to Alexander's notes. I've slightly rearranged files between
> patches for it to be easier to review, so now there are 3 patches:
> v23-0001-json-table-plan-clause.patch - main code changes
> v23-0002-json-table-plan-tests.patch - test cases and out file changes
> v23-0003-json-table-plan-docs.patch - docs package
>
> <...>
> >1. IsA(planstate, JsonTableSiblingJoin) is wrong. planstate is not a
> >node, thus IsA() can't be applied to it. You should instead do
> >IsA(planstate->plan, JsonTableSiblingJoin). It wasn't catched,
> >because regression tests don't exercise this branch. So, you also
> >need to improve the coverage.
> - done;
>
> >2. get_json_table() with patch uses JSON_BEHAVIOR_EMPTY as the default
> >value for deparsing, while parsing still uses
> >JSON_BEHAVIOR_EMPTY_ARRAY. Looks plain wrong. I'm not sure what is
> >intention here.
> - looks like leftover from older version, changed to default behavior, so unnecessary
> emission of ERROR is omitted;
>
> >3. PLAN clause is always emitted during deparsing even if user didn't
> >specify anything. I would prefer to skip PLAN clause in this case
> >unless there is strong reason to do the opposite (this reason must be
> >pointed if any).
> - done, this made test cases much more readable;
>
> >4. Unused typedefs in src/tools/pgindent/typedefs.list:
> >JsonTableScanState, JsonPathSpec, JsonTablePlanStateType,
> >JsonTableJoinState.
> - done;
>
> >5. Empty comment in JsonTablePlanState definition. Pointed by Amit,
> >but not fixed.
> - done;
>
> >6. Rename passingArgs to passing_Args for no reason in parse_jsontable.c.
> - done;
>
> >7. Patch lacks documentation (also pointed by Amit)
> - done, documentation is provided in separate patch;
>
> >8. Patch could use pgindent run.
> - not done yet but would provide a newer version with it.

Thank you. I've made following additional changes.

1. Actually, I don't see my previous item 3 fully addressed. We
still had PLAN clause deparsed in the case user didn't specify it. I
see this as an undesirable behavior. And there are at least two ways
to fix this: don't output PLAN clause if it's not the default,
remember if user specified as a special flag or even original clause.
I found first way (don't output default) more appealing as it don't
require additional struct members and we already do this in other
similar cases (NULLS FIRST/LAST, ON EMPTY/ON ERROR). So, I did this
for PLAN clause.
2. Simplification for JsonTablePlanNextRow(): no reason to check
(planstate->advanceNested || planstate->nested) if we already issued
break for (planstate->nested == NULL).
3. Similar simplification for transformJsonTableNestedColumns().

------
Regards,
Alexander Korotkov
Supabase