Re: pgsql: Add more SQL/JSON constructor functions

On Tue, Jun 18, 2024 at 5:02 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
>
> On Tue, Jun 4, 2024 at 7:03 PM Amit Langote <amitlangote09(at)gmail(dot)com> wrote:
> > On Tue, Jun 4, 2024 at 2:20 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > Peter Eisentraut <peter(at)eisentraut(dot)org> writes:
> > > > On 02.06.24 21:46, Tom Lane wrote:
> > > >> If you don't
> > > >> like our current behavior, then either you have to say that RETURNING
> > > >> with a length-limited target type is illegal (which is problematic
> > > >> for the spec, since they have no such type) or that the cast behaves
> > > >> like an implicit cast, with errors for overlength input (which I find
> > > >> to be an unintuitive definition for a construct that names the target
> > > >> type explicitly).
> > >
> > > > It asks for the latter behavior, essentially (but it's not defined in
> > > > terms of casts). It says:
> > >
> > > Meh. Who needs consistency? But I guess the answer is to do what was
> > > suggested earlier and change the code to use COERCE_IMPLICIT_CAST.
> >
> > OK, will post a patch to do so in a new thread on -hackers.
>
> Oops, didn't realize that this is already on -hackers.
>
> Attached is a patch to use COERCE_IMPLICIT_CAST when the RETURNING
> type specifies a length limit.
>

hi.
i am a little confused.

here[1] tom says:
> Yeah, I too think this is a cast, and truncation is the spec-defined
> behavior for casting to varchar with a specific length limit. I see
> little reason that this should work differently from
>
> select json_serialize('{"a":1, "a":2}' returning text)::varchar(5);
> json_serialize
> ----------------
> {"a":
> (1 row)

if i understand it correctly, and my english interpretation is fine.
i think tom means something like:

select json_serialize('{"a":1, "a":2}' returning text)::varchar(5) =
json_serialize('{"a":1, "a":2}' returning varchar(5));

should return true.
the master will return true, but apply your patch, the above query
will yield an error.

your patch will make domain and char(n) behavior inconsistent.
create domain char2 as char(2);
SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char2 ERROR ON ERROR);
SELECT JSON_VALUE(jsonb '"aaa"', '$' RETURNING char(2) ERROR ON ERROR);

another example:
SELECT JSON_query(jsonb '"aaa"', '$' RETURNING char(2) keep quotes
default '"aaa"'::jsonb ON ERROR);
same value (jsonb "aaa") error on error will yield error,
but `default expression on error` can coerce the value to char(2),
which looks a little bit inconsistent, I think.

------------------------------------------
current in ExecInitJsonExpr we have

if (jsexpr->coercion_expr)
...
else if (jsexpr->use_json_coercion)
...
else if (jsexpr->use_io_coercion)
...

do you think it is necessary to add following asserts:
Assert (!(jsexpr->coercion_expr && jsexpr->use_json_coercion))
Assert (!(jsexpr->coercion_expr && jsexpr->use_io_coercion))

[1] https://www.postgresql.org/message-id/3189.1717001075%40sss.pgh.pa.us