five-key syscaches

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.

...Robert

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)

Joachim

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

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
>
>

On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> Attach is a new patch with all things described above addressed.

Thanks!

I think we should probably hold off applying this until some of the
other KNNGIST work is ready, or we have some other concrete need for
5-key syscaches.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas wrote:
> On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
>
>> Attach is a new patch with all things described above addressed.
>>
>
> Thanks!
>
> I think we should probably hold off applying this until some of the
> other KNNGIST work is ready, or we have some other concrete need for
> 5-key syscaches.
>
Any thoughts about the << 16 and >> 16 bit shifting on the 5th hash key
computation? I blithely copied it from the 3rd key.

--
Yeb

On Wed, Jul 14, 2010 at 10:56 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> Robert Haas wrote:
>> On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
>>> Attach is a new patch with all things described above addressed.
>> Thanks!
>>
>> I think we should probably hold off applying this until some of the
>> other KNNGIST work is ready, or we have some other concrete need for
>> 5-key syscaches.
>
> Any thoughts about the << 16 and >> 16 bit shifting on the 5th hash key
> computation? I blithely copied it from the 3rd key.

Hmm, I thought I had the bit in my version, but I see that I don't.
Must have gotten lost from an earlier incarnation. It's probably bad
to duplicate the bit-shifting pattern of an existing key. We might
want to shift by something that's not a multiple of 8, like 12/20.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company