Horton

Re: pgsql: Add more SQL/JSON constructor functions

Amit Langote <amitlangote09@gmail.com>

View thread View on postgr.es
From: Amit Langote <amitlangote09@gmail.com>
To: jian he <jian.universality@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Alvaro Herrera <alvherre@alvh.no-ip.org>, Peter Eisentraut <peter@eisentraut.org>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Date: 2024-07-23T12:52:39Z
Lists: pgsql-hackers

Attachments

On Tue, Jul 23, 2024 at 11:45 AM jian he <jian.universality@gmail.com> wrote:
> On Mon, Jul 22, 2024 at 4:46 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >
> > On Thu, Jul 18, 2024 at 3:04 PM jian he <jian.universality@gmail.com> wrote:
> > > we still have problem in transformJsonBehavior
> > >
> > > currently transformJsonBehavior:
> > > SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3)  DEFAULT 010111 ON ERROR);
> > > ERROR:  cannot cast behavior expression of type text to bit
> > > LINE 1: ...VALUE(jsonb '1234', '$' RETURNING bit(3)  DEFAULT 010111 ON ...
> > >
> > > here, 010111 will default to int4, so "cannot cast behavior expression
> > > of type text to bit"
> > > is wrong?
> > > also int4/int8 can be explicitly cast to bit(3), in this case, it
> > > should return 111.
> >
> > I think we shouldn't try too hard in the code to "automatically" cast
> > the DEFAULT expression, especially if that means having to add special
> > case code for all sorts of source-target-type combinations.
> >
> > I'm inclined to just give a HINT to the user to cast the DEFAULT
> > expression by hand, because they *can* do that with the syntax that
> > exists.
>
> select typname, typinput, pg_get_function_identity_arguments(typinput)
> from pg_type pt join pg_proc proc  on proc.oid = pt.typinput
> where typtype = 'b' and typarray <> 0 and proc.pronargs > 1;
>
> As you can see from the query result, we only need to deal with bit
> and character type
> in this context.
>
> SELECT JSON_VALUE(jsonb '1234', '$.a' RETURNING bit(3) DEFAULT 10111 ON empty);
> SELECT JSON_VALUE(jsonb '1234', '$.a' RETURNING char(3) DEFAULT 10111
> ON empty) ;
>
> the single quote literal ', no explicit cast, resolve to text type.
> no single quote like 11, no explicit cast, resolve to int type.
> we actually can cast int to bit, also have pg_cast entry.
> so the above these 2 examples should behave the same, given there is
> no pg_cast entry for int to text.
>
> select castsource::regtype ,casttarget::regtype ,castfunc,castcontext,
> castmethod
> from pg_cast where 'int'::regtype in (castsource::regtype ,casttarget::regtype);
>
> but i won't insist on it, since bit/varbit don't use that much.

The cast from int to bit that exists in pg_cast is only good for
explicit casts, so would truncate user's value instead of flagging it
as invalid input, and this whole discussion is about not doing that.
With the DEFAULT expression specified or interpreted as a text string,
we don't have that problem because we can then use CoerceViaIO as an
assignment-level cast, whereby the invalid input *is* flagged as it
should, like this:

SELECT JSON_VALUE(jsonb '1234', '$' RETURNING bit(3)  DEFAULT '11111' ON ERROR);
ERROR:  bit string length 5 does not match type bit(3)

So it seems fair to me to flag it when the user specifies an integer
in DEFAULT we can't create a cast expression that does not truncate a
value to fit the RETURNING type.

> > I'm planning to push the attached 2 patches.  0001 is to fix
> > transformJsonBehavior() for these cases and 0002 to adjust the
> > behavior of casting the result of JSON_EXISTS() and EXISTS columns to
> > integer type.  I've included the tests in your patch in 0001.  I
> > noticed using cast expression to coerce the boolean constants to
> > fixed-length types would produce unexpected errors when the planner's
> > const-simplification calls the cast functions.  So in 0001, I've made
> > that case also use runtime coercion using json_populate_type().
> >
>
> +  <note>
> +   <para>
> +    If an <literal>ON ERROR</literal> or <literal>ON EMPTY</literal>
> +    expression can't be coerced to the <literal>RETURNING</literal> type
> +    successfully, an SQL NULL value will be returned.
> +   </para>
> +  </note>
>
> I think this change will have some controversy.

On second thought, I agree.  I've made some changes to *throw* the
error when the JsonBehavior values fail being coerced to the RETURNING
type.  Please check the attached.

In the attached patch, I've also taken care of the problem mentioned
in your latest email -- the solution I've chosen is not to produce the
error when ERROR ON ERROR is specified but to use runtime coercion
also for the jsonb type or any type that is not integer.  Also fixed
the typos.

Thanks for your attention!

--
Thanks, Amit Langote

Commits

  1. SQL/JSON: Avoid initializing unnecessary ON ERROR / ON EMPTY steps

  2. SQL/JSON: Fix default ON ERROR behavior for JSON_TABLE

  3. SQL/JSON: Fix JSON_TABLE() column deparsing

  4. Update comment about ExprState.escontext

  5. SQL/JSON: Fix casting for integer EXISTS columns in JSON_TABLE

  6. SQL/JSON: Some fixes to JsonBehavior expression casting

  7. SQL/JSON: Remove useless code in ExecInitJsonExpr()

  8. SQL/JSON: Respect OMIT QUOTES when RETURNING domains over jsonb

  9. SQL/JSON: Improve error-handling of JsonBehavior expressions

  10. SQL/JSON: Fix error-handling of some JsonBehavior expressions

  11. SQL/JSON: Rethink c2d93c3802b

  12. SQL/JSON: Always coerce JsonExpr result at runtime

  13. SQL/JSON: Fix coercion of constructor outputs to types with typmod