Horton
Menu

Thread

  1. five-key syscaches

    Robert Haas <robertmhaas@gmail.com> — 2010-03-28T22:32:36Z

    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

  2. Re: five-key syscaches

    Joachim Wieland <joe@mcknight.de> — 2010-03-29T08:21:46Z

    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)


Joachim

  3. Re: five-key syscaches

    Robert Haas <robertmhaas@gmail.com> — 2010-03-29T13:39:22Z

    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

  4. Re: five-key syscaches

    Yeb Havinga <yebhavinga@gmail.com> — 2010-07-14T11:27:44Z

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

  5. Re: five-key syscaches

    Robert Haas <robertmhaas@gmail.com> — 2010-07-14T14:39:50Z

    On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga@gmail.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

  6. Re: five-key syscaches

    Yeb Havinga <yebhavinga@gmail.com> — 2010-07-14T14:56:04Z

    Robert Haas wrote:
> On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga@gmail.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

  7. Re: five-key syscaches

    Robert Haas <robertmhaas@gmail.com> — 2010-07-14T15:04:36Z

    On Wed, Jul 14, 2010 at 10:56 AM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> Robert Haas wrote:
>> On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga@gmail.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