Horton

Re: remaining sql/json patches

amit <amitlangote09@gmail.com>

View thread
From: Amit Langote <amitlangote09@gmail.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: jian he <jian.universality@gmail.com>, Andres Freund <andres@anarazel.de>, Andrew Dunstan <andrew@dunslane.net>, Erik Rijkers <er@xs4all.nl>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2023-12-07T09:32:06Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. SQL/JSON: Various improvements to SQL/JSON query function docs

  2. SQL/JSON: Fix some obsolete comments.

  3. SQL/JSON: Fix issues with DEFAULT .. ON ERROR / EMPTY

  4. JSON_TABLE: Add support for NESTED paths and columns

  5. Fix JsonExpr deparsing to emit QUOTES and WRAPPER correctly

  6. Fix typo introduced in 6185c9737

  7. Add basic JSON_TABLE() functionality

  8. Avoid splitting errmsg string to span multiple lines

  9. Add SQL/JSON query functions

  10. Implement various jsonpath methods

  11. Add soft error handling to some expression nodes

  12. Adjust populate_record_field() to handle errors softly

  13. Refactor code used by jsonpath executor to fetch variables

  14. Test EXPLAIN (FORMAT JSON) ... XMLTABLE

  15. Simplify productions for FORMAT JSON [ ENCODING name ]

  16. Add trailing commas to enum definitions

  17. doc: add missing <returnvalue> and whitespace

  18. Add more SQL/JSON constructor functions

  19. Rename a nonterminal used in SQL/JSON grammar

  20. Some refactoring to export json(b) conversion functions

  21. Don't include CaseTestExpr in JsonValueExpr.formatted_expr

  22. Code review for commit b6e1157e7d

  23. Pass constructName to transformJsonValueExpr()

  24. Unify JSON categorize type API and export for external use

  25. Make some indentation in gram.y consistent

  26. Allow most keywords to be used as column labels without requiring AS.

  27. Reduce size of backend scanner's tables.

  28. Use perfect hashing, instead of binary search, for keyword lookup.

Attachments

On Thu, Dec 7, 2023 at 12:26 AM Alvaro Herrera <alvherre@alvh.no-ip.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.universality@gmail.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@eisentraut.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