Re: [HACKERS] Add pgstathashindex() to get hash index table statistics.

2017-04-12 Thread Robert Haas 
On Wed, Apr 12, 2017 at 2:28 AM, Noah Misch  wrote:
> [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.

Oops.  I had forgotten about this one; committed now.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-04-12 Thread Noah Misch 
On Thu, Apr 06, 2017 at 10:45:26AM +0530, Ashutosh Sharma wrote:
> >> Based on the earlier discussions, I have prepared a patch that would
> >> allow pgstathashindex() to show the number of unused pages in hash
> >> index. Please find the attached patch for the same. Thanks.
> >
> > My idea is that we shouldn't end up with both a zero_pages column and
> > an unused_pages column.  Instead, we should end up with just an
> > unused_pages column, which will include both pages that are all-zeroes
> > and pages that have a valid special space marked as LH_UNUSED.
> >
> > Also, I don't see why it's correct to test PageIsEmpty() here instead
> > of just testing the page type as we do in pageinspect.
> >
> > Attached is a revised patch that shows what I have in mind; please
> > review.  Along the way I made the code for examining the page type
> > more similar to what pageinspect does, because I see no reason for
> > those things to be different, and I think the pageinspect code is
> > better.
> 
> I have reviewed the patch and it looks good to me. Also, the idea of
> including both zero and unused pages in a single 'unused' column looks
> better. Thanks.

[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] Add pgstathashindex() to get hash index table statistics.

2017-04-05 Thread Ashutosh Sharma 
Hi,

>>
>> Based on the earlier discussions, I have prepared a patch that would
>> allow pgstathashindex() to show the number of unused pages in hash
>> index. Please find the attached patch for the same. Thanks.
>
> My idea is that we shouldn't end up with both a zero_pages column and
> an unused_pages column.  Instead, we should end up with just an
> unused_pages column, which will include both pages that are all-zeroes
> and pages that have a valid special space marked as LH_UNUSED.
>
> Also, I don't see why it's correct to test PageIsEmpty() here instead
> of just testing the page type as we do in pageinspect.
>
> Attached is a revised patch that shows what I have in mind; please
> review.  Along the way I made the code for examining the page type
> more similar to what pageinspect does, because I see no reason for
> those things to be different, and I think the pageinspect code is
> better.

I have reviewed the patch and it looks good to me. Also, the idea of
including both zero and unused pages in a single 'unused' column looks
better. Thanks.

--
With Regards,
Ashutosh Sharma
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] Add pgstathashindex() to get hash index table statistics.

2017-04-05 Thread Robert Haas 
On Thu, Mar 23, 2017 at 1:54 PM, Ashutosh Sharma  wrote:
>> Yeah, but I think "unused" might be better.  Because a page could be
>> in use (as an overflow page or primary bucket page) and still be
>> empty.
>
> Based on the earlier discussions, I have prepared a patch that would
> allow pgstathashindex() to show the number of unused pages in hash
> index. Please find the attached patch for the same. Thanks.

My idea is that we shouldn't end up with both a zero_pages column and
an unused_pages column.  Instead, we should end up with just an
unused_pages column, which will include both pages that are all-zeroes
and pages that have a valid special space marked as LH_UNUSED.

Also, I don't see why it's correct to test PageIsEmpty() here instead
of just testing the page type as we do in pageinspect.

Attached is a revised patch that shows what I have in mind; please
review.  Along the way I made the code for examining the page type
more similar to what pageinspect does, because I see no reason for
those things to be different, and I think the pageinspect code is
better.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


pgstathashindex-empty.patch
Description: Binary data

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-03-25 Thread Ashutosh Sharma 
>>
>> +1.  If we consider some more names for that column then probably one
>> alternative could be "empty pages".
>
> Yeah, but I think "unused" might be better.  Because a page could be
> in use (as an overflow page or primary bucket page) and still be
> empty.
>

 Based on the earlier discussions, I have prepared a patch that would
 allow pgstathashindex() to show the number of unused pages in hash
 index. Please find the attached patch for the same. Thanks.

>>>
>>>   else if (opaque->hasho_flag & LH_BITMAP_PAGE)
>>>   stats.bitmap_pages++;
>>> + else if (PageIsEmpty(page))
>>> + stats.unused_pages++;
>>>
>>> I think having this check after PageIsNew() makes more sense then
>>> having at the place where you currently have,
>>
>> I don't think having it just below PageIsNew() check would be good.
>> The reason being, there can be a bucket page which is in use but still
>> be empty. So, if we add a check just below PageIsNew check then even
>> though a page is marked as bucket page and is empty it will be shown
>> as unused page which i feel is not correct. Here is one simple example
>> that illustrates this point,
>>
>
> oh, is it a page where all the items have been deleted and no new
> items have been inserted?

Yes, it is a page from where items have been removed and no new
insertion has happened.

The reason why I have told that place is
> not appropriate is because all the other checks in near by code are on
> the page type and this check looks odd at that place, but we might
> need to do this way if there is no other clean solution.

