Re: SQL/JSON revisited

Hi,

On 2023-02-20 16:35:52 +0900, Amit Langote wrote:
> Subject: [PATCH v4 03/10] SQL/JSON query functions
> +/*
> + * Evaluate a JSON error/empty behavior result.
> + */
> +static Datum
> +ExecEvalJsonBehavior(JsonBehavior *behavior, bool *is_null)
> +{
> +    *is_null = false;
> +
> +    switch (behavior->btype)
> +    {
> +        case JSON_BEHAVIOR_EMPTY_ARRAY:
> +            return JsonbPGetDatum(JsonbMakeEmptyArray());
> +
> +        case JSON_BEHAVIOR_EMPTY_OBJECT:
> +            return JsonbPGetDatum(JsonbMakeEmptyObject());
> +
> +        case JSON_BEHAVIOR_TRUE:
> +            return BoolGetDatum(true);
> +
> +        case JSON_BEHAVIOR_FALSE:
> +            return BoolGetDatum(false);
> +
> +        case JSON_BEHAVIOR_NULL:
> +        case JSON_BEHAVIOR_UNKNOWN:
> +            *is_null = true;
> +            return (Datum) 0;
> +
> +        case JSON_BEHAVIOR_DEFAULT:
> +            /* Always handled in the caller. */
> +            Assert(false);
> +            return (Datum) 0;
> +
> +        default:
> +            elog(ERROR, "unrecognized SQL/JSON behavior %d", behavior->btype);
> +            return (Datum) 0;
> +    }
> +}

Does this actually need to be evaluated at expression eavluation time?
Couldn't we just emit the proper constants in execExpr.c?

> +/* ----------------------------------------------------------------
> + *        ExecEvalJson
> + * ----------------------------------------------------------------
> + */
> +void
> +ExecEvalJson(ExprState *state, ExprEvalStep *op, ExprContext *econtext)

Pointless comment.


> +{
> +    JsonExprState *jsestate = op->d.jsonexpr.jsestate;
> +    JsonExprPreEvalState *pre_eval = &jsestate->pre_eval;
> +    JsonExprPostEvalState *post_eval = &jsestate->post_eval;
> +    JsonExpr   *jexpr = jsestate->jsexpr;
> +    Datum        item;
> +    Datum        res = (Datum) 0;
> +    JsonPath   *path;
> +    bool        throwErrors = jexpr->on_error->btype == JSON_BEHAVIOR_ERROR;
> +
> +    *op->resnull = true;        /* until we get a result */
> +    *op->resvalue = (Datum) 0;
> +
> +    item = pre_eval->formatted_expr.value;
> +    path = DatumGetJsonPathP(pre_eval->pathspec.value);
> +
> +    /* Reset JsonExprPostEvalState for this evaluation. */
> +    memset(post_eval, 0, sizeof(*post_eval));
> +
> +    res = ExecEvalJsonExpr(op, econtext, path, item, op->resnull,
> +                           !throwErrors ? &post_eval->error : NULL);
> +
> +    *op->resvalue = res;
> +}

I really don't like having both ExecEvalJson() and ExecEvalJsonExpr(). There's
really no way to know what which version does, just based on the name.


> --- a/src/backend/parser/gram.y
> +++ b/src/backend/parser/gram.y

This stuff adds quite a bit of complexity to the parser. Do we realy need like
a dozen new rules here?


