Обсуждение: amvalidate(): cache lookup failed for operator class 123

Per sqlsmith.

postgres=# select amvalidate(123);
ERROR:  cache lookup failed for operator class 123
postgres=# \errverbose 
ERROR:  XX000: cache lookup failed for operator class 123
LOCATION:  amvalidate, amapi.c:125

The usual expectation is that sql callable functions should return null rather
than hitting elog().

-- 
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:
> Per sqlsmith.
> postgres=# select amvalidate(123);
> ERROR:  cache lookup failed for operator class 123
> postgres=# \errverbose
> ERROR:  XX000: cache lookup failed for operator class 123
> LOCATION:  amvalidate, amapi.c:125

> The usual expectation is that sql callable functions should return null rather
> than hitting elog().

Meh.  I'm not convinced that that position ought to apply to amvalidate.
Under what circumstances would you be calling that on an opclass that
might be about to be dropped?

            regards, tom lane

On Thu, May 13, 2021 at 02:22:16PM -0400, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > Per sqlsmith.
> > postgres=# select amvalidate(123);
> > ERROR:  cache lookup failed for operator class 123
> > postgres=# \errverbose 
> > ERROR:  XX000: cache lookup failed for operator class 123
> > LOCATION:  amvalidate, amapi.c:125
> 
> > The usual expectation is that sql callable functions should return null rather
> > than hitting elog().
> 
> Meh.  I'm not convinced that that position ought to apply to amvalidate.
> Under what circumstances would you be calling that on an opclass that
> might be about to be dropped?

Sure, no problem.  I'm just passing on the message :)

-- 
Justin

On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Meh.  I'm not convinced that that position ought to apply to amvalidate.

I am still of the opinion that we ought to apply it across the board,
for consistency. It makes it easier for humans to know which problems
are known to be reachable and which are thought to be can't-happen and
thus bugs. If we fix cases like this to return a real error code, then
anything that comes up as XX000 is likely to be a real bug, whereas if
we don't, the things that we're not concerned about have to be
filtered out by some other method, probably involving a human being.
If the filter that human being has to apply further involves reading
Tom Lane's mind and knowing what he will think about a particular
report, or alternatively asking him, it just makes complicated
something that we could have made simple.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Meh.  I'm not convinced that that position ought to apply to amvalidate.

> I am still of the opinion that we ought to apply it across the board,
> for consistency.

The main reason I'm concerned about applying that rule to amvalidate
is that then how do you know what's actually an error case?

As a hardly-irrelevant counterexample, we have a whole bunch of
regression tests that do something like

SELECT ...
WHERE NOT amvalidate(oid);

Every one of those is silently and dangerously wrong if amvalidate
might sometimes return null.

            regards, tom lane

On Thu, May 13, 2021 at 4:49 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> The main reason I'm concerned about applying that rule to amvalidate
> is that then how do you know what's actually an error case?
>
> As a hardly-irrelevant counterexample, we have a whole bunch of
> regression tests that do something like
>
> SELECT ...
> WHERE NOT amvalidate(oid);
>
> Every one of those is silently and dangerously wrong if amvalidate
> might sometimes return null.

Oh, I didn't notice previously that Justin's proposal was to make the
functions return NULL. He's correct that this is consistent with other
cases, and if we go that way, then these queries need to be updated. I
had just been imaging using ereport(ERROR, ...) which wouldn't have
that problem. I think either approach would be an improvement over the
status quo.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

On Sat, May 15, 2021 at 12:00:37PM -0400, Robert Haas wrote:
> Oh, I didn't notice previously that Justin's proposal was to make the
> functions return NULL. He's correct that this is consistent with other
> cases, and if we go that way, then these queries need to be updated. I
> had just been imaging using ereport(ERROR, ...) which wouldn't have
> that problem. I think either approach would be an improvement over the
> status quo.

FWIW, I am not convinced with what we could gain by sending NULL as
result rather than bump on an ERROR with this function.  Don't take me
wrong, I like when system functions return a gentle NULL result if
something does not exist, if the function is something we document and
if it can be used with large catalog scans a-la-pg_class.  I don't
think that there is any need to apply that to amvalidate() though, and
it could mean potential issues with out-of-core modules, rum for one,
no?
--
Michael

