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