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

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.

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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