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

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.

> for json_value, json_query, i believe we can save some circles in
> ExecInitJsonExpr
> if you don't specify on error, on empty
>
> can you please check the attached, based on your latest attachment.

Perhaps makes sense, though I haven't checked closely.  I'll take a
look next week.

--
Thanks, Amit Langote