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@mcknight.de> wrote:
>   
>> On Mon, Mar 29, 2010 at 12:32 AM, Robert Haas <robertmhaas@gmail.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
>
>