I got your point but then i think that is the only one solution we
have as of now.

--
With Regards,
Ashutosh Sharma
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] Add pgstathashindex() to get hash index table statistics.

2017-03-25 Thread Amit Kapila 
On Sat, Mar 25, 2017 at 12:33 PM, Ashutosh Sharma  wrote:
> On Sat, Mar 25, 2017 at 11:02 AM, Amit Kapila  wrote:
>> On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma  
>> wrote:
>>> Hi,
>>>
>>> On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas  wrote:
 On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila  
 wrote:
>> Maybe we should call them "unused pages".
>
> +1.  If we consider some more names for that column then probably one
> alternative could be "empty pages".

 Yeah, but I think "unused" might be better.  Because a page could be
 in use (as an overflow page or primary bucket page) and still be
 empty.

>>>
>>> Based on the earlier discussions, I have prepared a patch that would
>>> allow pgstathashindex() to show the number of unused pages in hash
>>> index. Please find the attached patch for the same. Thanks.
>>>
>>
>>   else if (opaque->hasho_flag & LH_BITMAP_PAGE)
>>   stats.bitmap_pages++;
>> + else if (PageIsEmpty(page))
>> + stats.unused_pages++;
>>
>> I think having this check after PageIsNew() makes more sense then
>> having at the place where you currently have,
>
> I don't think having it just below PageIsNew() check would be good.
> The reason being, there can be a bucket page which is in use but still
> be empty. So, if we add a check just below PageIsNew check then even
> though a page is marked as bucket page and is empty it will be shown
> as unused page which i feel is not correct. Here is one simple example
> that illustrates this point,
>

oh, is it a page where all the items have been deleted and no new
items have been inserted?  The reason why I have told that place is
not appropriate is because all the other checks in near by code are on
the page type and this check looks odd at that place, but we might
need to do this way if there is no other clean solution.

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-03-25 Thread Ashutosh Sharma 
On Sat, Mar 25, 2017 at 11:02 AM, Amit Kapila  wrote:
> On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma  
> wrote:
>> Hi,
>>
>> On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas  wrote:
>>> On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila  
>>> wrote:
> Maybe we should call them "unused pages".

 +1.  If we consider some more names for that column then probably one
 alternative could be "empty pages".
>>>
>>> Yeah, but I think "unused" might be better.  Because a page could be
>>> in use (as an overflow page or primary bucket page) and still be
>>> empty.
>>>
>>
>> Based on the earlier discussions, I have prepared a patch that would
>> allow pgstathashindex() to show the number of unused pages in hash
>> index. Please find the attached patch for the same. Thanks.
>>
>
>   else if (opaque->hasho_flag & LH_BITMAP_PAGE)
>   stats.bitmap_pages++;
> + else if (PageIsEmpty(page))
> + stats.unused_pages++;
>
> I think having this check after PageIsNew() makes more sense then
> having at the place where you currently have,

I don't think having it just below PageIsNew() check would be good.
The reason being, there can be a bucket page which is in use but still
be empty. So, if we add a check just below PageIsNew check then even
though a page is marked as bucket page and is empty it will be shown
as unused page which i feel is not correct. Here is one simple example
that illustrates this point,

Query:
==
select hash_page_type(get_raw_page('con_hash_index', 17));

gdb shows following info for this block number 17,

(gdb) p *(PageHeader)page
$1 = {pd_lsn = {xlogid = 0, xrecoff = 122297104}, pd_checksum = 0,
pd_flags = 0, pd_lower = 24,pd_upper = 8176, pd_special = 8176,
  pd_pagesize_version = 8196, pd_prune_xid = 0,pd_linp = 0x22a82d8}

(gdb) p((HashPageOpaque) PageGetSpecialPointer(page))->hasho_flag
$2 = 66

(gdb) p(((HashPageOpaque) PageGetSpecialPointer(page))->hasho_flag
& LH_BUCKET_PAGE)
$3 = 2

>From above information it is clear that this page is a bucket page and
is empty. I think it should be shown as bucket page rather than unused
page. Also,  this has already been discussed in [1].

other than that
> code-wise your patch looks okay, although I haven't tested it.
>

I have tested it properly and didn't find any issues.

> I think this should also be tracked under PostgreSQL 10 open items,
> but as this is not directly a bug, so not sure, what do others think?

Well, Even I am not sure if this has to be added under open items list
or not. Thanks.

[1] - 
https://www.postgresql.org/message-id/CA%2BTgmobwLKpKe99qnTJCBzFB4UaVGKrLNX3hwp8wcxObMDx7pA%40mail.gmail.com

--
With Regards,
Ashutosh Sharma
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] Add pgstathashindex() to get hash index table statistics.

2017-03-24 Thread Amit Kapila 
On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma  wrote:
> Hi,
>
> On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas  wrote:
>> On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila  wrote:
 Maybe we should call them "unused pages".
