Add pgstathashindex() to get hash index table statistics.

On Wed, Dec 21, 2016 at 7:22 PM, Ashutosh Sharma <ashu.coek88@gmail.com> 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.

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

+   </varlistentry>
+
+
+   <varlistentry>
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

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.

+       <row>
+        <entry><structfield>ffactor</structfield></entry>
+        <entry><type>integer</type></entry>
+        <entry>Average number of tuples per bucket</entry>
+       </row>
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.

+   </varlistentry>
+
+
+   <varlistentry>
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

On Fri, Jan 6, 2017 at 6:58 PM, Ashutosh Sharma <ashu.coek88@gmail.com> 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

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

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.

<git-commit>
commit f21a563d25dbae153937aec062161184189478b8
Author: Peter Eisentraut <peter_e@gmx.net>
Date: Fri Jan 20 20:29:53 2017 -0500

Move some things from builtins.h to new header files

</git-commit>

On Mon, Jan 23, 2017 at 2:56 PM, Ashutosh Sharma <ashu.coek88@gmail.com> 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

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

On Tue, Jan 24, 2017 at 10:09 AM, Ashutosh Sharma <ashu.coek88@gmail.com> 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

On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> 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

On Wed, Feb 1, 2017 at 10:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> 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

On Sat, Feb 4, 2017 at 1:12 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Wed, Feb 1, 2017 at 10:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:

On Tue, Jan 24, 2017 at 8:36 PM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> 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

On Mon, Feb 6, 2017 at 10:06 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

On Sat, Feb 4, 2017 at 1:12 AM, Robert Haas <robertmhaas@gmail.com> 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

On Sun, Feb 5, 2017 at 11:36 PM, Ashutosh Sharma <ashu.coek88@gmail.com> 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

On Tue, Feb 7, 2017 at 2:04 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Sun, Feb 5, 2017 at 11:36 PM, Ashutosh Sharma <ashu.coek88@gmail.com> 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

On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila <amit.kapila16@gmail.com> 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

Hi,

On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila <amit.kapila16@gmail.com> 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

On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

Hi,

On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila <amit.kapila16@gmail.com> 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

On Sat, Mar 25, 2017 at 11:02 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

Hi,

On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila <amit.kapila16@gmail.com> 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]/messages/by-id/CA+TgmobwLKpKe99qnTJCBzFB4UaVGKrLNX3hwp8wcxObMDx7pA@mail.gmail.com.

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]: /messages/by-id/CA+TgmobwLKpKe99qnTJCBzFB4UaVGKrLNX3hwp8wcxObMDx7pA@mail.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

On Sat, Mar 25, 2017 at 12:33 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

On Sat, Mar 25, 2017 at 11:02 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, Mar 23, 2017 at 11:24 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:

Hi,

On Tue, Feb 7, 2017 at 9:23 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Feb 6, 2017 at 10:40 PM, Amit Kapila <amit.kapila16@gmail.com> 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