Horton

Thread

  1. SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2024-12-17T15:11:02Z

    Hi hackers!

This thread is further work continued in [1], where Amit Langote
suggested starting discussion on the remaining SQL/JSON feature
'PLAN clause for JSON_TABLE' anew.

We'd like to help with merging SQL/JSON patches into vanilla,
and have adapted PLAN clause to recent changes in JSON_TABLE
function.

While doing this we've found that some tests with the PLAN clause
were incorrect, along with JSON_TABLE behavior with this clause.
We've corrected this behavior, but these corrections required reverting
some removed and heavily refactored code, so we'd be glad for review
and feedback on this patch.

Also, we've noticed that Oracle set the PLAN clause for JSON_TABLE
aside for some reason.

[1]
https://www.postgresql.org/message-id/flat/CA%2BHiwqE4XTdfb1nW%3DOjoy_tQSRhYt-q_kb6i5d4xcKyrLC1Nbg%40mail.gmail.com

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

  2. Re: SQL/JSON json_table plan clause

    Andrew Dunstan <andrew@dunslane.net> — 2024-12-17T17:36:24Z

    On 2024-12-17 Tu 10:11 AM, Nikita Malakhov wrote:
> Hi hackers!
>
> This thread is further work continued in [1], where Amit Langote
> suggested starting discussion on the remaining SQL/JSON feature
> 'PLAN clause for JSON_TABLE' anew.
>
> We'd like to help with merging SQL/JSON patches into vanilla,
> and have adapted PLAN clause to recent changes in JSON_TABLE
> function.
>
> While doing this we've found that some tests with the PLAN clause
> were incorrect, along with JSON_TABLE behavior with this clause.
> We've corrected this behavior, but these corrections required reverting
> some removed and heavily refactored code, so we'd be glad for review
> and feedback on this patch.
>
> Also, we've noticed that Oracle set the PLAN clause for JSON_TABLE
> aside for some reason.


I noticed that several months ago, and I was somewhat surprised. I'm not 
sure anyone else has it either. But that doesn't mean we shouldn't have 
it. It's in the standard and seems useful, so I'm very glad to see this 
effort.


cheers


andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  3. Re: SQL/JSON json_table plan clause

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2025-01-16T09:18:04Z

    The following review has been posted through the commitfest application:
make installcheck-world:  tested, failed
Implements feature:       tested, failed
Spec compliant:           not tested
Documentation:            not tested

Hi!
Proposed patch tested at Macosx, compiled and build without warnings.
All tests passed without errors.
JSON_TABLE clause PLAN tested, works as intended.

+1 to commit.

  4. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2025-02-02T19:43:43Z

    Hi hackers!

Patch has been rebased onto the current master.

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

  5. Re: SQL/JSON json_table plan clause

    Vladlen Popolitov <v.popolitov@postgrespro.ru> — 2025-02-03T07:15:59Z

    Nikita Malakhov писал(а) 2025-02-03 02:43:
> Hi hackers!
> 
> Patch has been rebased onto the current master.
> 
> --
> 
> Regards,
> Nikita Malakhov
> Postgres Professional
> The Russian Postgres Company
> https://postgrespro.ru/
Hi!

Tested on Mac with 'make check', all tests passed OK.

-- 
Best regards,

Vladlen Popolitov.

  6. Re: SQL/JSON json_table plan clause

    amit <amitlangote09@gmail.com> — 2025-02-04T03:05:07Z

    Hi Nikita,

On Wed, Dec 18, 2024 at 12:11 AM Nikita Malakhov <hukutoc@gmail.com> wrote:
>
> Hi hackers!
>
> This thread is further work continued in [1], where Amit Langote
> suggested starting discussion on the remaining SQL/JSON feature
> 'PLAN clause for JSON_TABLE' anew.
>
> We'd like to help with merging SQL/JSON patches into vanilla,
> and have adapted PLAN clause to recent changes in JSON_TABLE
> function.

Thanks for working on this.

> While doing this we've found that some tests with the PLAN clause
> were incorrect, along with JSON_TABLE behavior with this clause.
> We've corrected this behavior, but these corrections required reverting
> some removed and heavily refactored code, so we'd be glad for review
> and feedback on this patch.

