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

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