On Thu, Jun 27, 2024 at 6:57 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Wed, Jun 26, 2024 at 11:46 PM jian he <jian.universality@gmail.com> wrote:
> > On Wed, Jun 26, 2024 at 8:39 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > >
> > > >
> > > > 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.
> > >
> >
> > SELECT JSON_VALUE(jsonb '111', '$' RETURNING queryfuncs_char2 default
> > '13' on error);
> > return
> > ERROR: value too long for type character(2)
> > should return 13
> >
> > I found out the source of the problem is in coerceJsonExprOutput
> > /*
> > * Use cast expression for domain types; we need CoerceToDomain here.
> > */
> > if (get_typtype(returning->typid) != TYPTYPE_DOMAIN)
> > {
> > jsexpr->use_io_coercion = true;
> > return;
> > }
>
> Thanks for this test case and the analysis. Yes, using a cast
> expression for coercion to the RETURNING type generally seems to be a
> source of many problems that could've been solved by fixing things so
> that only use_io_coercion and use_json_coercion are enough to handle
> all the cases.
>
> I've attempted that in the attached 0001, which removes
> JsonExpr.coercion_expr and a bunch of code around it.
>
> 0002 is now the original patch minus the changes to make
> JSON_EXISTS(), JSON_QUERY(), and JSON_VALUE() behave as we would like,
> because the changes in 0001 covers them. The changes for JsonBehavior
> expression coercion as they were in the last version of the patch are
> still needed, but I decided to move those into 0001 so that the
> changes for query functions are all in 0001 and those for constructors
> in 0002. It would be nice to get rid of that coerce_to_target_type()
> call to coerce the "behavior expression" to RETURNING type, but I'm
> leaving that as a task for another day.
Updated 0001 to remove outdated references, remove some more unnecessary code.
--
Thanks, Amit Langote