Re: pgsql: Add more SQL/JSON constructor functions

Hi,

Thanks for taking a look.

On Fri, Jun 21, 2024 at 4:05 PM jian he <jian(dot)universality(at)gmail(dot)com> wrote:
> 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.

The RETURNING variant giving an error is what the standard asks us to
do apparently. I read Tom's last message on this thread as agreeing
to that, even though hesitantly. He can correct me if I got that
wrong.

> 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.

Interesting examples, thanks for sharing.

Attached updated version should take into account that typmod may be
hiding under domains. Please test.

> ------------------------------------------
> 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))

Yeah, perhaps, but let's consider this independently please.

--
Thanks, Amit Langote