Horton

Thread

Commits

  1. Fix bugs in polymorphic-argument resolution for multiranges.

  2. Fixes for multirange selectivity estimation

  1. BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    The Post Office <noreply@postgresql.org> — 2021-06-20T21:00:01Z

    The following bug has been logged on the website:

Bug reference:      17066
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 14beta1
Operating system:   Ubuntu 20.04
Description:        

When calling the simple test function:
create function multirange_test(a anycompatiblemultirange)
  returns anycompatible as 'select lower($1);' language sql;

with the NULL argument:
SELECT multirange_test(null);

I get a somewhat unusual error:
ERROR:  cache lookup failed for type 0

The variation with the anycompatiblerange (without multi):
create function range_test(a anycompatiblerange)
  returns anycompatible as 'select lower($1);' language sql;
SELECT range_test(null);

produces:
ERROR:  could not determine polymorphic type anycompatiblerange because
input has type unknown

  2. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Neil Chen <carpenter.nail.cz@gmail.com> — 2021-06-22T07:52:52Z

    Greetings,

I tried to investigate the bug, but the complicated logic here completely
messed up my mind...

Anyway, this patch can fix it and make the regress test happy. But I think
it's better to get the author's advice - I copied this email to Alvaro,
hope it doesn't offend him...


-- 
There is no royal road to learning.
HighGo Software Co.

  3. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2021-06-22T13:42:18Z

    On 2021-Jun-22, Neil Chen wrote:

> Greetings,
> 
> I tried to investigate the bug, but the complicated logic here completely
> messed up my mind...
> 
> Anyway, this patch can fix it and make the regress test happy. But I think
> it's better to get the author's advice - I copied this email to Alvaro,
> hope it doesn't offend him...

Without looking too deeply, the patch seems sensible to me.  However,
I'm CC'ing Alexander and Paul :-)

-- 
Álvaro Herrera       Valdivia, Chile

  4. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-22T16:01:36Z

    On Tue, Jun 22, 2021 at 4:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2021-Jun-22, Neil Chen wrote:
>
> > Greetings,
> >
> > I tried to investigate the bug, but the complicated logic here completely
> > messed up my mind...
> >
> > Anyway, this patch can fix it and make the regress test happy. But I think
> > it's better to get the author's advice - I copied this email to Alvaro,
> > hope it doesn't offend him...
>
> Without looking too deeply, the patch seems sensible to me.  However,
> I'm CC'ing Alexander and Paul :-)

Thank you for pointing this out.  And thanks to Neil for the fix.

Looks good at the first glance.  I'm going to review this in the next
couple of days.

------
Regards,
Alexander Korotkov

  5. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-23T15:31:05Z

    On Tue, Jun 22, 2021 at 7:01 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Tue, Jun 22, 2021 at 4:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > On 2021-Jun-22, Neil Chen wrote:
> >
> > > Greetings,
> > >
> > > I tried to investigate the bug, but the complicated logic here completely
> > > messed up my mind...
> > >
> > > Anyway, this patch can fix it and make the regress test happy. But I think
> > > it's better to get the author's advice - I copied this email to Alvaro,
> > > hope it doesn't offend him...
> >
> > Without looking too deeply, the patch seems sensible to me.  However,
> > I'm CC'ing Alexander and Paul :-)
>
> Thank you for pointing this out.  And thanks to Neil for the fix.
>
> Looks good at the first glance.  I'm going to review this in the next
> couple of days.

I've reviewed enforce_generic_type_consistency() more carefully.  It
looks for me that this function requires more significant rework.  For
instance, these two functions throw errors, but it seems that both of
them should work.

create function multirange_func(r anycompatiblerange)
  returns anycompatiblemultirange as 'select multirange($1);' language sql;
create function range_func(r anycompatiblemultirange)
  returns anycompatiblerange as 'select multirange($1);' language sql;

# select multirange_func(int4range(1,10));
ERROR:  could not identify anycompatiblemultirange type

# select range_func(int4multirange(int4range(1,10)));
ERROR:  could not determine polymorphic type anycompatiblerange
because input has type unknown

So, I'm still investigating this.