> +json_behavior_empty_array:
> +            EMPTY_P ARRAY    { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> +            /* non-standard, for Oracle compatibility only */
> +            | EMPTY_P        { $$ = makeJsonBehavior(JSON_BEHAVIOR_EMPTY_ARRAY, NULL); }
> +        ;

Do we really want to add random oracle compat crud here?


> +/*
> + * Evaluate a JSON path variable caching computed value.
> + */
> +int
> +EvalJsonPathVar(void *cxt, char *varName, int varNameLen,
> +                JsonbValue *val, JsonbValue *baseObject)

Missing static?

> +{
> +    JsonPathVariableEvalContext *var = NULL;
> +    List       *vars = cxt;
> +    ListCell   *lc;
> +    int            id = 1;
> +
> +    if (!varName)
> +        return list_length(vars);
> +
> +    foreach(lc, vars)
> +    {
> +        var = lfirst(lc);
> +
> +        if (!strncmp(var->name, varName, varNameLen))
> +            break;
> +
> +        var = NULL;
> +        id++;
> +    }
> +
> +    if (!var)
> +        return -1;
> +
> +    /*
> +     * When belonging to a JsonExpr, path variables are computed with the
> +     * JsonExpr's ExprState (var->estate is NULL), so don't need to be computed
> +     * here.  In some other cases, such as when the path variables belonging
> +     * to a JsonTable instead, those variables must be evaluated on their own,
> +     * without the enclosing JsonExpr itself needing to be evaluated, so must
> +     * be handled here.
> +     */
> +    if (var->estate && !var->evaluated)
> +    {
> +        Assert(var->econtext != NULL);
> +        var->value = ExecEvalExpr(var->estate, var->econtext, &var->isnull);
> +        var->evaluated = true;

Uh, so this continues to do recursive expression evaluation, as
ExecEvalJsonExpr()->JsonPathQuery()->executeJsonPath(EvalJsonPathVar)

I'm getting grumpy here. This is wrong, has been pointed out many times. The
only thing that changes is that the point of recursion is moved around.


> +
> +/*
> + * ExecEvalExprSafe
> + *
> + * Like ExecEvalExpr(), though this allows the caller to pass an
> + * ErrorSaveContext to declare its intenion to catch any errors that occur when
> + * executing the expression, such as when calling type input functions that may
> + * be present in it.
> + */
> +static inline Datum
> +ExecEvalExprSafe(ExprState *state,
> +                 ExprContext *econtext,
> +                 bool *isNull,
> +                 Node *escontext,
> +                 bool *error)

Afaict there's no caller of this?


>  
> +/*
> + * ExecInitExprWithCaseValue
> + *
> + * This is the same as ExecInitExpr, except the caller passes the Datum and
> + * bool pointers that it would like the ExprState.innermost_caseval
> + * and ExprState.innermost_casenull, respectively, to be set to.  That way,
> + * it can pass an input value to evaluate the expression via a CaseTestExpr.
> + */
> +ExprState *
> +ExecInitExprWithCaseValue(Expr *node, PlanState *parent,
> +                          Datum *caseval, bool *casenull)
> +{
> +    ExprState  *state;
> +    ExprEvalStep scratch = {0};
> +
> +    /* Special case: NULL expression produces a NULL ExprState pointer */
> +    if (node == NULL)
> +        return NULL;
> +
> +    /* Initialize ExprState with empty step list */
> +    state = makeNode(ExprState);
> +    state->expr = node;
> +    state->parent = parent;
> +    state->ext_params = NULL;
> +    state->innermost_caseval = caseval;
> +    state->innermost_casenull = casenull;
> +
> +    /* Insert EEOP_*_FETCHSOME steps as needed */
> +    ExecInitExprSlots(state, (Node *) node);
> +
> +    /* Compile the expression proper */
> +    ExecInitExprRec(node, state, &state->resvalue, &state->resnull);
> +
> +    /* Finally, append a DONE step */
> +    scratch.opcode = EEOP_DONE;
> +    ExprEvalPushStep(state, &scratch);
> +
> +    ExecReadyExpr(state);
> +
> +    return state;

> +struct JsonTableJoinState
> +{
> +    union
> +    {
> +        struct
> +        {
> +            JsonTableJoinState *left;
> +            JsonTableJoinState *right;
> +            bool        advanceRight;
> +        }            join;
> +        JsonTableScanState scan;
> +    }            u;
> +    bool        is_join;
> +};

A join state that unions the join member with a scan, and has a is_join field?


> +/*
> + * JsonTableInitOpaque
> + *        Fill in TableFuncScanState->opaque for JsonTable processor
> + */
> +static void
> +JsonTableInitOpaque(TableFuncScanState *state, int natts)
> +{
> +    JsonTableContext *cxt;
> +    PlanState  *ps = &state->ss.ps;
> +    TableFuncScan *tfs = castNode(TableFuncScan, ps->plan);
> +    TableFunc  *tf = tfs->tablefunc;
> +    JsonExpr   *ci = castNode(JsonExpr, tf->docexpr);
> +    JsonTableParent *root = castNode(JsonTableParent, tf->plan);
> +    List       *args = NIL;
> +    ListCell   *lc;
> +    int            i;
> +
> +    cxt = palloc0(sizeof(JsonTableContext));
> +    cxt->magic = JSON_TABLE_CONTEXT_MAGIC;
> +
> +    if (ci->passing_values)
> +    {
> +        ListCell   *exprlc;
> +        ListCell   *namelc;
> +
> +        forboth(exprlc, ci->passing_values,
> +                namelc, ci->passing_names)
> +        {
> +            Expr       *expr = (Expr *) lfirst(exprlc);
> +            String       *name = lfirst_node(String, namelc);
> +            JsonPathVariableEvalContext *var = palloc(sizeof(*var));
> +
> +            var->name = pstrdup(name->sval);
> +            var->typid = exprType((Node *) expr);
> +            var->typmod = exprTypmod((Node *) expr);
> +            var->estate = ExecInitExpr(expr, ps);
> +            var->econtext = ps->ps_ExprContext;
> +            var->mcxt = CurrentMemoryContext;
> +            var->evaluated = false;
> +            var->value = (Datum) 0;
> +            var->isnull = true;
> +
> +            args = lappend(args, var);
> +        }
> +    }
> +
> +    cxt->colexprs = palloc(sizeof(*cxt->colexprs) *
> +                           list_length(tf->colvalexprs));
> +
> +    JsonTableInitScanState(cxt, &cxt->root, root, NULL, args,
> +                           CurrentMemoryContext);
> +
> +    i = 0;
> +
> +    foreach(lc, tf->colvalexprs)
> +    {
> +        Expr       *expr = lfirst(lc);
> +
> +        cxt->colexprs[i].expr =
> +            ExecInitExprWithCaseValue(expr, ps,
> +                                      &cxt->colexprs[i].scan->current,
> +                                      &cxt->colexprs[i].scan->currentIsNull);
> +
> +        i++;
> +    }
> +
> +    state->opaque = cxt;
> +}

Evaluating N expressions for a json table isn't a good approach, both memory
and CPU efficiency wise.

Why don't you just emit the proper expression directly, insted of the
CaseTestExpr stuff, that you then separately evaluate?



Greetings,

Andres Freund