Re: free space % calculation in pgstathashindex

On Mon, Aug 7, 2017 at 7:19 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Mon, Aug 7, 2017 at 6:07 PM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>> Hi,
>>
> ..
>> In step #1, assuming '*' as an arithmetic operator, the left operand
>> i.e. 'stats.unused_pages' is of type uint32 whereas the right operand
>> i.e. 'stats.space_per_page' is of type int32 and arithmetic
>> conversions of the ANSI C standard states: 'if either operand has type
>> unsigned int, the other operand is converted to unsigned int' which
>> means stats.space_per_page would be converted to uint32 and then the
>> multiplication would be performed. But, if the result of
>> multiplication is greater than the max value accepted by 32 bits
>> number, we would get an incorrect result. Hence, it is important to do
>> proper typecasting of operands before performing any arithmetic
>> operation. In this case, i would typecast both stats.unused_pages and
>> stats.space_per_page to uint64 and the
>> perform multiplication on them. Similar handling needs to be done in
>> step #2 as well.
>>
>
> I think even if you have typecasted the first variable, it would have
> served the purpose.
>

Yes, that's right, typecasting the first variable can also prevent the
overflow problem but, then, if the operands are of two different
types, compiler will promote all to the largest or more precise type.
Therefore, even if we explicitly typecast it or not, compiler will
eventually be doing that. However, i have now just typecasted the
first value. Attached is the updated patch.

> /* Count unused pages as free space. */
>
> - stats.free_space += stats.unused_pages * stats.space_per_page;
> + stats.free_space += ((uint64) stats.unused_pages *
> + (uint64) stats.space_per_page);
>
> Why an extra parenthesis in above case whereas not in below case? I
> think the code will look consistent if you follow the same coding
> practice. I suggest don't use it unless you need it.

That is because in the 1st case, there are multiple operators (*, +)
whereas in the 2nd case we have just one(*). So, just to ensure that
'*' is performed before '+', i had used parenthesis, though it is not
required as '*' has higher precedence than '+'. I have removed the
extra parenthesis and attached is the new version of patch. Thanks.

>
> /*
> * Total space available for tuples excludes the metapage and the bitmap
> * pages.
> */
> - total_space = (nblocks - (stats.bitmap_pages + 1)) * stats.space_per_page;
> + total_space = (uint64) (nblocks - (stats.bitmap_pages + 1)) *
> + (uint64) stats.space_per_page;
>

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com