Re: remaining sql/json patches

From: Peter Eisentraut <peter(at)eisentraut(dot)org>
To: Amit Langote <amitlangote09(at)gmail(dot)com>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Andrew Dunstan <andrew(at)dunslane(dot)net>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, jian he <jian(dot)universality(at)gmail(dot)com>, Erik Rijkers <er(at)xs4all(dot)nl>
Subject: Re: remaining sql/json patches
Date: 2023-11-21 07:09:17
Message-ID: 1d211324-4a9c-4129-b2bf-d4b754167aef@eisentraut.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

I looked a bit at the parser additions, because there were some concerns
expressed that they are quite big.

It looks like the parser rules were mostly literally copied from the BNF
in the SQL standard. That's probably a reasonable place to start, but
now at the end, there is some room for simplification.

Attached are a few patches that apply on top of the 0003 patch. (I
haven't gotten to 0004 in detail yet.) Some explanations:

0001-Put-keywords-in-right-order.patch

This is just an unrelated cleanup.

0002-Remove-js_quotes-union-entry.patch

We usually don't want to put every single node type into the gram.y
%union. This one can be trivially removed.

0003-Move-some-code-from-gram.y-to-parse-analysis.patch

Code like this can be postponed to parse analysis, keeping gram.y
smaller. The error pointer loses a bit of precision, but I think that's
ok. (There is similar code in your 0004 patch, which could be similarly
moved.)

0004-Remove-JsonBehavior-stuff-from-union.patch

Similar to my 0002. This adds a few casts as a result, but that is the
typical style in gram.y.

0005-Get-rid-of-JsonBehaviorClause.patch

I think this two-level wrapping of the behavior clauses is both
confusing and overkill. I was trying to just list the on-empty and
on-error clauses separately in the top-level productions (JSON_VALUE
etc.), but that led to shift/reduce errors. So the existing rule
structure is probably ok. But we don't need a separate node type just
to combine two values and then unpack them again shortly thereafter. So
I just replaced all this with a list.

0006-Get-rid-of-JsonCommon.patch

This is an example where the SQL standard BNF is not sensible to apply
literally. I moved those clauses up directly into their callers, thus
removing one intermediate levels of rules and also nodes. Also, the
path name (AS name) stuff is only for JSON_TABLE, so it's not needed in
this patch. I removed it here, but it would have to be readded in your
0004 patch.

Another thing: In your patch, JSON_EXISTS has a RETURNING clause
(json_returning_clause_opt), but I don't see that in the standard, and
also not in the Oracle or Db2 docs. Where did this come from?

With these changes, I think the grammar complexity in your 0003 patch is
at an acceptable level. Similar simplification opportunities exist in
the 0004 patch, but I haven't worked on that yet. I suggest that you
focus on getting 0001..0003 committed around this commit fest and then
deal with 0004 in the next one. (Also split up the 0005 patch into the
pieces that apply to 0003 and 0004, respectively.)

Attachment Content-Type Size
0001-Put-keywords-in-right-order.patch.nocfbot text/plain 842 bytes
0002-Remove-js_quotes-union-entry.patch.nocfbot text/plain 1.2 KB
0003-Move-some-code-from-gram.y-to-parse-analysis.patch.nocfbot text/plain 3.6 KB
0004-Remove-JsonBehavior-stuff-from-union.patch.nocfbot text/plain 4.4 KB
0005-Get-rid-of-JsonBehaviorClause.patch.nocfbot text/plain 6.8 KB
0006-Get-rid-of-JsonCommon.patch.nocfbot text/plain 8.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Laurenz Albe 2023-11-21 07:42:42 Re: Use of backup_label not noted in log
Previous Message Jeff Davis 2023-11-21 06:37:47 Re: simplehash: SH_OPTIMIZE_REPEAT for optimizing repeated lookups of the same key