Re: pageinspect: Hash index support

On Wed, Feb 8, 2017 at 11:26 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Feb 8, 2017 at 11:58 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>> And then, instead, you need to add some code to set bit based on the
>>> bitmap page, like what you have:
>>>
>>> +    mapbuf = _hash_getbuf(indexRel, bitmapblkno, HASH_READ, LH_BITMAP_PAGE);
>>> +    mappage = BufferGetPage(mapbuf);
>>> +    freep = HashPageGetBitmap(mappage);
>>> +
>>> +    if (ISSET(freep, bitmapbit))
>>> +        bit = 1;
>>>
>>> Except I would write that last line as...
>>>
>>> bit = ISSET(freep, bitmapbit) != 0
>>>
>>> ...instead of using an if statement.
>>>
>>> I'm sort of confused as to why the idea of not reading the underlying
>>> page is so hard to understand.
>>
>> It was not so hard to understand your point. The only reason for
>> reading overflow page is to ensure that we are passing an overflow
>> block as input to '_hash_ovflblkno_to_bitno(metap, (BlockNumber)
>> ovflblkno)'. I had removed the code for reading an overflow page
>> assuming that _hash_ovflblkno_to_bitno() will throw an error if we
>> pass block number of a page other than overflow page but, it doesn't
>> seem to guarantee that it will always return value for an overflow
>> page. Here are my observations,
>>
>> postgres=# select hash_page_type(get_raw_page('con_hash_index', 65));
>>  hash_page_type
>> ----------------
>>  bucket
>> (1 row)
>>
>> postgres=# SELECT * FROM
>> hash_bitmap_info(get_raw_page('con_hash_index', 0), 'con_hash_index',
>> 65);
>>  bitmapblkno | bitmapbit | bitstatus
>> -------------+-----------+-----------
>>           33 |         0 | t
>> (1 row)
>>
>> postgres=# select hash_page_type(get_raw_page('con_hash_index', 64));
>>  hash_page_type
>> ----------------
>>  bucket
>> (1 row)
>>
>> postgres=# SELECT * FROM
>> hash_bitmap_info(get_raw_page('con_hash_index', 0), 'con_hash_index',
>> 64);
>> ERROR:  invalid overflow block number 64
>>
>> postgres=# select hash_page_type(get_raw_page('con_hash_index', 63));
>>  hash_page_type
>> ----------------
>>  bucket
>> (1 row)
>>
>> postgres=# SELECT * FROM
>> hash_bitmap_info(get_raw_page('con_hash_index', 0), 'con_hash_index',
>> 63);
>> ERROR:  invalid overflow block number 63
>>
>> postgres=# select hash_page_type(get_raw_page('con_hash_index', 33));
>>  hash_page_type
>> ----------------
>>  bitmap
>> (1 row)
>>
>> postgres=# SELECT * FROM
>> hash_bitmap_info(get_raw_page('con_hash_index', 0), 'con_hash_index',
>> 33);
>>  bitmapblkno | bitmapbit | bitstatus
>> -------------+-----------+-----------
>>           33 |         0 | t
>> (1 row)
>
> Right, I understand.  But if the caller cares about that, they can use
> hash_page_type() in order to find out whether the result of
> hash_bitmap_info() will be meaningful.  The problem with the way
> you've done it is that if someone wants to check the status of a
> million bitmap bits, that currently requires reading a million pages
> (8GB) whereas if you don't read the underlying page it requires
> reading only enough pages to contain a million bitmap bits (~128kB).
> That's a big difference.
>
> If you want to verify that the supplied page number is an overflow
> page before returning the bit, I think you should do that calculation
> based on the metapage.  There's enough information in hashm_spares to
> figure out which pages are primary bucket pages as opposed to overflow
> pages, and there's enough information in hashm_mapp to identify which
> pages are bitmap pages, and the metapage is always page 0.  If you
> take that approach, then you can check a million bitmap bits reading
> only the relevant bitmap pages plus the metapage, which is a LOT less
> I/O than reading a million index pages.
>

Thanks for the inputs. Attached is the patch modified as per your suggestions.

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