Feedback-ID: ia2694551:Fastmail
Date: Wed, 6 Dec 2023 16:26:21 +0100
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>
Subject: Re: remaining sql/json patches
Message-ID: <202312061526.g53ppimnvxxd@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+HiwqET8gAhoCqNx_ueELrs4UevhRMNOYSmq=WykOu9THn=og@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/202312061526.g53ppimnvxxd%40alvherre.pgsql>
Precedence: bulk

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/