Subject: Re: ON ERROR in json_query and the like

From: Pavel Stehule <[email-masked]>
Date: Fri, 21 Jun 2024 06:39:48 +0200

pá 21. 6. 2024 v 6:01 odesílatel Amit Langote <[email-masked]>
napsal:

> On Fri, Jun 21, 2024 at 10:01 AM David G. Johnston
> <[email-masked]> wrote:
> > On Thu, Jun 20, 2024 at 5:22 PM Amit Langote <[email-masked]>
> wrote:
> >>
> >>
> >> Soft error handling *was* used for catching cast errors in the very
> >> early versions of this patch (long before I got involved and the
> >> infrastructure you mention got added).  It was taken out after Pavel
> >> said [1] that he didn't like producing NULL instead of throwing an
> >> error.  Not sure if Pavel's around but it would be good to know why he
> >> didn't like it at the time.
> >>
> >
> > I'm personally in the "make it error" camp but "make it conform to the
> standard" is a stronger membership (in general).
> >
> > I see this note in your linked thread:
> >
> > > By the standard, it is implementation-defined whether JSON parsing
> errors
> > > should be caught by ON ERROR clause.
> >
> > Absent someone contradicting that claim I retract my position here and
> am fine with failing if these "functions" are supplied with something that
> cannot be cast to json.  I'd document them like functions that accept json
> with the implications that any casting to json happens before the function
> is called and thus its arguments do not apply to that step.
>
> Thanks for that clarification.
>
> So, there are the following options:
>
> 1. Disallow anything but jsonb for context_item (the patch I posted
> yesterday)
>
> 2. Continue allowing context_item to be non-json character or utf-8
> encoded bytea strings, but document that any parsing errors do not
> respect the ON ERROR clause.
>
> 3. Go ahead and fix implicit casts to jsonb so that any parsing errors
> respect ON ERROR (no patch written yet).
>
> David's vote seems to be 2, which is my inclination too.  Markus' vote
> seems to be either 1 or 3.  Anyone else?
>

@3 can be possibly messy (although be near Oracle or standard). I don't
think it is safe - one example '{a:10}' is valid for Oracle, but not for
Postgres, and using @3 impacts different results (better to raise an
exception).

The effect of @1 and @2 is similar - @1 is better so the user needs to
explicitly cast, so maybe it is cleaner, so the cast should not be handled,
@2 is more user friendly, because it accepts unknown string literal. From a
developer perspective I prefer @1, from a user perspective I prefer @2.
Maybe @2 is a good compromise.

Regards

Pavel

> --
> Thanks, Amit Langote

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 20 Jun 2024 21:46:43 -0700

> Maybe @2 is a good compromise.

2 also has the benefit of being standard conforming while 1 does not.

3 is also conforming and I wouldn’t object to it had we already done it
that way.

But since 2 is conforming too and implemented, and we are in beta, I'm
thinking we need to go with this option.

David J.

---

From: Markus Winand <[email-masked]>
Date: Fri, 21 Jun 2024 07:28:22 +0200

> On 21.06.2024, at 06:46, David G. Johnston <[email-masked]> wrote:
>> 
>> pá 21. 6. 2024 v 6:01 odesílatel Amit Langote <[email-masked]> napsal:
>> > > By the standard, it is implementation-defined whether JSON parsing errors
>> > Absent someone contradicting that claim I retract my position here and am fine with failing if these "functions" are supplied with something that cannot be cast to json.  I'd document them like functions that accept json with the implications that any casting to json happens before the function is called and thus its arguments do not apply to that step.
>> 
>> 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)
>> 

With a very strong preference of 3.

>> 
>> @3 can be possibly messy (although be near Oracle or standard). I don't think it is safe - one example '{a:10}' is valid for Oracle, but not for Postgres, and using @3 impacts different results (better to raise an exception).

The question of what is valid JSON is a different question, I guess. My original report is about something that is invalid everywhere. Having that in line would be a start. Also I believe Oracle’s habit to accept unquoted object keys is not covered by the standard (unless defined as a JSON format and also explicitly using the corresponding FORMAT clause).

>> The effect of @1 and @2 is similar - @1 is better so the user needs to explicitly cast, so maybe it is cleaner, so the cast should not be handled, @2 is more user friendly, because it accepts unknown string literal. From a developer perspective I prefer @1, from a user perspective I prefer @2. Maybe @2 is a good compromise.
> 