Sorry, I don't fully understand this paragraph.  Do you mean that
there might be bugs in the existing JSON_TABLE() functionality that
was committed into v17?

-- 
Thanks, Amit Langote

  7. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2025-02-04T11:50:35Z

    Hi Amit!

No, JSON_TABLE is ok. I've meant that in the previous approach to PLAN
clause
implementation there were some wrong tests.

On Tue, Feb 4, 2025 at 6:05 AM Amit Langote <amitlangote09@gmail.com> wrote:

> Hi Nikita,
>
> On Wed, Dec 18, 2024 at 12:11 AM Nikita Malakhov <hukutoc@gmail.com>
> wrote:
> >
> > Hi hackers!
> >
> > This thread is further work continued in [1], where Amit Langote
> > suggested starting discussion on the remaining SQL/JSON feature
> > 'PLAN clause for JSON_TABLE' anew.
> >
> > We'd like to help with merging SQL/JSON patches into vanilla,
> > and have adapted PLAN clause to recent changes in JSON_TABLE
> > function.
>
> Thanks for working on this.
>
> > While doing this we've found that some tests with the PLAN clause
> > were incorrect, along with JSON_TABLE behavior with this clause.
> > We've corrected this behavior, but these corrections required reverting
> > some removed and heavily refactored code, so we'd be glad for review
> > and feedback on this patch.
>
> Sorry, I don't fully understand this paragraph.  Do you mean that
> there might be bugs in the existing JSON_TABLE() functionality that
> was committed into v17?
>
> --
> Thanks, Amit Langote
>


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

  8. Re: SQL/JSON json_table plan clause

    amit <amitlangote09@gmail.com> — 2025-03-14T07:25:22Z

    Hi Nikita,

I was looking at the patch you posted on Feb 3. Two high-level comments:

* The documentation additions are missing.

* It looks like the patch mixes refactoring changes with new
functionality, which makes it harder to follow what's existing vs.
new. Could you separate these changes so the new additions are easier
to review?

More specifically on the 2nd point:

-/*
- * Check if the type is "composite" for the purpose of checking whether to use
- * JSON_VALUE() or JSON_QUERY() for a given JsonTableColumn.
- */
-static bool
-isCompositeType(Oid typid)
-{
-    char        typtype = get_typtype(typid);
-
-    return typid == JSONOID ||
-        typid == JSONBOID ||
-        typid == RECORDOID ||
-        type_is_array(typid) ||
-        typtype == TYPTYPE_COMPOSITE ||
-    /* domain over one of the above? */
-        (typtype == TYPTYPE_DOMAIN &&
-         isCompositeType(getBaseType(typid)));
 }
...

Diffs like this one make me wonder whether this code actually needs to
change for PLAN clause support -- but I suspect it does not.

+
+    /**/
+    bool        cross;
+    bool        outerJoin;
+    bool        advanceNested;
+    bool        advanceRight;
+    bool        reset;

Incomplete comment.

@@ -4124,6 +4130,7 @@ JsonTableInitOpaque(TableFuncScanState *state, int natts)
      * Evaluate JSON_TABLE() PASSING arguments to be passed to the jsonpath
      * executor via JsonPathVariables.
      */
+
     if (state->passingvalexprs)
@@ -4155,15 +4162,13 @@ JsonTableInitOpaque(TableFuncScanState *state,
int natts)
     }

     cxt->colplanstates = palloc(sizeof(JsonTablePlanState *) *
-                                list_length(tf->colvalexprs));
-
+                               list_length(tf->colvalexprs));
     /*
      * Initialize plan for the root path and, recursively, also any child
      * plans that compute the NESTED paths.
      */
     cxt->rootplanstate = JsonTableInitPlan(cxt, rootplan, NULL, args,
-                                           CurrentMemoryContext);
-
+                                       CurrentMemoryContext);
     state->opaque = cxt;
 }