------
Regards,
Alexander Korotkov

  6. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-23T22:41:42Z

    On Wed, Jun 23, 2021 at 6:31 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Tue, Jun 22, 2021 at 7:01 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Tue, Jun 22, 2021 at 4:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > On 2021-Jun-22, Neil Chen wrote:
> > >
> > > > Greetings,
> > > >
> > > > I tried to investigate the bug, but the complicated logic here completely
> > > > messed up my mind...
> > > >
> > > > Anyway, this patch can fix it and make the regress test happy. But I think
> > > > it's better to get the author's advice - I copied this email to Alvaro,
> > > > hope it doesn't offend him...
> > >
> > > Without looking too deeply, the patch seems sensible to me.  However,
> > > I'm CC'ing Alexander and Paul :-)
> >
> > Thank you for pointing this out.  And thanks to Neil for the fix.
> >
> > Looks good at the first glance.  I'm going to review this in the next
> > couple of days.
>
> I've reviewed enforce_generic_type_consistency() more carefully.  It
> looks for me that this function requires more significant rework.  For
> instance, these two functions throw errors, but it seems that both of
> them should work.
>
> create function multirange_func(r anycompatiblerange)
>   returns anycompatiblemultirange as 'select multirange($1);' language sql;
> create function range_func(r anycompatiblemultirange)
>   returns anycompatiblerange as 'select multirange($1);' language sql;
>
> # select multirange_func(int4range(1,10));
> ERROR:  could not identify anycompatiblemultirange type
>
> # select range_func(int4multirange(int4range(1,10)));
> ERROR:  could not determine polymorphic type anycompatiblerange
> because input has type unknown
>
> So, I'm still investigating this.

The second function was intended to be this to be a meaningful
example.  But anyway it fails with the same error.

create function range_func(r anycompatiblemultirange)
  returns anycompatiblerange as 'select range_merge($1);' language sql;

------
Regards,
Alexander Korotkov

  7. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-23T23:02:52Z

    On Thu, Jun 24, 2021 at 1:41 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Wed, Jun 23, 2021 at 6:31 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Tue, Jun 22, 2021 at 7:01 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > On Tue, Jun 22, 2021 at 4:42 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > > > On 2021-Jun-22, Neil Chen wrote:
> > > >
> > > > > Greetings,
> > > > >
> > > > > I tried to investigate the bug, but the complicated logic here completely
> > > > > messed up my mind...
> > > > >
> > > > > Anyway, this patch can fix it and make the regress test happy. But I think
> > > > > it's better to get the author's advice - I copied this email to Alvaro,
> > > > > hope it doesn't offend him...
> > > >
> > > > Without looking too deeply, the patch seems sensible to me.  However,
> > > > I'm CC'ing Alexander and Paul :-)
> > >
> > > Thank you for pointing this out.  And thanks to Neil for the fix.
> > >
> > > Looks good at the first glance.  I'm going to review this in the next
> > > couple of days.
> >
> > I've reviewed enforce_generic_type_consistency() more carefully.  It
> > looks for me that this function requires more significant rework.  For
> > instance, these two functions throw errors, but it seems that both of
> > them should work.
> >
> > create function multirange_func(r anycompatiblerange)
> >   returns anycompatiblemultirange as 'select multirange($1);' language sql;
> > create function range_func(r anycompatiblemultirange)
> >   returns anycompatiblerange as 'select multirange($1);' language sql;
> >
> > # select multirange_func(int4range(1,10));
> > ERROR:  could not identify anycompatiblemultirange type
> >
> > # select range_func(int4multirange(int4range(1,10)));
> > ERROR:  could not determine polymorphic type anycompatiblerange
> > because input has type unknown
> >
> > So, I'm still investigating this.
>
> The second function was intended to be this to be a meaningful
> example.  But anyway it fails with the same error.
>
> create function range_func(r anycompatiblemultirange)
>   returns anycompatiblerange as 'select range_merge($1);' language sql;

I've a bit stuck in understanding of
enforce_generic_type_consistency().  There is a set of "argument
declared..." errors.  But they aren't present in regression tests.
And I didn't manage to reproduce them, because a function is filtered
out before entering enforce_generic_type_consistency() and I get just
"No function matches ..." error.

Are "argument declared..." errors just internal, which users shouldn't
normally see.  Or do they happen in some circumstances?  If latter,
then I think it should be added to the regression tests.

