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

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.

{
...
    /*
     * 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?


        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?



While reviewing it, I found another minor issue.


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");

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.