Thread: bug?

I found the following while poking around. RangeVarGetRelid takes a 
second parameter that is intended to allow it to not fail, returning 
InvalidOid instead. However it calls LookupExplicitNamespace, which does 
not honor any such request, and happily generates an error on a bad 
namespace name:

/* * RangeVarGetRelid *     Given a RangeVar describing an existing relation, *     select the proper namespace and
lookup the relation OID. * * If the relation is not found, return InvalidOid if failOK = true, * otherwise raise an
error.*/
 
Oid
RangeVarGetRelid(const RangeVar *relation, bool failOK)
{
[...]  if (relation->schemaname)  {    /* use exact schema given */    namespaceId =
LookupExplicitNamespace(relation->schemaname);   relId = get_relname_relid(relation->relname, namespaceId);  }
 
[...]
}

Oid
LookupExplicitNamespace(const char *nspname)
{
[...]  namespaceId = GetSysCacheOid(NAMESPACENAME,                  CStringGetDatum(nspname),0, 0, 0);  if
(!OidIsValid(namespaceId))   elog(ERROR, "Namespace \"%s\" does not exist", nspname);
 
[...]
}

Shouldn't LookupExplicitNamespace be changed to allow the same second 
parameter? All uses of LookupExplicitNamespace, besides in 
RangeVarGetRelid, would have the parameter set to false.

Comments?

Joe

Joe Conway <mail@joeconway.com> writes:
> I found the following while poking around. RangeVarGetRelid takes a 
> second parameter that is intended to allow it to not fail, returning 
> InvalidOid instead. However it calls LookupExplicitNamespace, which does 
> not honor any such request, and happily generates an error on a bad 
> namespace name:

ISTR deciding that that was okay, and there was no need to clutter
LookupExplicitNamespace with an extra parameter.  Don't recall the
reasoning at the moment...
        regards, tom lane

I said:
> Joe Conway <mail@joeconway.com> writes:
>> I found the following while poking around. RangeVarGetRelid takes a 
>> second parameter that is intended to allow it to not fail, returning 
>> InvalidOid instead. However it calls LookupExplicitNamespace, which does 
>> not honor any such request, and happily generates an error on a bad 
>> namespace name:

> ISTR deciding that that was okay, and there was no need to clutter
> LookupExplicitNamespace with an extra parameter.  Don't recall the
> reasoning at the moment...

After looking: the only place that calls RangeVarGetRelid with a "true"
second parameter is tcop/utility.c, and it just does it so that it can
give a different error message for the "relation not found" case.  Thus,
we don't actually *want* failures other than "relation not found" to
return from RangeVarGetRelid.  So the code is right as-is.  Perhaps the
comments could stand improvement though, to make it clearer what failOK
is meant to do.
        regards, tom lane

Tom Lane wrote:
>>Joe Conway <mail@joeconway.com> writes:
>>
>>>I found the following while poking around. RangeVarGetRelid takes a 
>>>second parameter that is intended to allow it to not fail, returning 
>>>InvalidOid instead. However it calls LookupExplicitNamespace, which does 
>>>not honor any such request, and happily generates an error on a bad 
>>>namespace name:
>>
> 
>>ISTR deciding that that was okay, and there was no need to clutter
>>LookupExplicitNamespace with an extra parameter.  Don't recall the
>>reasoning at the moment...
> 
> 
> After looking: the only place that calls RangeVarGetRelid with a "true"
> second parameter is tcop/utility.c, and it just does it so that it can
> give a different error message for the "relation not found" case.  Thus,
> we don't actually *want* failures other than "relation not found" to
> return from RangeVarGetRelid.  So the code is right as-is.  Perhaps the
> comments could stand improvement though, to make it clearer what failOK
> is meant to do.

OK. The reason I brought it up was that while working on the plpgsql patch 
(posted last night), I found that plpgsql gives a better error message if 
RangeVarGetRelid returns InvalidOid instead of simply elog'ing. The comments 
did lead me to believe I could get an InvalidOid, so I was surprized when I 
didn't.

Joe