>>>
>>> +1.  If we consider some more names for that column then probably one
>>> alternative could be "empty pages".
>>
>> Yeah, but I think "unused" might be better.  Because a page could be
>> in use (as an overflow page or primary bucket page) and still be
>> empty.
>>
>
> Based on the earlier discussions, I have prepared a patch that would
> allow pgstathashindex() to show the number of unused pages in hash
> index. Please find the attached patch for the same. Thanks.
>

  else if (opaque->hasho_flag & LH_BITMAP_PAGE)
  stats.bitmap_pages++;
+ else if (PageIsEmpty(page))
+ stats.unused_pages++;

I think having this check after PageIsNew() makes more sense then
having at the place where you currently have, other than that
code-wise your patch looks okay, although I haven't tested it.

I think this should also be tracked under PostgreSQL 10 open items,
but as this is not directly a bug, so not sure, what do others think?

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-03-23 Thread Ashutosh Sharma 
Hi,

On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas  wrote:
> On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila  wrote:
>>> Maybe we should call them "unused pages".
>>
>> +1.  If we consider some more names for that column then probably one
>> alternative could be "empty pages".
>
> Yeah, but I think "unused" might be better.  Because a page could be
> in use (as an overflow page or primary bucket page) and still be
> empty.
>

Based on the earlier discussions, I have prepared a patch that would
allow pgstathashindex() to show the number of unused pages in hash
index. Please find the attached patch for the same. Thanks.

--
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
From e3b59fa85f16d6d15be5360e85b7faf63e8683a9 Mon Sep 17 00:00:00 2001
From: ashu 
Date: Thu, 23 Mar 2017 23:02:26 +0530
Subject: [PATCH] Allow pgstathashindex to show unused pages v1

---
 contrib/pgstattuple/expected/pgstattuple.out  | 12 ++--
 contrib/pgstattuple/pgstatindex.c | 19 ---
 contrib/pgstattuple/pgstattuple--1.4--1.5.sql |  1 +
 doc/src/sgml/pgstattuple.sgml | 17 -
 4 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index 2c3515b..1f1ff46 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -132,9 +132,9 @@ select * from pgstatginindex('test_ginidx');
 
 create index test_hashidx on test using hash (b);
 select * from pgstathashindex('test_hashidx');
- version | bucket_pages | overflow_pages | bitmap_pages | zero_pages | live_items | dead_items | free_percent 
--+--++--++++--
-   2 |4 |  0 |1 |  0 |  0 |  0 |  100
+ version | bucket_pages | overflow_pages | bitmap_pages | zero_pages | unused_pages | live_items | dead_items | free_percent 
+-+--++--++--+++--
+   2 |4 |  0 |1 |  0 |0 |  0 |  0 |  100
 (1 row)
 
 -- these should error with the wrong type
@@ -233,9 +233,9 @@ select pgstatindex('test_partition_idx');
 (1 row)
 
 select pgstathashindex('test_partition_hash_idx');
-   pgstathashindex   
--
- (2,8,0,1,0,0,0,100)
+pgstathashindex
+---
+ (2,8,0,1,0,0,0,0,100)
 (1 row)
 
 drop table test_partitioned;
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index d448e9e..6fc41d6 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -120,6 +120,7 @@ typedef struct HashIndexStat
 	BlockNumber overflow_pages;
 	BlockNumber bitmap_pages;
 	BlockNumber zero_pages;
+	BlockNumber unused_pages;
 
 	int64	live_items;
 	int64	dead_items;
@@ -588,8 +589,8 @@ pgstathashindex(PG_FUNCTION_ARGS)
 	BufferAccessStrategy bstrategy;
 	HeapTuple	tuple;
 	TupleDesc	tupleDesc;
-	Datum		values[8];
-	bool		nulls[8];
+	Datum		values[9];
+	bool		nulls[9];
 	Buffer		metabuf;
 	HashMetaPage	metap;
 	float8		free_percent;
@@ -667,6 +668,8 @@ pgstathashindex(PG_FUNCTION_ARGS)
 			}
 			else if (opaque->hasho_flag & LH_BITMAP_PAGE)
 stats.bitmap_pages++;
