Thread: code cleanup for SearchSysCache

There are roughly 420 calls of SearchSysCache() and 217 of which are just
report "cache lookup failed". Shall we put the elog in the SearchSysCache
itself?

Notice that most search is on the "Oid" field -- which is *not* user
visible, so I think most of them can safely let SearchSysCache handle the
failed search without reporting any misleading information. Also, to support
situations where indeed need to check the return tuple, we can add a boolean
parameter "isComplain" to the argument list.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> There are roughly 420 calls of SearchSysCache() and 217 of which are just
> report "cache lookup failed". Shall we put the elog in the SearchSysCache
> itself?

You'd need two essentially equivalent versions of SearchSysCache, and
you'd lose the ability to make the error message identify what was being
searched for, so I vote no.
        regards, tom lane

"Tom Lane" <tgl@sss.pgh.pa.us> wrote
>
> You'd need two essentially equivalent versions of SearchSysCache, and
> you'd lose the ability to make the error message identify what was being
> searched for, so I vote no.
>

Both arguments are not necessarily true. This change is quite like what we
made to hash_search(). There is only one SearchSysCache() which will take an
extra argument "isComplain" (vs. HASH_ENTER_NULL). The error message can be
easily identified from the first parameter "cacheId" -- we will add another
field in struct cachedesc which describs the cache name.

Regards,
Qingqing

"Qingqing Zhou" <zhouqq@cs.toronto.edu> writes:
> "Tom Lane" <tgl@sss.pgh.pa.us> wrote
>> You'd need two essentially equivalent versions of SearchSysCache, and
>> you'd lose the ability to make the error message identify what was being
>> searched for, so I vote no.

> Both arguments are not necessarily true. This change is quite like what we
> made to hash_search(). There is only one SearchSysCache() which will take an
> extra argument "isComplain" (vs. HASH_ENTER_NULL). The error message can be
> easily identified from the first parameter "cacheId" -- we will add another
> field in struct cachedesc which describs the cache name.

I think you misunderstood my second point: you might want a custom error
message for a particular usage.

The bottom line though is I don't see this as a useful improvement, and
given the amount of code it will break (both inside and outside our
CVS), marginal niceness isn't a good enough reason to change.  If we had
another reason forcing a change in SearchSysCache's API, then maybe we'd
do this at the same time, but I can't see doing it by itself.
        regards, tom lane