On Thu, May 13, 2021 at 12:01:22PM -0500, Justin Pryzby wrote:
> postgres=# select amvalidate(123);
> ERROR:  cache lookup failed for operator class 123
> The usual expectation is that sql callable functions should return null rather
> than hitting elog().

On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Meh.  I'm not convinced that that position ought to apply to amvalidate.

On Thu, May 13, 2021 at 04:12:10PM -0400, Robert Haas wrote:
> I am still of the opinion that we ought to apply it across the board,
> for consistency. It makes it easier for humans to know which problems
> are known to be reachable and which are thought to be can't-happen and
> thus bugs. If we fix cases like this to return a real error code, then
> anything that comes up as XX000 is likely to be a real bug, whereas if
> we don't, the things that we're not concerned about have to be
> filtered out by some other method, probably involving a human being.
> If the filter that human being has to apply further involves reading
> Tom Lane's mind and knowing what he will think about a particular
> report, or alternatively asking him, it just makes complicated
> something that we could have made simple.

FWIW, here are some other cases from sqlsmith which hit elog()/XX000:

postgres=# select unknownin('');
ERROR:  failed to find conversion function from unknown to text
postgres=# \errverbose
ERROR:  XX000: failed to find conversion function from unknown to text
LOCATION:  coerce_type, parse_coerce.c:542

postgres=# SELECT pg_catalog.interval( '12 seconds'::interval ,3);
ERROR:  unrecognized interval typmod: 3

postgres=# SELECT pg_describe_object(1,0,1);
ERROR:  invalid non-zero objectSubId for object class 1

postgres=# SELECT pg_read_file( repeat('a',333));
ERROR:  could not open file
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
forreading: File name too long
 

-- 
Justin

On Mon, Feb 13, 2023 at 07:50:53AM -0600, Justin Pryzby wrote:
> On Thu, May 13, 2021 at 12:01:22PM -0500, Justin Pryzby wrote:
> > postgres=# select amvalidate(123);
> > ERROR:  cache lookup failed for operator class 123
> > The usual expectation is that sql callable functions should return null rather
> > than hitting elog().
> 
> On Thu, May 13, 2021 at 2:22 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Meh.  I'm not convinced that that position ought to apply to amvalidate.
> 
> On Thu, May 13, 2021 at 04:12:10PM -0400, Robert Haas wrote:
> > I am still of the opinion that we ought to apply it across the board,
> > for consistency. It makes it easier for humans to know which problems
> > are known to be reachable and which are thought to be can't-happen and
> > thus bugs. If we fix cases like this to return a real error code, then
> > anything that comes up as XX000 is likely to be a real bug, whereas if
> > we don't, the things that we're not concerned about have to be
> > filtered out by some other method, probably involving a human being.
> > If the filter that human being has to apply further involves reading
> > Tom Lane's mind and knowing what he will think about a particular
> > report, or alternatively asking him, it just makes complicated
> > something that we could have made simple.
> 
> FWIW, here are some other cases from sqlsmith which hit elog()/XX000:
> 
> postgres=# select unknownin('');
> ERROR:  failed to find conversion function from unknown to text
> postgres=# \errverbose
> ERROR:  XX000: failed to find conversion function from unknown to text
> LOCATION:  coerce_type, parse_coerce.c:542
> 
> postgres=# SELECT pg_catalog.interval( '12 seconds'::interval ,3);
> ERROR:  unrecognized interval typmod: 3
> 
> postgres=# SELECT pg_describe_object(1,0,1);
> ERROR:  invalid non-zero objectSubId for object class 1
> 
> postgres=# SELECT pg_read_file( repeat('a',333));
> ERROR:  could not open file
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
forreading: File name too long
 

postgres=# SELECT acldefault('a',0);
ERROR:  unrecognized object type abbreviation: a

postgres=# SELECT setweight('a','1');
ERROR:  unrecognized weight: 49

regression=# select float8_regr_intercept(ARRAY[1]) ;
ERROR:  float8_regr_intercept: expected 6-element float8 array

None of these is new in v16.