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-08-22T03:44:33Z
Lists: pgsql-hackers 
On Thu, Aug 22, 2024 at 11:02 jian he <jian.universality@gmail.com> wrote:

> On Tue, Jul 30, 2024 at 12:59 PM Amit Langote <amitlangote09@gmail.com>
> wrote:
> >
> > Hi,
> >
> > On Fri, Jul 26, 2024 at 11:19 PM jian he <jian.universality@gmail.com>
> wrote:
> > > On Fri, Jul 26, 2024 at 4:53 PM Amit Langote <amitlangote09@gmail.com>
> wrote:
> > > >
> > > >
> > > > Pushed 0003-0005 ahead of 0001-0002.  Will try to push them over the
> > > > weekend.  Rebased for now.
> >
> > Pushed them now.
> >
> > > {
> > > ...
> > >     /*
> > >      * For expression nodes that support soft errors.  Should be set
> to NULL
> > >      * before calling ExecInitExprRec() if the caller wants errors
> thrown.
> > >      */
> > >     ErrorSaveContext *escontext;
> > > } ExprState;
> > >
> > > i believe by default makeNode will set escontext to NULL.
> > > So the comment should be, if you want to catch the soft errors, make
> > > sure the escontext pointing to an allocated ErrorSaveContext.
> > > or maybe just say, default is NULL.
> > >
> > > Otherwise, the original comment's meaning feels like: we need to
> > > explicitly set it to NULL
> > > for certain operations, which I believe is false?
> >
> > OK, I'll look into updating this.
> >
> > >         struct
> > >         {
> > >             Oid            targettype;
> > >             int32        targettypmod;
> > >             bool        omit_quotes;
> > >             bool        exists_coerce;
> > >             bool        exists_cast_to_int;
> > >             bool        check_domain;
> > >             void       *json_coercion_cache;
> > >             ErrorSaveContext *escontext;
> > >         }            jsonexpr_coercion;
> > > do we need to comment that "check_domain" is only used for
> JSON_EXISTS_OP?
> >
> > I've renamed it to exists_check_domain and added a comment that
> > exists_* fields are relevant only for JSON_EXISTS_OP.
> >
> > > json_behavior_type:
> > >             ERROR_P        { $$ = JSON_BEHAVIOR_ERROR; }
> > >             | NULL_P    { $$ = JSON_BEHAVIOR_NULL; }
> > >             | TRUE_P    { $$ = JSON_BEHAVIOR_TRUE; }
> > >             | FALSE_P    { $$ = JSON_BEHAVIOR_FALSE; }
> > >             | UNKNOWN    { $$ = JSON_BEHAVIOR_UNKNOWN; }
> > >             | EMPTY_P ARRAY    { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > >             | EMPTY_P OBJECT_P    { $$ = JSON_BEHAVIOR_EMPTY_OBJECT; }
> > >             /* non-standard, for Oracle compatibility only */
> > >             | EMPTY_P    { $$ = JSON_BEHAVIOR_EMPTY_ARRAY; }
> > >         ;
> > >
> > > EMPTY_P behaves the same as EMPTY_P ARRAY
> > > so for function GetJsonBehaviorConst, the following "case
> > > JSON_BEHAVIOR_EMPTY:" is wrong?
> > >
> > >         case JSON_BEHAVIOR_NULL:
> > >         case JSON_BEHAVIOR_UNKNOWN:
> > >         case JSON_BEHAVIOR_EMPTY:
> > >             val = (Datum) 0;
> > >             isnull = true;
> > >             typid = INT4OID;
> > >             len = sizeof(int32);
> > >             isbyval = true;
> > >             break;
> > >
> > > also src/backend/utils/adt/ruleutils.c
> > >     if (jexpr->on_error->btype != JSON_BEHAVIOR_EMPTY)
> > >         get_json_behavior(jexpr->on_error, context, "ERROR");
> >
> > Something like the attached makes sense?  While this meaningfully
> > changes the deparsing output, there is no behavior change for
> > JsonTable top-level path execution.  That's because the behavior when
> > there's an error in the execution of the top-level path is to throw it
> > or return an empty set, which is handled in jsonpath_exec.c, not
> > execExprInterp.c.
> >
>
> hi amit.
> seems you forgot to attach the patch?


Yeah, I had planned to look at this after my vacation earlier this month,
but I context switched into working on another project and lost track of
this. I’ll make some time next week to fix whatever remains go be fixed
here. Thanks for the reminder.

>

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