+			else if (PageIsEmpty(page))
+stats.unused_pages++;
 			else
 ereport(ERROR,
 		(errcode(ERRCODE_INDEX_CORRUPTED),
@@ -680,8 +683,9 @@ pgstathashindex(PG_FUNCTION_ARGS)
 	/* Done accessing the index */
 	index_close(rel, AccessShareLock);
 
-	/* Count zero pages as free space. */
-	stats.free_space += stats.zero_pages * stats.space_per_page;
+	/* Count zero and unused pages as free space. */
+	stats.free_space += (stats.zero_pages + stats.unused_pages) *
+		 stats.space_per_page;
 
 	/*
 	 * Total space available for tuples excludes the metapage and the bitmap
@@ -711,9 +715,10 @@ pgstathashindex(PG_FUNCTION_ARGS)
 	values[2] = Int64GetDatum((int64) stats.overflow_pages);
 	values[3] = Int64GetDatum((int64) stats.bitmap_pages);
 	values[4] = Int64GetDatum((int64) stats.zero_pages);
-	values[5] = Int64GetDatum(stats.live_items);
-	values[6] = Int64GetDatum(stats.dead_items);
-	values[7] = Float8GetDatum(free_percent);
+	values[5] = Int64GetDatum((int64) stats.unused_pages);
+	values[6] = Int64GetDatum(stats.live_items);
+	values[7] = Int64GetDatum(stats.dead_items);
+	values[8] = Float8GetDatum(free_percent);
 	tuple = heap_form_tuple(tupleDesc, values, nulls);
 
 	PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
diff --git a/contrib/pgstattuple/pgstattuple--1.4--1.5.sql b/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
index 84e112e..eda719a 100644
--- a/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
+++

Re: [HACKERS] Add pgstathashindex() to get hash index table statistics.

2017-02-06 Thread Robert Haas 
On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila  wrote:
>> Maybe we should call them "unused pages".
>
> +1.  If we consider some more names for that column then probably one
> alternative could be "empty pages".

Yeah, but I think "unused" might be better.  Because a page could be
in use (as an overflow page or primary bucket page) and still be
empty.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-02-06 Thread Amit Kapila 
On Tue, Feb 7, 2017 at 2:04 AM, Robert Haas  wrote:
> On Sun, Feb 5, 2017 at 11:36 PM, Ashutosh Sharma  
> wrote:
>>> Committed with those changes.
>>
>> Thanks for above corrections and commit. But, There are couple of
>> things that we might have to change once the patch for 'WAL in Hash
>> Indexes' gets checked-in.
>>
>> 1) The test-case result needs to be changed because there won't be any
>> WARNING message : "WARNING:  hash indexes are not WAL-logged and their
>> use is discouraged".
>>
>> 2) From WAL patch for Hash Indexes onwards, we won't have any zero
>> pages in Hash Indexes so I don't think we need to have column showing
>> zero pages (zero_pages). When working on WAL in hash indexes, we found
>> that WAL routine 'XLogReadBufferExtended' does not expect a page to be
>> completely zero page else it returns Invalid Buffer. To fix this, we
>> started initializing freed overflow page and newly allocated bucket
>> pages using _hash_pageinit() hence I don't think there will be any
>> zero pages from here onwards.
>
> Maybe we should call them "unused pages".
>

+1.  If we consider some more names for that column then probably one
alternative could be "empty pages".

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-02-06 Thread Robert Haas 
On Sun, Feb 5, 2017 at 11:36 PM, Ashutosh Sharma  wrote:
>> Committed with those changes.
>
> Thanks for above corrections and commit. But, There are couple of
> things that we might have to change once the patch for 'WAL in Hash
> Indexes' gets checked-in.
>
> 1) The test-case result needs to be changed because there won't be any
> WARNING message : "WARNING:  hash indexes are not WAL-logged and their
> use is discouraged".
>
> 2) From WAL patch for Hash Indexes onwards, we won't have any zero
> pages in Hash Indexes so I don't think we need to have column showing
> zero pages (zero_pages). When working on WAL in hash indexes, we found
> that WAL routine 'XLogReadBufferExtended' does not expect a page to be
> completely zero page else it returns Invalid Buffer. To fix this, we
> started initializing freed overflow page and newly allocated bucket
> pages using _hash_pageinit() hence I don't think there will be any
> zero pages from here onwards.

Maybe we should call them "unused pages".  Those will presumably still exist.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-02-05 Thread Amit Kapila 
On Mon, Feb 6, 2017 at 10:06 AM, Ashutosh Sharma  wrote:
> On Sat, Feb 4, 2017 at 1:12 AM, Robert Haas  wrote:
>>
>> Committed with those changes.
>
> Thanks for above corrections and commit. But, There are couple of
> things that we might have to change once the patch for 'WAL in Hash
> Indexes' gets checked-in.
>
> 1) The test-case result needs to be changed because there won't be any
> WARNING message : "WARNING:  hash indexes are not WAL-logged and their
> use is discouraged".
>

I think this should be changed in WAL patch itself, no need to handle
it separately.

> 2) From WAL patch for Hash Indexes onwards, we won't have any zero
> pages in Hash Indexes so I don't think we need to have column showing
> zero pages (zero_pages). When working on WAL in hash indexes, we found
> that WAL routine 'XLogReadBufferExtended' does not expect a page to be
> completely zero page else it returns Invalid Buffer. To fix this, we
> started initializing freed overflow page and newly allocated bucket
> pages using _hash_pageinit() hence I don't think there will be any
> zero pages from here onwards.
>

Can't we use PageIsEmpty() to show such information?



-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-02-05 Thread Ashutosh Sharma 
On Sat, Feb 4, 2017 at 1:12 AM, Robert Haas  wrote:
> On Wed, Feb 1, 2017 at 10:13 PM, Michael Paquier
>  wrote:
>> On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
>>  wrote:
>>> Nothing else to add from my side. I'm marking this 'Ready for commiter'.
>>
>> Moved to CF 2017-03 with the same status.
>
> OK, I took a look at this.
>
> - The handling of the extension stuff wasn't correct.  You can't go
> back and modify version 1.4; that's already been released.  But
> version 1.5 hasn't been released yet, so we can (and should) add more
> stuff to that version instead of creating a new version.  We don't
> need the pushups that you did with superuser checks, because there's
> no version out there in the wild that has those checks, so users with
> installed binaries can't be relying on them.  I cleaned all that stuff
> up, which made this significantly simpler.
>
> - I removed several of the columns being returned from the metapage.
> The pageinspect code that I committed yesterday can be used to look at
> those values; there doesn't seem to be a need to also return them
> here.  What this is really useful for is getting the "real" values by
> scanning through the whole index and tallying things up.
>
> - I adjusted some of the data types, both in the SQL and in the C
> code, so that they all match and that they're wide enough to return
> the values they might contain without overflowing (but not wider).  I
> also made the free_percent float8 rather than float4, consistent with
> other things in this module.  One thing I cheated on slightly: I left
> the version as int4 rather than int8 even though the underlying field
> is uint32, for consistency with everything else in this module.
> That's not entirely intellectually satisfying, but I guess it's better
> to be consistent for the moment.
>
> - I fixed a number of cosmetic issues, like the fact that the
> documentation for this didn't use \x format, unlike all of the other
> pgstattuple documentation, and the fact that table_len isn't a very
> good name for a variable representing the total amount of space for
> tuples, and the fact that the documentation referred to a hash index
> as a "hash table", and the fact that the wording for the columns
> wasn't consistent with other functions in pgstattuple with similar
> columns.
>
> Committed with those changes.

Thanks for above corrections and commit. But, There are couple of
things that we might have to change once the patch for 'WAL in Hash
Indexes' gets checked-in.

1) The test-case result needs to be changed because there won't be any
WARNING message : "WARNING:  hash indexes are not WAL-logged and their
use is discouraged".

2) From WAL patch for Hash Indexes onwards, we won't have any zero
pages in Hash Indexes so I don't think we need to have column showing
zero pages (zero_pages). When working on WAL in hash indexes, we found
that WAL routine 'XLogReadBufferExtended' does not expect a page to be
completely zero page else it returns Invalid Buffer. To fix this, we
started initializing freed overflow page and newly allocated bucket
pages using _hash_pageinit() hence I don't think there will be any
zero pages from here onwards.

--
With Regards,
Ashutosh Sharma
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] Add pgstathashindex() to get hash index table statistics.

2017-02-03 Thread Robert Haas 
On Wed, Feb 1, 2017 at 10:13 PM, Michael Paquier
 wrote:
> On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
>  wrote:
>> Nothing else to add from my side. I'm marking this 'Ready for commiter'.
>
> Moved to CF 2017-03 with the same status.

OK, I took a look at this.

- The handling of the extension stuff wasn't correct.  You can't go
back and modify version 1.4; that's already been released.  But
version 1.5 hasn't been released yet, so we can (and should) add more
stuff to that version instead of creating a new version.  We don't
need the pushups that you did with superuser checks, because there's
no version out there in the wild that has those checks, so users with
installed binaries can't be relying on them.  I cleaned all that stuff
up, which made this significantly simpler.

- I removed several of the columns being returned from the metapage.
The pageinspect code that I committed yesterday can be used to look at
those values; there doesn't seem to be a need to also return them
here.  What this is really useful for is getting the "real" values by
scanning through the whole index and tallying things up.

- I adjusted some of the data types, both in the SQL and in the C
code, so that they all match and that they're wide enough to return
the values they might contain without overflowing (but not wider).  I
also made the free_percent float8 rather than float4, consistent with
other things in this module.  One thing I cheated on slightly: I left
the version as int4 rather than int8 even though the underlying field
is uint32, for consistency with everything else in this module.
That's not entirely intellectually satisfying, but I guess it's better
to be consistent for the moment.

- I fixed a number of cosmetic issues, like the fact that the
documentation for this didn't use \x format, unlike all of the other
pgstattuple documentation, and the fact that table_len isn't a very
good name for a variable representing the total amount of space for
tuples, and the fact that the documentation referred to a hash index
as a "hash table", and the fact that the wording for the columns
wasn't consistent with other functions in pgstattuple with similar
columns.

Committed with those changes.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-02-01 Thread Michael Paquier 
On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
 wrote:
> Nothing else to add from my side. I'm marking this 'Ready for commiter'.

Moved to CF 2017-03 with the same status.
-- 
Michael


-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-24 Thread Kuntal Ghosh 
On Tue, Jan 24, 2017 at 10:09 AM, Ashutosh Sharma  wrote:
>> I've looked at the patch. It looks good. However, I was wondering why
>> an exclusive lock for extension is necessary for reading the number
>> blocks in this case. Please refer to the following code.
>>
>> +   /* Get the current relation length */
>> +   LockRelationForExtension(rel, ExclusiveLock);
>> +   nblocks = RelationGetNumberOfBlocks(rel);
>> +   UnlockRelationForExtension(rel, ExclusiveLock);
>>
>> Apart from this, I've nothing else to add.
>
>
> Actually in my case I may not need to acquire ExclusiveLock just to
> get the number of pages in a relation. Infact I have already opened a
> relation using
> 'AccessShareLock' which should be enough to read a table length.
> Thanks for putting this point. I have corrected it in the attached v5
> patch.
I've checked the last attached patch and didn't find any issue. There
is no problem with patching and compilation. It also passed all
regression tests.
Nothing else to add from my side. I'm marking this 'Ready for commiter'.

-- 
Thanks & Regards,
Kuntal Ghosh
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] Add pgstathashindex() to get hash index table statistics.

2017-01-23 Thread Ashutosh Sharma 
> I've looked at the patch. It looks good. However, I was wondering why
> an exclusive lock for extension is necessary for reading the number
> blocks in this case. Please refer to the following code.
>
> +   /* Get the current relation length */
> +   LockRelationForExtension(rel, ExclusiveLock);
> +   nblocks = RelationGetNumberOfBlocks(rel);
> +   UnlockRelationForExtension(rel, ExclusiveLock);
>
> Apart from this, I've nothing else to add.


Actually in my case I may not need to acquire ExclusiveLock just to
get the number of pages in a relation. Infact I have already opened a
relation using
'AccessShareLock' which should be enough to read a table length.
Thanks for putting this point. I have corrected it in the attached v5
patch. I think
in 'pgstat_index()' currently LockRelationForExtension(rel,
ExclusiveLock); is being used just to read a table length which I feel
is not required. You may raise this point in the community in a
separete mail thread if you agree with it. Also, I guess
LockRelationForExtension() is used when trying to add a new page in an
existing relation so not sure if it would be right to call it from
contrib modules like pgstattuple where we are just trying to read the
tables.

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


0001-Add-pgstathashindex-to-pgstattuple-extension-v5.patch
Description: invalid/octet-stream

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-23 Thread Kuntal Ghosh 
On Mon, Jan 23, 2017 at 2:56 PM, Ashutosh Sharma  wrote:
> Hi,
>
> Please find the attached v4 patch rebased on a latest commitID in
> head. I had to rebase it as the following git commit has some changes
> in pgstatindex.c file due to which 'git apply' was failing.
>
I've looked at the patch. It looks good. However, I was wondering why
an exclusive lock for extension is necessary for reading the number
blocks in this case. Please refer to the following code.

+   /* Get the current relation length */
+   LockRelationForExtension(rel, ExclusiveLock);
+   nblocks = RelationGetNumberOfBlocks(rel);
+   UnlockRelationForExtension(rel, ExclusiveLock);

Apart from this, I've nothing else to add.

-- 
Thanks & Regards,
Kuntal Ghosh
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] Add pgstathashindex() to get hash index table statistics.

