Re: [HACKERS] free space % calculation in pgstathashindex

[Noah Misch]

On Tue, Aug 08, 2017 at 02:30:51PM +0530, Amit Kapila wrote:
> On Mon, Aug 7, 2017 at 9:38 PM, Ashutosh Sharma  wrote:
> > On Mon, Aug 7, 2017 at 7:19 PM, Amit Kapila  wrote:
> >> On Mon, Aug 7, 2017 at 6:07 PM, Ashutosh Sharma  
> >> wrote:
> >>> Hi,
> >>>
> >> ..
> ..
> >> 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.
> >
> 
> Your latest patch looks good to me.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Robert,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] 
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] free space % calculation in pgstathashindex

[Amit Kapila]

On Mon, Aug 7, 2017 at 9:38 PM, Ashutosh Sharma  wrote:
> On Mon, Aug 7, 2017 at 7:19 PM, Amit Kapila  wrote:
>> On Mon, Aug 7, 2017 at 6:07 PM, Ashutosh Sharma  
>> wrote:
>>> Hi,
>>>
>> ..
..
>> 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.
>

Your latest patch looks good to me.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] free space % calculation in pgstathashindex

[Ashutosh Sharma]

On Mon, Aug 7, 2017 at 7:19 PM, Amit Kapila  wrote:
> On Mon, Aug 7, 2017 at 6:07 PM, Ashutosh Sharma  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
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 44e322d..9365ba7 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -687,13 +687,14 @@ pgstathashindex(PG_FUNCTION_ARGS)
 	index_close(rel, AccessShareLock);
 
 	/* Count unused pages as free space. */
-	stats.free_space += stats.unused_pages * stats.space_per_page;
+	stats.free_space += (uint64) stats.unused_pages * stats.space_per_page;
 
 	/*
 	 * 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)) *
+		stats.space_per_page;
 
 	if (total_space == 0)
 		free_percent = 0.0;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] free space % calculation in pgstathashindex

[Amit Kapila]

On Mon, Aug 7, 2017 at 6:07 PM, Ashutosh Sharma  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.

  /* 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.

  /*
  * 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,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] free space % calculation in pgstathashindex

[Ashutosh Sharma]

Hi,

While working on - [1]  (one of the bug reported for hash index), i
noticed that the percentage of free space shown in 'free_percent'
column of pgstathashindex() is incorrect for some of the boundary
cases. This is how the free space percentage is calculated by
pgstathashindex(),

1)  /* Count unused pages as free space. */
stats.free_space += stats.unused_pages * stats.space_per_page;

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

3)  Calculate the percentage of free space in hash index table.
free_percent = 100.0 * stats.free_space / total_space;

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.

Attached is the patch that fixes this.

[1] - 
https://www.postgresql.org/message-id/20170704105728.mwb72jebfmok2nm2%40zip.com.au
[2] - 
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka9483.html
commit a34659959fe7385e68d196e57e90fe92b12764d4
Author: ashu 
Date:   Mon Aug 7 11:18:27 2017 +0530

Add proper typecasting to the operands when doing arithmatic operations.

Patch by Ashutosh Sharma.

diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 44e322d..4e363cd 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -687,13 +687,15 @@ pgstathashindex(PG_FUNCTION_ARGS)
 	index_close(rel, AccessShareLock);
 
 	/* 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);
 
 	/*
 	 * 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;
 
 	if (total_space == 0)
 		free_percent = 0.0;

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers