Re: [sqlsmith] Failed assertion in joinrels.c

Dilip Kumar <dilipbalaut(at)gmail(dot)com> writes:
> On Thu, Sep 8, 2016 at 2:23 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> In the case of record_in, it seems like it'd be easy enough to use
>> lookup_rowtype_tupdesc_noerror() instead of lookup_rowtype_tupdesc()
>> and then throw a user-facing error right there.

> Actually when we are passing invalid type to "record_in", error is
> thrown in "lookup_type_cache" function, and this function is not
> taking "no_error" input (it's only limited to
> lookup_rowtype_tupdesc_internal).

Ah, should have dug a bit deeper.

> One option is to pass "no_error" parameter to "lookup_type_cache"
> function also, and throw error only in record_in function.
> But problem is, "lookup_type_cache" has lot of references. And also
> will it be good idea to throw one generic error instead of multiple
> specific errors. ?

We could make it work like that without breaking the ABI if we were
to add a NOERROR bit to the allowed "flags". However, after looking
around a bit I'm no longer convinced what I said above is a good idea.
In particular, if we put the responsibility of reporting the error on
callers then we'll lose the ability to distinguish "no such pg_type OID"
from "type exists but it's only a shell". So I'm now thinking it's okay
to promote lookup_type_cache's elog to a full ereport, especially since
it's already using ereport for some of the related error cases such as
the shell-type failure.

That still leaves us with what to do for domain_in. A really simple
fix would be to move the InitDomainConstraintRef call before the
getBaseTypeAndTypmod call, because the former fetches the typcache
entry and would then benefit from lookup_type_cache's ereport.
But that seems a bit magic.

I'm tempted to propose that domain_state_setup start with

typentry = lookup_type_cache(domainType, TYPECACHE_DOMAIN_INFO);
if (typentry->typtype != TYPTYPE_DOMAIN)
ereport(ERROR,
(errcode(ERRCODE_DATATYPE_MISMATCH),
errmsg("type %s is not a domain",
format_type_be(domainType))));

removing the need to verify that after getBaseTypeAndTypmod.

The cache loading is basically free because InitDomainConstraintRef
would do it anyway; so the extra cost here is only one dynahash search.
You could imagine buying back those cycles by teaching the typcache
to be able to cache the result of getBaseTypeAndTypmod, but I'm doubtful
that we really care. This whole setup sequence only happens once per
query anyway.

regards, tom lane