2017-01-23 Thread Ashutosh Sharma 
Hi,

Please find the attached v4 patch rebased on a latest commitID in
head. I had to rebase it as the following git commit has some changes
in pgstatindex.c file due to which 'git apply' was failing.


commit f21a563d25dbae153937aec062161184189478b8
Author: Peter Eisentraut 
Date:   Fri Jan 20 20:29:53 2017 -0500

Move some things from builtins.h to new header files



On Thu, Jan 19, 2017 at 12:27 PM, Ashutosh Sharma  wrote:
>> However, I've some minor comments on the patch:
>>
>> +/*
>> + * HASH_ALLOCATABLE_PAGE_SZ represents allocatable
>> + * space (pd_upper - pd_lower) on a hash page.
>> + */
>> +#define HASH_ALLOCATABLE_PAGE_SZ \
>> +   BLCKSZ - \
>> +   (SizeOfPageHeaderData + sizeof(HashPageOpaqueData))
>> My suggestion will be not to write "(pd_upper - pd_lower)" in the
>> comment. You may write allocatable space on a empty hash page.
>
> Accepted. Have changed the comment accordingly.
>
>>
>> +   buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno,
>> RBM_NORMAL, NULL);
>> Use BAS_BULKREAD strategy to read the buffer.
>>
>
> okay, corrected. Please check the attached v3 patch with corrections.
>
> With Regards,
> Ashutosh Sharma
> EnterpriseDB: http://www.enterprisedb.com


