Re: five-key syscaches

Hello Robert,

As part of the current reviewfest, I reviewed your patch, and made some
changes on the way.

This was all ok:

*) while proofreading I did not find typos other than the one that
Joachim had already pointed out.
*) the addition of 5-key lookups to the existing ones seems a natural
extension, and the best way to solve finding the index that
can-order-by-op needed for the knngist. Solutions were debated in a
relatively long thread 'knngist patch support', where the first
reference of four columns being too less was in
http://archives.postgresql.org/pgsql-hackers/2010-02/msg01071.php
*) regression test ok
*) performance: comparing make check speeds with and without patch did
not reveal significant differences.

The changes:

*) since the API of the syscache functions is changed, one would expect
a lot of compile errors but none were found. The patch of
http://archives.postgresql.org/pgsql-committers/2010-02/msg00174.php
that introduced macro's around the base functions made that possible.
Two calls in contrib/tsearch2 were overlooked.
*) after changing the calls in contrib/tsearch2 and compiled and
installchecked ok
*) I also removed a few unneeded includes of syscache.h from some
contrib modules
*) In syscache.c the cachedesc structure has a key array that is
increased from 4 to CATCACHE_MAXKEYS. However, each element of the
cacheinfo[] array still has 4 attribute numbers listed, so the 5th
element is undefined. To not rely on compiler or platform and for code
uniformity I changed all syscaches to have 5 attribute numbers.
*) To test the new functions I added an extra syscache and performed a 5
key lookup. This gave the following error FATAL: wrong number of hash
keys: 5 in CatalogCacheComputeHashValue. I changed that as well, but
somebody with intimate knowledge of hash algorithms should probably
decide which bit-shifting on the key values is appropriate. It currently
does the same as key 3: hashValue ^= oneHash << 16; hashValue ^= oneHash
>> 16;

I tested a negative and positive search with SearchSysCacheExists5, that
were executed as expected. Regression test still ok.

Attach is a new patch with all things described above addressed.

regards,
Yeb Havinga

Robert Haas wrote:
> On Mon, Mar 29, 2010 at 4:21 AM, Joachim Wieland <joe(at)mcknight(dot)de> wrote:
>
>> On Mon, Mar 29, 2010 at 12:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>
>>> Per previous discussion, PFA a patch to change the maximum number of
>>> keys for a syscache from 4 to 5.
>>>
>>> http://archives.postgresql.org/pgsql-hackers/2010-02/msg01105.php
>>>
>>> This is intended for application to 9.1, and is supporting
>>> infrastructure for knngist.
>>>
>> It looks like there should be a 5 rather than a 4 for nkeys of
>> SearchSysCacheList().
>>
>> +#define SearchSysCacheList5(cacheId, key1, key2, key3, key4, key5) \
>> + SearchSysCacheList(cacheId, 4, key1, key2, key3, key4, key5)
>>
>
> Good catch. Will fix.
>
> ...Robert
>
>