Re: remaining sql/json patches

On Thu, Dec 7, 2023 at 12:26 AM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> On 2023-Dec-06, Amit Langote wrote:
> > I think I'm inclined toward adapting the LA-token fix (attached 0005),
> > because we've done that before with SQL/JSON constructors patch.
> > Also, if I understand the concerns that Tom mentioned at [1]
> > correctly, maybe we'd be better off not assigning precedence to
> > symbols as much as possible, so there's that too against the approach
> > #1.
>
> Sounds ok to me, but I'm happy for this decision to be overridden by
> others with more experience in parser code.

OK, I'll wait to hear from others.

> > Also I've attached 0006 to add news tests under ECPG for the SQL/JSON
> > query functions, which I haven't done so far but realized after you
> > mentioned ECPG. It also includes the ECPG variant of the LA-token
> > fix. I'll eventually merge it into 0003 and 0004 after expanding the
> > test cases some more. I do wonder what kinds of tests we normally add
> > to ECPG suite but not others?
>
> Well, I only added tests to the ecpg suite in the previous round of
> SQL/JSON deeds because its grammar was being modified, so it seemed
> possible that it'd break. Because you're also going to modify its
> parser.c, it seems reasonable to expect tests to be added. I wouldn't
> expect to have to do this for other patches, because it should behave
> like straight SQL usage.

Ah, ok, so ecpg tests are only needed in the JSON_TABLE patch.

> Looking at 0002 I noticed that populate_array_assign_ndims() is called
> in some places and its return value is not checked, so we'd ultimately
> return JSON_SUCCESS even though there's actually a soft error stored
> somewhere. I don't know if it's possible to hit this in practice, but
> it seems odd.

Indeed, fixed. I think I missed the callbacks in JsonSemAction
because I only looked at functions directly reachable from
json_populate_record() or something.

> Looking at get_json_object_as_hash(), I think its comment is not
> explicit enough about its behavior when an error is stored in escontext,
> so its hard to judge whether its caller is doing the right thing (I
> think it is).

I've modified get_json_object_as_hash() to return NULL if
pg_parse_json_or_errsave() returns false because of an error. Maybe
that's an overkill but that's at least a bit clearer than a hash table
of indeterminate state. Added a comment too.

> OTOH, populate_record seems to have the same issue, but
> callers of that definitely seem to be doing the wrong thing -- namely,
> not checking whether an error was saved; particularly populate_composite
> seems to rely on the returned tuple, even though an error might have
> been reported.

Right, populate_composite() should return NULL after checking escontext. Fixed.

On Thu, Dec 7, 2023 at 12:11 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> typo:
> + * If a soft-error occurs, it will be checked by EEOP_JSONEXPR_COECION_FINISH

Fixed.

> json_exists no RETURNING clause.
> so the following part in src/backend/parser/parse_expr.c can be removed?
>
> + else if (jsexpr->returning->typid != BOOLOID)
> + {
> + Node *coercion_expr;
> + CaseTestExpr *placeholder = makeNode(CaseTestExpr);
> + int location = exprLocation((Node *) jsexpr);
> +
> + /*
> + * We abuse CaseTestExpr here as placeholder to pass the
> + * result of evaluating JSON_EXISTS to the coercion
> + * expression.
> + */
> + placeholder->typeId = BOOLOID;
> + placeholder->typeMod = -1;
> + placeholder->collation = InvalidOid;
> +
> + coercion_expr =
> + coerce_to_target_type(pstate, (Node *) placeholder, BOOLOID,
> + jsexpr->returning->typid,
> + jsexpr->returning->typmod,
> + COERCION_EXPLICIT,
> + COERCE_IMPLICIT_CAST,
> + location);
> +
> + if (coercion_expr == NULL)
> + ereport(ERROR,
> + (errcode(ERRCODE_CANNOT_COERCE),
> + errmsg("cannot cast type %s to %s",
> + format_type_be(BOOLOID),
> + format_type_be(jsexpr->returning->typid)),
> + parser_coercion_errposition(pstate, location, (Node *) jsexpr)));
> +
> + if (coercion_expr != (Node *) placeholder)
> + jsexpr->result_coercion = coercion_expr;
> + }

This is needed in the JSON_TABLE patch as explained in [1]. Moved
this part into patch 0004.

> Similarly, since JSON_EXISTS has no RETURNING clause, the following
> also needs to be refactored?
>
> + /*
> + * Disallow FORMAT specification in the RETURNING clause of JSON_EXISTS()
> + * and JSON_VALUE().
> + */
> + if (func->output &&
> + (func->op == JSON_VALUE_OP || func->op == JSON_EXISTS_OP))
> + {
> + JsonFormat *format = func->output->returning->format;
> +
> + if (format->format_type != JS_FORMAT_DEFAULT ||
> + format->encoding != JS_ENC_DEFAULT)
> + ereport(ERROR,
> + (errcode(ERRCODE_SYNTAX_ERROR),
> + errmsg("cannot specify FORMAT in RETURNING clause of %s",
> + func->op == JSON_VALUE_OP ? "JSON_VALUE()" :
> + "JSON_EXISTS()"),
> + parser_errposition(pstate, format->location)));

This one needs to be fixed, so done.

On Thu, Dec 7, 2023 at 5:25 PM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
> Here are a couple of small patches to tidy up the parser a bit in your
> v28-0004 (JSON_TABLE) patch. It's not a lot; the rest looks okay to me.

Thanks Peter. I've merged these into 0004.

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

[1] https://www.postgresql.org/message-id/CA%2BHiwqGsByGXLUniPxBgZjn6PeDr0Scp0jxxQOmBXy63tiJ60A%40mail.gmail.com