Horton

Re: pgsql: Add more SQL/JSON constructor functions

jian he <jian.universality@gmail.com>

View thread View on postgr.es
From: jian he <jian.universality@gmail.com>
To: Amit Langote <amitlangote09@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-07-26T14:19:00Z
Lists: pgsql-hackers

Attachments

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.

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