Subject: Re: SQL/JSON json_table plan clause

From: Amit Langote <[email-masked]>
Date: Tue, 4 Feb 2025 12:05:07 +0900

Hi Nikita,

On Wed, Dec 18, 2024 at 12:11 AM Nikita Malakhov <[email-masked]> 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

---

From: Nikita Malakhov <[email-masked]>
Date: Tue, 4 Feb 2025 14:50:35 +0300

Hi Amit!

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

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

---

From: Alexander Korotkov <[email-masked]>
Date: Sun, 5 Jul 2026 00:02:40 +0300

Hi Nikita!

On Fri, May 15, 2026 at 2:53 PM Nikita Malakhov <[email-masked]> 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

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 8 Jul 2026 02:38:56 +0300

Hi!

>  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

---

From: Nikita Malakhov <[email-masked]>
Date: Wed, 8 Jul 2026 18:35:00 +0300

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!

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 8 Jul 2026 20:03:48 +0300

On Wed, Jul 8, 2026 at 6:35 PM Nikita Malakhov <[email-masked]> wrote:
> 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!

OK, no problem.  Are you good with the current shape of the patch?

------
Regards,
Alexander Korotkov
Supabase

---

From: Nikita Malakhov <[email-masked]>
Date: Thu, 9 Jul 2026 14:35:15 +0300

Hi Alexander!

Don't have any objections. I've looked through your version of the patch,
additional comments
are very helpful. Thank you!

---

From: Thom Brown <[email-masked]>
Date: Thu, 9 Jul 2026 17:11:32 +0100

> OK, no problem.  Are you good with the current shape of the patch?

I've been giving this a test drive. I saw that this has been
committed, and re-tested against that, and the issues I identified
(unless I've misunderstood some functionality) seem to have survived.

First, doesn't this need a catversion bump? This adds fields to
existing node types.

I only get the first column from the following. The second one
disappears without error:

SELECT * FROM JSON_TABLE(
  jsonb '[{"x":[1],"y":[2]}]', '$[*]' AS p0
  COLUMNS (
    NESTED PATH '$.x[*]' AS json_table_path_0 COLUMNS (x int PATH '$'),
    NESTED PATH '$.y[*]' COLUMNS (y int PATH '$')
  )
  PLAN (p0 OUTER json_table_path_0)
) jt;
 x
---
 1
(1 row)

I have the following view:
CREATE VIEW v_chain AS
SELECT * FROM JSON_TABLE(
  jsonb '[{"x": [{"y": [1,2]}]}]', '$[*]' AS p0
  COLUMNS ( NESTED PATH '$.x[*]' AS p1 COLUMNS (
   NESTED PATH '$.y[*]' AS p11 COLUMNS ( y int PATH '$' ) ) )
  PLAN (p0 OUTER (p1 INNER p11))
) jt;

When I dump it, I get:

CREATE VIEW public.v_chain AS
 SELECT y
   FROM JSON_TABLE(
        '[{"x": [{"y": [1, 2]}]}]'::jsonb, '$[*]' AS p0
        COLUMNS (
            NESTED PATH '$."x"[*]' AS p1
            COLUMNS (
                NESTED PATH '$."y"[*]' AS p11
                COLUMNS (
                    y integer PATH '$'
                )
            )
        )
        PLAN (p0 OUTER p1 INNER p11)
    ) jt;

The parentheses around "p1 INNER p11" aren't preserved.

Another dump issue with the following:

CREATE VIEW v_onempty AS SELECT * FROM JSON_TABLE(
  jsonb '{}', '$' AS p0
  COLUMNS ( a int PATH '$.nosuch' ERROR ON EMPTY )
  ERROR ON ERROR
) jt;

This dumps as:

CREATE VIEW public.v_onempty AS
 SELECT a
   FROM JSON_TABLE(
            '{}'::jsonb, '$' AS p0
            COLUMNS (
                a integer PATH '$."nosuch"'
            ) ERROR ON ERROR
        ) jt;

It's lost "ERROR ON EMPTY".

Another example:

SELECT * FROM JSON_TABLE(jsonb '"mystring"', '$'
  COLUMNS (a int PATH '$')
  ERROR ON ERROR
) jt;

This gives me:

ERROR:  invalid input syntax for type integer: "mystring"

But the documentation says that the table-level clause "does not
affect the errors that occur when evaluating columns". Am I missing
something here?

I haven't done any performance testing yet

Thom