I really appreciate a hint here.

------
Regards,
Alexander Korotkov

  8. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-06-23T23:35:10Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> I really appreciate a hint here.

I think I'm to blame for most of that code originally, so I'll take
a look soon.  Been up to my neck in other stuff recently.

			regards, tom lane

  9. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Law <exclusion@gmail.com> — 2021-06-25T04:00:00Z

    Hello,
24.06.2021 02:35, Tom Lane wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
>> I really appreciate a hint here.
> I think I'm to blame for most of that code originally, so I'll take
> a look soon.  Been up to my neck in other stuff recently.			
I'm not sure whether it related to the initial issue, but there is
another anomaly with the multirange types. (May be I should report it as
a distinct bug?) The query:
create table test_multirange(mr int4multirange);
select count(*) from test_multirange where mr << int4range(100,100);
produces:
ERROR:  unexpected operator 4396

while
select count(*) from test_multirange where mr << int4range(100,500);
returns a result (as the multirangetypes test shows).

Best regards,
Alexander

  10. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-25T21:02:24Z

    On Fri, Jun 25, 2021 at 7:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> Hello,
> 24.06.2021 02:35, Tom Lane wrote:
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> >> I really appreciate a hint here.
> > I think I'm to blame for most of that code originally, so I'll take
> > a look soon.  Been up to my neck in other stuff recently.
> I'm not sure whether it related to the initial issue, but there is
> another anomaly with the multirange types. (May be I should report it as
> a distinct bug?) The query:
> create table test_multirange(mr int4multirange);
> select count(*) from test_multirange where mr << int4range(100,100);
> produces:
> ERROR:  unexpected operator 4396
>
> while
> select count(*) from test_multirange where mr << int4range(100,500);
> returns a result (as the multirangetypes test shows).

Yep, that's a distinct bug.  It seems that I've added some missing
operators, but forgot to add some selectivity estimates for them.

I'll come with a fix soon.

------
Regards,
Alexander Korotkov

  11. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-26T23:29:06Z

     On Sat, Jun 26, 2021 at 12:02 AM Alexander Korotkov
<aekorotkov@gmail.com> wrote:
> On Fri, Jun 25, 2021 at 7:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> > Hello,
> > 24.06.2021 02:35, Tom Lane wrote:
> > > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > >> I really appreciate a hint here.
> > > I think I'm to blame for most of that code originally, so I'll take
> > > a look soon.  Been up to my neck in other stuff recently.
> > I'm not sure whether it related to the initial issue, but there is
> > another anomaly with the multirange types. (May be I should report it as
> > a distinct bug?) The query:
> > create table test_multirange(mr int4multirange);
> > select count(*) from test_multirange where mr << int4range(100,100);
> > produces:
> > ERROR:  unexpected operator 4396
> >
> > while
> > select count(*) from test_multirange where mr << int4range(100,500);
> > returns a result (as the multirangetypes test shows).
>
> Yep, that's a distinct bug.  It seems that I've added some missing
> operators, but forgot to add some selectivity estimates for them.
>
> I'll come with a fix soon.

The patch is attached.  It fixes some switches in calc_multirangesel()
and calc_multirangesel().  Additionally, it improves regression test
coverage for matching empty range/multirange.

I'm going to push it if no objection.

------
Regards,
Alexander Korotkov

  12. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-29T20:22:27Z

    On Sun, Jun 27, 2021 at 2:29 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
>  On Sat, Jun 26, 2021 at 12:02 AM Alexander Korotkov
> <aekorotkov@gmail.com> wrote:
> > On Fri, Jun 25, 2021 at 7:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> > > Hello,
> > > 24.06.2021 02:35, Tom Lane wrote:
> > > > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > >> I really appreciate a hint here.
> > > > I think I'm to blame for most of that code originally, so I'll take
> > > > a look soon.  Been up to my neck in other stuff recently.
> > > I'm not sure whether it related to the initial issue, but there is
> > > another anomaly with the multirange types. (May be I should report it as
> > > a distinct bug?) The query:
> > > create table test_multirange(mr int4multirange);
> > > select count(*) from test_multirange where mr << int4range(100,100);
> > > produces:
> > > ERROR:  unexpected operator 4396
> > >
> > > while
> > > select count(*) from test_multirange where mr << int4range(100,500);
> > > returns a result (as the multirangetypes test shows).
> >
> > Yep, that's a distinct bug.  It seems that I've added some missing
> > operators, but forgot to add some selectivity estimates for them.
> >
> > I'll come with a fix soon.
>
> The patch is attached.  It fixes some switches in calc_multirangesel()
> and calc_multirangesel().  Additionally, it improves regression test
> coverage for matching empty range/multirange.
>
> I'm going to push it if no objection.