@@ -4201,8 +4206,9 @@ JsonTableInitPlan(JsonTableExecContext *cxt,
JsonTablePlan *plan,
     if (IsA(plan, JsonTablePathScan))
     {
         JsonTablePathScan *scan = (JsonTablePathScan *) plan;
-        int         i;
+        int     i;

Unnecessary whitespace addition/removal.

 /*
- * Transform JSON_TABLE column definition into a JsonFuncExpr
- * This turns:
+ * Transform JSON_TABLE column

Not sure why the comment was rewritten.

-JsonTableResetRowPattern(JsonTablePlanState *planstate, Datum item)
+JsonTableResetContextItem(JsonTablePlanState *planstate, Datum item)

I remember renaming ContextItem with RowPattern, but it seems this is
switching it back.

It makes it look like you're reverting to code from the old patch that
implemented the PLAN clause together with JSON_TABLE(), instead of
building on the committed code and adding just what's needed to
support PLAN clause in JsonTablePlan.  Especially, the changes to
parse_jsontable.c and jsonpath_exec.c,

  9. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2025-03-15T16:21:31Z

    Hi Amit!

Thank you for your feedback, I'll check it out in a couple of days and
reply.

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

  10. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2025-04-11T18:57:05Z

    Hi hackers!

Thanks to Amit for reviewing the patch. I've reworked it according to Amit's
recommendations and divided it into 2 parts: part 1 contains PLAN clause
code and part 2 tests. Reworked code is much more compact, so I hope
it would not be so painful to review.

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

  11. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2025-12-22T15:11:20Z

    Hi hackers!

Patch was tested with the current master. All is good with minor changes in
messages.
The 1st part contains grammar changes with JSON_TABLE transformations
according
to given PLAN strategy, the 2nd contains corrected tests for JSON_TABLE and
new tests
for PLAN clause.

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

  12. Re: SQL/JSON json_table plan clause

    Alexander Korotkov <aekorotkov@gmail.com> — 2026-05-11T12:42:33Z

    Hi, Nikita!

On Mon, Mar 16, 2026 at 6:30 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> I've rebased patch set onto current master and have corrected
> output according to recent changes in JSON array processing.
> Awaiting review.

Thank you for your efforts in this direction. I've reviewed the v22 of
the patch, and I have the following notes.
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.
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.
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).
4. Unused typedefs in src/tools/pgindent/typedefs.list:
JsonTableScanState, JsonPathSpec, JsonTablePlanStateType,
JsonTableJoinState.
5. Empty comment in JsonTablePlanState definition.  Pointed by Amit,
but not fixed.
6. Rename passingArgs to passing_Args for no reason in parse_jsontable.c.
7. Patch lacks documentation (also pointed by Amit)
8. Patch could use pgindent run.

------
Regards,
Alexander Korotkov
Supabase

  13. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2026-05-11T20:12:53Z

    Hi Alexander,
Thank you for the review! I'll try to correct noted points asap.

On Mon, May 11, 2026 at 3:42 PM Alexander Korotkov <aekorotkov@gmail.com>
wrote:

> Hi, Nikita!
>
> On Mon, Mar 16, 2026 at 6:30 PM Nikita Malakhov <hukutoc@gmail.com> wrote:
> > I've rebased patch set onto current master and have corrected
> > output according to recent changes in JSON array processing.
> > Awaiting review.
>
> Thank you for your efforts in this direction. I've reviewed the v22 of
> the patch, and I have the following notes.
> 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.
> 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.
> 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).
> 4. Unused typedefs in src/tools/pgindent/typedefs.list:
> JsonTableScanState, JsonPathSpec, JsonTablePlanStateType,
> JsonTableJoinState.
> 5. Empty comment in JsonTablePlanState definition.  Pointed by Amit,
> but not fixed.
> 6. Rename passingArgs to passing_Args for no reason in parse_jsontable.c.
> 7. Patch lacks documentation (also pointed by Amit)
> 8. Patch could use pgindent run.
>
> ------
> Regards,
> Alexander Korotkov
> Supabase
>


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

  14. Re: SQL/JSON json_table plan clause

    Nikita Malakhov <hukutoc@gmail.com> — 2026-05-15T11:53:14Z

    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/