Re: SQL/JSON json_table plan clause

Hi hackers!

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.

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