Pushed!

------
Regards,
Alexander Korotkov

  13. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-06-29T22:06:34Z

    On Thu, Jun 24, 2021 at 2:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > I really appreciate a hint here.
>
> I think I'm to blame for most of that code originally, so I'll take
> a look soon.  Been up to my neck in other stuff recently.

Do I understand correctly that enforce_generic_type_consistency() is
called only after check_generic_type_consistency() returned true?

If so, that means some of the checks are redundant.  Therefore, we can
replace ereport()'s with Assert()'s.

------
Regards,
Alexander Korotkov

  14. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-07-15T22:59:45Z

    On Wed, Jun 30, 2021 at 1:06 AM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Thu, Jun 24, 2021 at 2:35 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > I really appreciate a hint here.
> >
> > I think I'm to blame for most of that code originally, so I'll take
> > a look soon.  Been up to my neck in other stuff recently.
>
> Do I understand correctly that enforce_generic_type_consistency() is
> called only after check_generic_type_consistency() returned true?
>
> If so, that means some of the checks are redundant.  Therefore, we can
> replace ereport()'s with Assert()'s.

I got no feedback on this yet.  And 14beta3 is approaching...

I'm going to write a patch fixing enforce_generic_type_consistency()
while saving its general logic (as far as I can understand it).

------
Regards,
Alexander Korotkov

  15. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-20T21:54:55Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> Do I understand correctly that enforce_generic_type_consistency() is
> called only after check_generic_type_consistency() returned true?
> If so, that means some of the checks are redundant.  Therefore, we can
> replace ereport()'s with Assert()'s.

They are not redundant, IIRC.  I forget the exact mechanism for
reaching them, but it likely has something to do with aggregates
or variadic functions.

