Re: Refactoring SearchSysCache + HeapTupleIsValid

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.