Re: SQL/JSON json_table plan clause

Hi!

Alexander, thank you for your participation. I am very sorry, I've seen
your previous
email but was busy with other tasks and didn't have time to work on your
notes.
Thank you very much!

On Wed, Jul 8, 2026 at 2:39 AM Alexander Korotkov <aekorotkov(at)gmail(dot)com>
wrote:

> 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
>

--
Regards,
Nikita Malakhov
Postgres Professional
The Russian Postgres Company
https://postgrespro.ru/