0001-Add-pgstathashindex-to-pgstattuple-extension-v4.patch
Description: Binary data

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-18 Thread Ashutosh Sharma 
> However, I've some minor comments on the patch:
>
> +/*
> + * HASH_ALLOCATABLE_PAGE_SZ represents allocatable
> + * space (pd_upper - pd_lower) on a hash page.
> + */
> +#define HASH_ALLOCATABLE_PAGE_SZ \
> +   BLCKSZ - \
> +   (SizeOfPageHeaderData + sizeof(HashPageOpaqueData))
> My suggestion will be not to write "(pd_upper - pd_lower)" in the
> comment. You may write allocatable space on a empty hash page.

Accepted. Have changed the comment accordingly.

>
> +   buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno,
> RBM_NORMAL, NULL);
> Use BAS_BULKREAD strategy to read the buffer.
>

okay, corrected. Please check the attached v3 patch with corrections.

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


0001-Add-pgstathashindex-to-pgstattuple-extension-v3.patch
Description: invalid/octet-stream

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-18 Thread Kuntal Ghosh 
On Fri, Jan 6, 2017 at 6:58 PM, Ashutosh Sharma  wrote:

I've successfully applied the patch on the latest head and ran a
regression tests without any failure. There is no major changes.
However, I've some minor comments on the patch:

