five-key syscaches
in [PgSql]
|
From: Yeb Havinga on 14 Jul 2010 07:27 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(a)mcknight.de> wrote: > >> On Mon, Mar 29, 2010 at 12:32 AM, Robert Haas <robertmhaas(a)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 > >
From: Robert Haas on 14 Jul 2010 10:39 On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Yeb Havinga on 14 Jul 2010 10:56 Robert Haas wrote: > On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
From: Robert Haas on 14 Jul 2010 11:04 On Wed, Jul 14, 2010 at 10:56 AM, Yeb Havinga <yebhavinga(a)gmail.com> wrote: > Robert Haas wrote: >> On Wed, Jul 14, 2010 at 7:27 AM, Yeb Havinga <yebhavinga(a)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 -- Sent via pgsql-hackers mailing list (pgsql-hackers(a)postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
|
Pages: 1 Prev: cross column correlation revisted Next: [HACKERS] standard_conforming_strings |