Re: GiST buffering build, bug in levelStep calculation

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> This is because of this rather complex calculation here:

>> while (
>> /* subtree must fit in cache (with safety factor of 4) */
>> (1 - pow(avgIndexTuplesPerPage, (double) (levelStep + 1))) / (1 - avgIndexTuplesPerPage) < effective_cache_size / 4
>> &&
>> /* each node in the lowest level of a subtree has one page in memory */
>> (pow(maxIndexTuplesPerPage, (double) levelStep) < (maintenance_work_mem * 1024) / BLCKSZ)
>> )

> What happens here is that the calculation of (maintenance_work_mem * 
> 1024) / BLCKSZ overflows the 32 bit signed integer type used there, if 
> maintenance_work_mem >= 2GB. I think we should be doing all these 
> calculations in double.

Given the use of pow(), I concur with changing the whole calculation to
double.  Just as a remark, the correct way to code that sort of thing
normally is (maintenance_work_mem * 1024L) / BLCKSZ, which avoids
overflow because guc.c limits MAX_KILOBYTES to ensure it won't overflow
a long.  (Given that we are now supporting Win64 where long is narrower
than size_t, we might want to revisit this coding rule eventually, but
it's not high on my priority list.)

While I'm looking at this, is the first test involving
effective_cache_size bulletproof either?  In particular, is
avgIndexTuplesPerPage clamped to be strictly greater than 1?

And for that matter, is either test sane from a units standpoint?
It seems to me that maxIndexTuplesPerPage would have units of
1/blocks, which is pretty dubious to be comparing to a block count
even disregarding the power function.
        regards, tom lane