Re: SQL/JSON json_table plan clause

Hi!

On Sun, Jul 5, 2026 at 12:02 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> 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().

I made some additional changes to the patch. In the previous versions
PLAN qual required name for root path, but didn't required names for
nested paths. I've rechecked with standard, and it appears that
standard claims the opposite: name is not required for the root path,
but required for nested paths. So, I decided to allow skipping name
for both root path (as standard claims), and nested path (our
extension to standard). Corresponding changes made to code, tests and
documentation.

Also, I merged 3 patches into 1, and edited sql_features.txt.

I'm going to push this if no objections.

------
Regards,
Alexander Korotkov
Supabase