Horton

Re: remaining sql/json patches

Alvaro Herrera <alvherre@alvh.no-ip.org>

View thread
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Amit Langote <amitlangote09@gmail.com>
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-06T15:26:21Z
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.

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.

> 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.


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.

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).  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.

(I didn't look at the subsequent patches in the series to see if these
things were fixed later.)

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/