In any case, apologies for taking so long to get back to this.  Here's
a proposed patch (based in part on Neil's earlier patch).

			regards, tom lane

  16. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-07-21T10:25:39Z

    On Wed, Jul 21, 2021 at 12:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > Do I understand correctly that enforce_generic_type_consistency() is
> > called only after check_generic_type_consistency() returned true?
> > If so, that means some of the checks are redundant.  Therefore, we can
> > replace ereport()'s with Assert()'s.
>
> They are not redundant, IIRC.  I forget the exact mechanism for
> reaching them, but it likely has something to do with aggregates
> or variadic functions.

If checks aren't redundant, there should be cases when they don't
pass.  It would be nice to identify these cases and add them to the
regression tests.  I didn't manage to do this yet.

> In any case, apologies for taking so long to get back to this.  Here's
> a proposed patch (based in part on Neil's earlier patch).

Thank you!  I'll review this and come back to you.

------
Regards,
Alexander Korotkov

  17. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-21T15:03:51Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Wed, Jul 21, 2021 at 12:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Alexander Korotkov <aekorotkov@gmail.com> writes:
>>> Do I understand correctly that enforce_generic_type_consistency() is
>>> called only after check_generic_type_consistency() returned true?
>>> If so, that means some of the checks are redundant.  Therefore, we can
>>> replace ereport()'s with Assert()'s.

>> They are not redundant, IIRC.  I forget the exact mechanism for
>> reaching them, but it likely has something to do with aggregates
>> or variadic functions.

> If checks aren't redundant, there should be cases when they don't
> pass.  It would be nice to identify these cases and add them to the
> regression tests.  I didn't manage to do this yet.

Hmm ... whether or not there are edge cases where those errors are
reachable, it's certainly true that the mainline case doesn't reach
them:

regression=# create function myadd(anyelement, anyelement) returns anyelement
as 'select $1 + $2' language sql;
CREATE FUNCTION
regression=# select myadd(1, 2.3);
ERROR:  function myadd(integer, numeric) does not exist
LINE 1: select myadd(1, 2.3);
               ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.

That's too bad, because IMO it'd be way more helpful to say
  ERROR:  arguments declared "anyelement" are not all alike
  DETAIL:  integer versus numeric
which is what enforce_generic_type_consistency would say if it
were reached.  Similarly, the other error cases in that code
are far more specific and thus more helpful than simply reporting
that there's no matching function.

I'm tempted to propose that, if there is only one possible match
but check_generic_type_consistency rejects it, then
function/operator lookup should return that OID anyway, allowing
enforce_generic_type_consistency to throw the appropriate error.
This would obviously not help when there are multiple polymorphic
functions having the same name and number of arguments, but that
strikes me as a very unusual corner case.

			regards, tom lane

  18. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-07-27T15:31:05Z

    On Wed, Jul 21, 2021 at 6:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Wed, Jul 21, 2021 at 12:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Alexander Korotkov <aekorotkov@gmail.com> writes:
> >>> Do I understand correctly that enforce_generic_type_consistency() is
> >>> called only after check_generic_type_consistency() returned true?
> >>> If so, that means some of the checks are redundant.  Therefore, we can
> >>> replace ereport()'s with Assert()'s.
>
> >> They are not redundant, IIRC.  I forget the exact mechanism for
> >> reaching them, but it likely has something to do with aggregates
> >> or variadic functions.
>
> > If checks aren't redundant, there should be cases when they don't
> > pass.  It would be nice to identify these cases and add them to the
> > regression tests.  I didn't manage to do this yet.
>
> Hmm ... whether or not there are edge cases where those errors are
> reachable, it's certainly true that the mainline case doesn't reach
> them:
>
> regression=# create function myadd(anyelement, anyelement) returns anyelement
> as 'select $1 + $2' language sql;
> CREATE FUNCTION
> regression=# select myadd(1, 2.3);
> ERROR:  function myadd(integer, numeric) does not exist
> LINE 1: select myadd(1, 2.3);
>                ^
> HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
>
> That's too bad, because IMO it'd be way more helpful to say
>   ERROR:  arguments declared "anyelement" are not all alike
>   DETAIL:  integer versus numeric
> which is what enforce_generic_type_consistency would say if it
> were reached.  Similarly, the other error cases in that code
> are far more specific and thus more helpful than simply reporting
> that there's no matching function.
>
> I'm tempted to propose that, if there is only one possible match
> but check_generic_type_consistency rejects it, then
> function/operator lookup should return that OID anyway, allowing
> enforce_generic_type_consistency to throw the appropriate error.
> This would obviously not help when there are multiple polymorphic
> functions having the same name and number of arguments, but that
> strikes me as a very unusual corner case.

I spend some time thinking about this.  I'm actually not sure this
approach is really correct.  If there is only one polymorphic
candidate, it's still possible that the user means non-polymorphic
function with exactly matching arguments, which is simply doesn't
exist.

------
Regards,
Alexander Korotkov

  19. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-07-27T15:34:10Z

    On Wed, Jul 21, 2021 at 12:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > Do I understand correctly that enforce_generic_type_consistency() is
> > called only after check_generic_type_consistency() returned true?
> > If so, that means some of the checks are redundant.  Therefore, we can
> > replace ereport()'s with Assert()'s.
>
> They are not redundant, IIRC.  I forget the exact mechanism for
> reaching them, but it likely has something to do with aggregates
> or variadic functions.
>
> In any case, apologies for taking so long to get back to this.  Here's
> a proposed patch (based in part on Neil's earlier patch).

This patch looks good to me.  Besides fixing the particular bug
report, it seems that the situation of mismatching range and
multirange types in arguments is now handled correctly.

------
Regards,
Alexander Korotkov

  20. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-27T15:38:07Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Wed, Jul 21, 2021 at 12:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> In any case, apologies for taking so long to get back to this.  Here's
>> a proposed patch (based in part on Neil's earlier patch).

> This patch looks good to me.  Besides fixing the particular bug
> report, it seems that the situation of mismatching range and
> multirange types in arguments is now handled correctly.

Thanks for reviewing!  I'll push it later today.

			regards, tom lane

  21. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-07-27T19:43:52Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Wed, Jul 21, 2021 at 6:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> That's too bad, because IMO it'd be way more helpful to say
>> ERROR:  arguments declared "anyelement" are not all alike
>> DETAIL:  integer versus numeric
>> which is what enforce_generic_type_consistency would say if it
>> were reached.  Similarly, the other error cases in that code
>> are far more specific and thus more helpful than simply reporting
>> that there's no matching function.
>> 
>> I'm tempted to propose that, if there is only one possible match
>> but check_generic_type_consistency rejects it, then
>> function/operator lookup should return that OID anyway, allowing
>> enforce_generic_type_consistency to throw the appropriate error.
>> This would obviously not help when there are multiple polymorphic
>> functions having the same name and number of arguments, but that
>> strikes me as a very unusual corner case.

> I spend some time thinking about this.  I'm actually not sure this
> approach is really correct.  If there is only one polymorphic
> candidate, it's still possible that the user means non-polymorphic
> function with exactly matching arguments, which is simply doesn't
> exist.

I don't particularly buy that reasoning.  Certainly the true cause of
the error could be that the user mistyped the function name, or meant
to refer to something that's not in the search_path, or forgot to load
the function into this particular database, etc etc.  But we have
to act on the basis of the information we have, and that is the
function(s) we see.  If we let possibilities like these paralyze us,
we'll never be able to issue useful error messages at all.

I don't deny that what I'm proposing above is a bit weird and
non-orthogonal; there may be a better way to do it.  But the
existing code structure where check_generic_type_consistency
silently returns a boolean just isn't very conducive to giving
a good error message.  We have a lot more information available
to give, if we choose to give it.

Possibly we should think in terms of rewriting
enforce_generic_type_consistency's messages so that they are
errdetail() messages with a common primary message that's still
some variation of "there's no matching function".

			regards, tom lane

  22. Re: BUG #17066: Cache lookup failed when null (unknown) is passed as anycompatiblemultirange

    Alexander Korotkov <aekorotkov@gmail.com> — 2021-07-27T21:12:30Z

    On Tue, Jul 27, 2021 at 10:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Wed, Jul 21, 2021 at 6:03 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> That's too bad, because IMO it'd be way more helpful to say
> >> ERROR:  arguments declared "anyelement" are not all alike
> >> DETAIL:  integer versus numeric
> >> which is what enforce_generic_type_consistency would say if it
> >> were reached.  Similarly, the other error cases in that code
> >> are far more specific and thus more helpful than simply reporting
> >> that there's no matching function.
> >>
> >> I'm tempted to propose that, if there is only one possible match
> >> but check_generic_type_consistency rejects it, then
> >> function/operator lookup should return that OID anyway, allowing
> >> enforce_generic_type_consistency to throw the appropriate error.
> >> This would obviously not help when there are multiple polymorphic
> >> functions having the same name and number of arguments, but that
> >> strikes me as a very unusual corner case.
>
> > I spend some time thinking about this.  I'm actually not sure this
> > approach is really correct.  If there is only one polymorphic
> > candidate, it's still possible that the user means non-polymorphic
> > function with exactly matching arguments, which is simply doesn't
> > exist.
>
> I don't particularly buy that reasoning.  Certainly the true cause of
> the error could be that the user mistyped the function name, or meant
> to refer to something that's not in the search_path, or forgot to load
> the function into this particular database, etc etc.  But we have
> to act on the basis of the information we have, and that is the
> function(s) we see.  If we let possibilities like these paralyze us,
> we'll never be able to issue useful error messages at all.
>
> I don't deny that what I'm proposing above is a bit weird and
> non-orthogonal; there may be a better way to do it.  But the
> existing code structure where check_generic_type_consistency
> silently returns a boolean just isn't very conducive to giving
> a good error message.  We have a lot more information available
> to give, if we choose to give it.
>
> Possibly we should think in terms of rewriting
> enforce_generic_type_consistency's messages so that they are
> errdetail() messages with a common primary message that's still
> some variation of "there's no matching function".

That's an interesting idea!  If the primary message is still  "there's
no matching function", I feel absolutely comfortable about putting
information about "closest match" into detail.

------
Regards,
Alexander Korotkov