+/*
+ * HASH_ALLOCATABLE_PAGE_SZ represents allocatable
+ * space (pd_upper - pd_lower) on a hash page.
+ */
+#define HASH_ALLOCATABLE_PAGE_SZ \
+   BLCKSZ - \
+   (SizeOfPageHeaderData + sizeof(HashPageOpaqueData))
My suggestion will be not to write "(pd_upper - pd_lower)" in the
comment. You may write allocatable space on a empty hash page.

+   buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno,
RBM_NORMAL, NULL);
Use BAS_BULKREAD strategy to read the buffer.


-- 
Thanks & Regards,
Kuntal Ghosh
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] Add pgstathashindex() to get hash index table statistics.

2017-01-06 Thread Ashutosh Sharma 
Hi,

> I think the calculation for max available spcae is wrong here. You
> should subtract the page header and special area from the total page size.
> A check for non-zero denominator should be added while calculating the
> percentage.
> There can be multiple bitmap pages. Right?

Yes, we can have multiple bitmap pages. I have adjusted the
calculation for free space percentage accordingly. Please check the
attached v2 patch.

>
> +values[10] = Float8GetDatum(free_percent);
> Some precision should be added.

Corrected. Please refer v2 patch.

>
> +   
> +ffactor
> +integer
> +Average number of tuples per bucket
> +   
> I feel that either the column name should be changed or it should
> just output how full the index method is in percentage.

Fixed. Refer to v2 patch.

>
> +   
> +
> +
> +   
> Please remove extra spaces.

Done. Please refer to v2 patch.

>
> And, please add some test cases for regression tests.
>
Added a test-case. Please check v2 patch attached with this mail.

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


0001-Add-pgstathashindex-to-pgstattuple-extension-v2.patch
Description: invalid/octet-stream

-- 
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] Add pgstathashindex() to get hash index table statistics.

2017-01-05 Thread Kuntal Ghosh 
On Wed, Dec 21, 2016 at 7:22 PM, Ashutosh Sharma  wrote:
> Hi All,
>
> I have introduced a new function 'pgstathashindex()' inside pgstatuple
> extension to view the statistics related to hash index table.
>
I think this feature is going to be helpful. I've some initial review
for the patch.
I've not applied the patch yet.

+buf = ReadBufferExtended(rel, MAIN_FORKNUM, blkno, RBM_NORMAL, NULL);
Use ReadBuffer() instead.

+/*
+ * Let us ignore metapage and bitmap page when calculating
+ * free space percentage for tuples in a table.
+ */
+table_len = (stats.total_pages - 2) * BLCKSZ;
+
+free_percent = 100.0 * stats.free_space / table_len;
I think the calculation for max available spcae is wrong here. You
should subtract the page header and special area from the total page size.
A check for non-zero denominator should be added while calculating the
percentage.
There can be multiple bitmap pages. Right?

+values[10] = Float8GetDatum(free_percent);
Some precision should be added.

+   
+ffactor
+integer
+Average number of tuples per bucket
+   
I feel that either the column name should be changed or it should
just output how full the index method is in percentage.

+   
+
+
+   
Please remove extra spaces.

And, please add some test cases for regression tests.

-- 
Thanks & Regards,
Kuntal Ghosh
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] Add pgstathashindex() to get hash index table statistics.

2016-12-21 Thread Ashutosh Sharma 
Hi All,

I have introduced a new function 'pgstathashindex()' inside pgstatuple
extension to view the statistics related to hash index table. I could
have used 'pgstattuple()' function to view hash index stats instead of
adding this new function but there are certain limitations when using
pgstattuple() for hash indexes. Firstly, it doesn't work if a hash
index contains zero or new pages which is very common in case of hash
indexes. Secondly, it doesn't provide information about different
types of pages in hash index and its count. Considering these points,
I have thought of introducing this function. Attached is the patch for
the same. Please have a look and let me your feedback. I would also
like to mention that this idea basically came from my colleague Kuntal
Ghosh and i  implemented it. I have also created a commit-fest entry
for this submission. Thanks.

