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 look up 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