Why do you think so? Do you have any references or is this just based on previous statements in this discussion?

-markus

---

From: "David G. Johnston" <[email-masked]>
Date: Thu, 20 Jun 2024 22:38:24 -0700

> previous statements in this discussion?
Hearsay.

https://www.postgresql.org/message-id/CAFj8pRCnzO2cnHi5ebXciV%3DtuGVvAQOW9uPU%2BDQV1GkL31R%3D-g%40mail.gmail.com

> 4) If ALREADY PARSED is False, then it is implementation-defined whether
the
> following rules are applied:
> a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied
with
> JT as JSON TEXT, an implementation-defined <JSON key uniqueness
constraint>
> as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS
and
> let CISJI be the SQL/JSON ITEM returned from the application of those
> General Rules.
> b) If ST is not successful completion, then ST is returned as the STATUS
of
> this application of these General Rules, and no further General Rules of
> this Subclause are applied.

But maybe I’m mis-interpreting that snippet and Nikita’s related commentary
regarding have chosen between options for this implementation-defined
feature.

David j.

---

From: Markus Winand <[email-masked]>
Date: Fri, 21 Jun 2024 12:59:12 +0200

> On 21.06.2024, at 07:38, David G. Johnston <[email-masked]> wrote:
> 
> > 4) If ALREADY PARSED is False, then it is implementation-defined whether the
> > a) The General Rules of Subclause 9.36, "Parsing JSON text", are applied with
> > JT as JSON TEXT, an implementation-defined <JSON key uniqueness constraint>
> > as UNIQUENESS CONSTRAINT, and FO as FORMAT OPTION; let ST be the STATUS and
> > b) If ST is not successful completion, then ST is returned as the STATUS of
> But maybe I’m mis-interpreting that snippet and Nikita’s related commentary regarding have chosen between options for this implementation-defined feature.

Ah, here we go. Nowadays this is called IA050, “Whether a JSON context item that is not of the JSON data type is parsed.” (Likewise IA054 “Whether a JSON parameter is parsed.”)

> 1. Disallow anything but jsonb for context_item (the patch I posted yesterday)

* Non-conforming
* patch available

> respect the ON ERROR clause.

* Conforming by choosing IA050 to implement GR4: raise errors independent of the ON ERROR clause.
* currently committed.

> respect ON ERROR (no patch written yet).

* Conforming by choosing IA050 not to implement GR4: Parsing happens later, considering the ON ERROR clause.
* no patch available, not trivial

I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (how could I miss that?), proper documentation is a very tempting option.

-markus
ps: Does anyone know a dialect that implements GR4?

---

From: Amit Langote <[email-masked]>
Date: Sat, 22 Jun 2024 17:43:05 +0900

Hi,

Thanks all for chiming in.

> I guess I’m the only one in favour of 3 ;) My remaining arguments are that Oracle and Db2 (LUW) do it that way and also that it is IMHO what users would expect. However, as 2 is also conforming (ho…

So, we should go with 2 for v17, because while 3 may be very
appealing, there's a risk that it might not get done in the time
remaining for v17.

I'll post the documentation patch on Monday.

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Wed, 26 Jun 2024 21:10:09 +0900

Hi,

> I'll post the documentation patch on Monday.

Here's that patch, which adds this note after table 9.16.3. SQL/JSON
Query Functions:

+  <note>
+   <para>
+    The <replaceable>context_item</replaceable> expression is converted to
+    <type>jsonb</type> by an implicit cast if the expression is not already of
+    type <type>jsonb</type>. Note, however, that any parsing errors that occur
+    during that conversion are thrown unconditionally, that is, are not
+    handled according to the (specified or implicit) <literal>ON
ERROR</literal>
+    clause.
+   </para>
+  </note>

Peter, sorry about the last-minute ask, but do you have any
thoughts/advice on conformance as discussed above?

--
Thanks, Amit Langote

---

From: Amit Langote <[email-masked]>
Date: Fri, 28 Jun 2024 09:49:45 +0900

> +    handled according to the (specified or implicit) <literal>ON

I have pushed this.

-- 
Thanks, Amit Langote