Re: SQL/JSON features for v15

Hi Nikita,

On Thu, Aug 18, 2022 at 12:46 PM Nikita Glukhov <n(dot)gluhov(at)postgrespro(dot)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