Re: Refactoring SearchSysCache + HeapTupleIsValid

On Thursday 11 December 2008 15:28:08 Tom Lane wrote:
> Peter Eisentraut <peter_e(at)gmx(dot)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.