Re: pgsql: Add more SQL/JSON constructor functions - Mailing list pgsql-hackers

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?