Re: [HACKERS] Add pgstathashindex() to get hash index table statistics.
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.
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.
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.
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.
>> >> +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.
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.
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.
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.
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
+++ b/contrib/pgstattuple/pgstattuple--1.4--1.5.sql
@@ -118,6 +118,7 @@ CREATE OR
Re: [HACKERS] Add pgstathashindex() to get hash index table statistics.
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.
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.
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.
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.
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.
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.
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.
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.
> 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.
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.
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.
> 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.
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.
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.
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.
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 pgstatindex_impl(Relation rel, Functi
