On Fri, Dec 16, 2016 at 5:16 AM, Mithun Cy <mithun...@enterprisedb.com>
wrote:
> Shouldn't _hash_doinsert() be using the cache, too
>>
>
> Yes, we have an opportunity there, added same in code. But one difference
> is at the end at-least once we need to read the meta page to split and/or
> write. Performance improvement might not be as much as read-only.
>
Why would you need to read it at least once in one case but not the other?
>
>> I think it's probably not a good idea to cache the entire metapage. The
>> only things that you are "really" trying to cache, I think, are
>> hashm_maxbucket, hashm_lowmask, and hashm_highmask. The entire
>> HashPageMetaData structure is 696 bytes on my machine, and it doesn't
>> really make sense to copy the whole thing into memory if you only need 16
>> bytes of it. It could even be dangerous -- if somebody tries to rely on
>> the cache for some other bit of data and we're not really guaranteeing that
>> it's fresh enough for that.
>>
>> I'd suggest defining a new structure HashMetaDataCache with members
>> hmc_maxbucket, hmc_lowmask, and hmc_highmask. The structure can have a
>> comment explaining that we only care about having the data be fresh enough
>> to test whether the bucket mapping we computed for a tuple is still
>> correct, and that for that to be the case we only need to know whether a
>> bucket has suffered a new split since we last refreshed the cache.
>>
>
> It is not only hashm_maxbucket, hashm_lowmask, and hashm_highmask (3
> uint32s) but we also need
>
> *uint32 hashm_spares[HASH_MAX_SPLITPOINTS],* for bucket number to
> block mapping in "BUCKET_TO_BLKNO(metap, bucket)".
>
> Note : #define HASH_MAX_SPLITPOINTS 32, so it is (3*uint32 + 32*uint32) =
> 35*4 = 140 bytes.
>
Well, I guess that makes it more appealing to cache the whole page at least
in terms of raw number of bytes, but I suppose my real complaint here is
that there don't seem to be any clear rules for where, whether, and to what
extent we can rely on the cache to be valid. Without that, I think this
patch is creating an extremely serious maintenance hazard for anyone who
wants to try to modify this code in the future. A future patch author
needs to be able to tell what data they can use and what data they can't
use, and why.
Apart from this, there seems to be some base bug in _hash_doinsert().
> + * XXX this is useless code if we are only storing hash keys.
>
> + */
>
> + if (itemsz > HashMaxItemSize((Page) metap))
>
> + ereport(ERROR,
>
> + (errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
>
> + errmsg("index row size %zu exceeds hash maximum %zu",
>
> + itemsz, HashMaxItemSize((Page) metap)),
>
> + errhint("Values larger than a buffer page cannot be
> indexed.")));
>
> "metap" (HashMetaPage) and Page are different data structure their member
> types are not in sync, so should not typecast blindly as above. I think we
> should remove this part of the code as we only store hash keys. So I have
> removed same but kept the assert below as it is.
>
Any existing bugs should be the subject of a separate patch.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