With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com
From a82d350b48b9daee017d2f31dee136809333ea82 Mon Sep 17 00:00:00 2001
From: ashu 
Date: Wed, 21 Dec 2016 18:39:47 +0530
Subject: [PATCH] Add pgstathashindex() to pgstattuple extension v1

This allows us to see the hash index table statistics.
We could have directly used pgstattuple() to see the
hash table statistics but it doesn't include some of
the stats related to hash index like number of bucket
pages, overflow pages, zero pages etc. Moreover, it
can't be used if hash index table contains a zero page.

Patch by Ashutosh. Needs review.
---
 contrib/pgstattuple/Makefile  |   8 +-
 contrib/pgstattuple/pgstatindex.c | 225 ++
 contrib/pgstattuple/pgstattuple--1.4.sql  |  15 ++
 contrib/pgstattuple/pgstattuple--1.5--1.6.sql |  22 +++
 contrib/pgstattuple/pgstattuple.control   |   2 +-
 doc/src/sgml/pgstattuple.sgml | 108 +
 6 files changed, 375 insertions(+), 5 deletions(-)
 create mode 100644 contrib/pgstattuple/pgstattuple--1.5--1.6.sql

diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index 294077d..a1601ec 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,10 +4,10 @@ MODULE_big	= pgstattuple
 OBJS		= pgstattuple.o pgstatindex.o pgstatapprox.o $(WIN32RES)
 
 EXTENSION = pgstattuple
-DATA = pgstattuple--1.4.sql pgstattuple--1.4--1.5.sql \
-	pgstattuple--1.3--1.4.sql pgstattuple--1.2--1.3.sql \
-	pgstattuple--1.1--1.2.sql pgstattuple--1.0--1.1.sql \
-	pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.5--1.6.sql pgstattuple--1.4--1.5.sql \
+	pgstattuple--1.4.sql pgstattuple--1.3--1.4.sql \
+	pgstattuple--1.2--1.3.sql pgstattuple--1.1--1.2.sql	\
+	pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
 PGFILEDESC = "pgstattuple - tuple-level statistics"
 
 REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index d9a722a..a3c8f23 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -29,6 +29,7 @@
 
 #include "access/gin_private.h"
 #include "access/heapam.h"
+#include "access/hash.h"
 #include "access/htup_details.h"
 #include "access/nbtree.h"
 #include "catalog/namespace.h"
@@ -36,6 +37,7 @@
 #include "funcapi.h"
 #include "miscadmin.h"
 #include "storage/bufmgr.h"
+#include "storage/lmgr.h"
 #include "utils/builtins.h"
 #include "utils/rel.h"
 
@@ -53,6 +55,7 @@ PG_FUNCTION_INFO_V1(pgstatindexbyid);
 PG_FUNCTION_INFO_V1(pg_relpages);
 PG_FUNCTION_INFO_V1(pg_relpagesbyid);
 PG_FUNCTION_INFO_V1(pgstatginindex);
+PG_FUNCTION_INFO_V1(pgstathashindex);
 
 PG_FUNCTION_INFO_V1(pgstatindex_v1_5);
 PG_FUNCTION_INFO_V1(pgstatindexbyid_v1_5);
@@ -60,11 +63,17 @@ PG_FUNCTION_INFO_V1(pg_relpages_v1_5);
 PG_FUNCTION_INFO_V1(pg_relpagesbyid_v1_5);
 PG_FUNCTION_INFO_V1(pgstatginindex_v1_5);
 
+PG_FUNCTION_INFO_V1(pgstathashindex_v1_6);
+
 Datum pgstatginindex_internal(Oid relid, FunctionCallInfo fcinfo);
+Datum pgstathashindex_internal(Oid relid, FunctionCallInfo fcinfo);
 
 #define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
 #define IS_BTREE(r) ((r)->rd_rel->relam == BTREE_AM_OID)
 #define IS_GIN(r) ((r)->rd_rel->relam == GIN_AM_OID)
+#define IS_HASH(r) ((r)->rd_rel->relam == HASH_AM_OID)
+
+#define HASH_HEAD_BLKNO HASH_METAPAGE + 1
 
 /* 
  * A structure for a whole btree index statistics
@@ -101,7 +110,29 @@ typedef struct GinIndexStat
 	int64		pending_tuples;
 } GinIndexStat;
 
+/* 
+ * A structure for a whole HASH index statistics
+ * used by pgstathashindex().
+ * 
+ */
+typedef struct HashIndexStat
+{
+	uint32	version;
+	uint32	total_pages;
+	uint32	bucket_pages;
+	uint32	overflow_pages;
+	uint32	bitmap_pages;
+	uint32	zero_pages;
+	uint64	ntuples;
+	uint16	ffactor;
+	uint64	live_items;
+	uint64	dead_items;
+	uint64	free_space;
+} HashIndexStat;
+
 static Datum