Refactoring SearchSysCache + HeapTupleIsValid
Our code contains about 200 copies of the following code:
tuple = SearchSysCache[Copy](FOOOID, ObjectIdGetDatum(fooid), 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for foo %u", fooid);
This only counts elog() calls, not user-facing error messages
constructed with ereport().
Shouldn't we try to refactor this, maybe like this:
HeapTuple
SearchSysCache[Copy]Oid(int cacheId, Oid key)
{
HeapTuple tuple;
tuple = SearchSysCache[Copy](cacheId, ObjectIdGetDatum(key),
0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed in cache %d (relation %u) for
OID %u",
cacheId, cacheinfo[cacheId].reloid, key);
return tuple;
}
Maybe some other verb than "Search" could be used to make it clearer
that this function has its own error handler.
Peter Eisentraut <peter_e@gmx.net> writes:
Our code contains about 200 copies of the following code:
tuple = SearchSysCache[Copy](FOOOID, ObjectIdGetDatum(fooid), 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for foo %u", fooid);
...
Shouldn't we try to refactor this, maybe like this:
I can't get excited about it, and I definitely do not like your
suggestion of embedding particular assumptions about the lookup keys
into the API. What you've got here is a worse error message and a
recipe for proliferation of ad-hoc wrappers around SearchSysCache,
in return for saving a couple of lines per call site.
If we could just move the error into SearchSysCache it might be worth
doing, but I think there are callers that need the flexibility to not
fail.
regards, tom lane
Tom Lane wrote:
If we could just move the error into SearchSysCache it might be worth
doing, but I think there are callers that need the flexibility to not
fail.
Pass a boolean flag?
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Thursday 11 December 2008 15:28:08 Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Our code contains about 200 copies of the following code:
tuple = SearchSysCache[Copy](FOOOID, ObjectIdGetDatum(fooid), 0, 0, 0);
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for foo %u", fooid);
...
Shouldn't we try to refactor this, maybe like this:I can't get excited about it, and I definitely do not like your
suggestion of embedding particular assumptions about the lookup keys
into the API. What you've got here is a worse error message and a
recipe for proliferation of ad-hoc wrappers around SearchSysCache,
in return for saving a couple of lines per call site.If we could just move the error into SearchSysCache it might be worth
doing, but I think there are callers that need the flexibility to not
fail.
This is hardly ad hoc. There are about 400 calls to SearchSysCache[Copy], and
about 200 fit into the exact pattern I described. Normally, I'd start
refactoring at around 3 pieces of identical code. But when 50% of all calls
have an identical code around it, it is more of an interface failure.
What about the other "convenience routines" in syscache.h? They have less
calls combined than this proposal alone.
There are really two very natural ways to make a syscache search. One, you
get an object name from a user and look it up. If it fails, it is probably a
user error, and you go back to the user and explain it in detailed terms.
Two, you get an OID reference from somewhere else in the system and look it
up. If it fails, you bail out because the internal state of the system is
inconsistent. Most uses fit nicely into these two categories (with the
notable exception of dealing with pg_attribute, which already has its ad-hoc
wrappers). In fact, other uses would probably be suspicious.
About the error message, I find neither version to be very good. People see
these messages and don't know what to do. Considering that users do see
these supposedly-internal messages on occasion, we could design something
much better like
ereport(ERROR,
(errmsg("syscache lookup failed for OID %u in cache %d for
relation %s", ...),
errdetail("This probably means the internal system catalogs or system
caches are inconsistent. Try to restart the session. If the problem
persists, report a bug.")));
But we should really only put this together if we can do it at a central
place.
The problem with the idea of putting the error right into SearchSysCache(),
possibly with a Boolean flag, is twofold:
1. It doesn't really match actual usage: either you look up an OID and want to
fail, or you look up something else and want to handle the error yourself.
You'd break the interface for no general benefit.
2. You can't really create good error messages if you have no informatioon
about the type of the key.
Maybe someone has better ideas, but 200 copies of the same poor error message
don't make sense to me.
Peter Eisentraut wrote:
About the error message, I find neither version to be very good. People see
these messages and don't know what to do.
I agree. People see this:
ERROR: cache lookup failure for constraint 123123123
and they think it means the same as this:
ERROR: cache lookup failure for relation 456456456
The difference is subtle for people that are not -hackers regulars (I
had a case of this just yesterday); they immediately start to think
about temp tables and plpgsql functions, even with the first error
message, even when we say that we solved the underlying problem.
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support