Horton

Re: pgsql: Add more SQL/JSON constructor functions

Amit Langote <amitlangote09@gmail.com>

View thread View on postgr.es
From: Amit Langote <amitlangote09@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2024-09-06T03:07:49Z
Lists: pgsql-hackers 
On Thu, Sep 5, 2024 at 9:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Tue, Sep 3, 2024 at 6:05 PM jian he <jian.universality@gmail.com> wrote:
> > On Mon, Sep 2, 2024 at 4:18 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > v2-0001 looks good to me.
> > +-- Test JSON_TABLE() column deparsing -- don't emit default ON ERROR / EMPTY
> > +-- behavior
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text PATH '$'));
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> >
> > Are these tests duplicated? appears both in v2-0002 and v2-0003.
> >
> > 0002 output is:
> > +EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> > +
> > QUERY PLAN
> > +------------------------------------------------------------------------------------------------------------------------------------------------
> > + Table Function Scan on "json_table"  (cost=0.01..1.00 rows=100 width=32)
> > +   Output: a
> > +   Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > json_table_path_0 COLUMNS (a text PATH '$' NULL ON EMPTY NULL ON
> > ERROR) ERROR ON ERROR)
> > +(3 rows)
> >
> > 0003 output is:
> > EXPLAIN VERBOSE SELECT * from JSON_TABLE('"a"', '$' COLUMNS (a text
> > PATH '$') ERROR ON ERROR);
> >                                                      QUERY PLAN
> > --------------------------------------------------------------------------------------------------------------------
> >  Table Function Scan on "json_table"  (cost=0.01..1.00 rows=100 width=32)
> >    Output: a
> >    Table Function Call: JSON_TABLE('"a"'::jsonb, '$' AS
> > json_table_path_0 COLUMNS (a text PATH '$') ERROR ON ERROR)
> > (3 rows)
> >
> > two patches with different output,
> > overall we should merge 0002 and 0003?
>
> Looks like I ordered the patches wrong.  I'd like to commit the two separately.
>
> >     if (jsexpr->on_error &&
> >         jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> >         (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > we can be simplified as
> >       if ( jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> >          (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > since if jsexpr->on_error is NULL, then segfault will appear at the beginning of
> > ExecInitJsonExpr
>
> Ok, done.
>
> > + *
> > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > + * domain type to check the constraints, if any.
> >   */
> > + jsestate->jump_error = state->steps_len;
> >   if (jsexpr->on_error &&
> > - jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR)
> > + jsexpr->on_error->btype != JSON_BEHAVIOR_ERROR &&
> > + (jsexpr->on_error->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > + *
> > + * Only add the extra steps for a NULL-valued expression when RETURNING a
> > + * domain type to check the constraints, if any.
> >   */
> > + jsestate->jump_empty = state->steps_len;
> >   if (jsexpr->on_empty != NULL &&
> > - jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR)
> > + jsexpr->on_empty->btype != JSON_BEHAVIOR_ERROR &&
> > + (jsexpr->on_empty->btype != JSON_BEHAVIOR_NULL || returning_domain))
> >
> > I am a little bit confused with the comments.
> > not sure the "NULL-valued expression" refers to.
>
> It refers to on_error/on_empty->expr, which is a Const node encoding
> the NULL (constisnull is true) for that behavior.
>
> That's actually the case also for behaviors UNKNOWN and EMPTY, so the
> condition should actually be checking whether the expr is a
> NULL-valued expression.
>
> I've updated the patch.
>
> > i think it is:
> > implicitly default for ON EMPTY | ERROR clause is NULL (JSON_BEHAVIOR_NULL)
> > for that situation, we can skip the json coercion process,
> > but this only applies when the returning type of JsonExpr is not domain,
>
> I've reworded the comment to mention that this speeds up the default
> ON ERROR / EMPTY handling for JSON_QUERY() and JSON_VALUE().
>
> Plan to push these tomorrow.

Pushed.

-- 
Thanks, Amit Langote

Commits

  1. SQL/JSON: Avoid initializing unnecessary ON ERROR / ON EMPTY steps

  2. SQL/JSON: Fix default ON ERROR behavior for JSON_TABLE

  3. SQL/JSON: Fix JSON_TABLE() column deparsing

  4. Update comment about ExprState.escontext

  5. SQL/JSON: Fix casting for integer EXISTS columns in JSON_TABLE

  6. SQL/JSON: Some fixes to JsonBehavior expression casting

  7. SQL/JSON: Remove useless code in ExecInitJsonExpr()

  8. SQL/JSON: Respect OMIT QUOTES when RETURNING domains over jsonb

  9. SQL/JSON: Improve error-handling of JsonBehavior expressions

  10. SQL/JSON: Fix error-handling of some JsonBehavior expressions

  11. SQL/JSON: Rethink c2d93c3802b

  12. SQL/JSON: Always coerce JsonExpr result at runtime

  13. SQL/JSON: Fix coercion of constructor outputs to types with typmod