Re: SQL/JSON features for v15

Hi Nikita,

On Thu, Aug 18, 2022 at 12:46 PM Nikita Glukhov <n.gluhov@postgrespro.ru> wrote:
> The desciprion of the v7 patches:
>
> 0001 Simplify JsonExpr execution
>  Andres's changes + mine:
>   - Added JsonCoercionType enum, fields like via_io replaced with it
>   - Emit only context item steps in JSON_TABLE_OP case
>   - Skip coercion of NULLs to non-domain types (is it correct?)

I like the parser changes to add JsonCoercionType, because that makes
ExecEvalJsonExprCoercion() so much simpler to follow.

In coerceJsonExpr():

+   if (!allow_io_coercion)
+       return NULL;
+

Might it make more sense to create a JsonCoercion even in this case
and assign it the type JSON_COERCION_ERROR, rather than allow the
coercion to be NULL and doing the following in ExecInitExprRec():

+                       if (!*coercion)
+                           /* Missing coercion here means missing cast */
+                           cstate->type = JSON_COERCION_ERROR;

Likewise in transformJsonFuncExpr():

+               if (coercion_expr != (Node *) placeholder)
+               {
+                   jsexpr->result_coercion = makeNode(JsonCoercion);
+                   jsexpr->result_coercion->expr = coercion_expr;
+                   jsexpr->result_coercion->ctype = JSON_COERCION_VIA_EXPR;
+               }

How about creating a JSON_COERCION_NONE coercion in the else block of
this, just like coerceJsonExpr() does?

Related to that, the JSON_EXISTS_OP block in
ExecEvalJsonExprInternal() sounds to assume that result_coercion would
always be non-NULL, per the comment in the last line:

        case JSON_EXISTS_OP:
            {
                bool        exists = JsonPathExists(item, path,
                                                    jsestate->args,
                                                    error);

                *resnull = error && *error;
                res = BoolGetDatum(exists);
                break;  /* always use result coercion */
            }

...but it won't be if the above condition is false?

> 0002 Fix returning of json[b] domains in JSON_VALUE:
>   simply rebase of v6 onto 0001

Especially after seeing the new comments in this one, I'm wondering if
it makes sense to rename result_coercion to, say, default_coercion?

> 0003 Add EEOP_SUBTRANS executor step
>   v6 + new recursive JIT
>
> 0004 Split JsonExpr execution into steps
>   simply rebase of v6 + used LLMBuildSwitch() in EEOP_JSONEXPR

Will need to spend more time looking at these.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com