Re: pageinspect and hash indexes

On Thu, Mar 23, 2017 at 10:02 AM, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com> wrote:
>>>
>>
>> 1.
>> @@ -70,6 +70,17 @@ verify_hash_page(bytea *raw_page, int flags)
>> pageopaque = (HashPageOpaque) PageGetSpecialPointer(page);
>> +
>> + /* Check if it is an unused hash page. */
>> + if (pageopaque->hasho_flag == LH_UNUSED_PAGE)
>> + return page;
>>
>>
>> I don't see we need to have a separate check for unused page, why
>> can't it be checked with other page types in below check.
>>
>> if (pagetype != LH_OVERFLOW_PAGE && pagetype != LH_BUCKET_PAGE &&
>> pagetype != LH_BITMAP_PAGE && pagetype != LH_META_PAGE)
>
> That is because UNUSED page is as good as an empty page except that
> empty page doesn't have any pagetype. If we add condition for checking
> UNUSED page in above if check it will never show unused page as an
> unsed page rather it will always show it as an empty page.
>

Oh, okay, but my main objection was that we should not check hash page
type (hasho_flag) without ensuring whether it is a hash page. Can you
try to adjust the above code so that this check can be moved after
hasho_page_id check?

> To avoid
> this, at the start of verify_hash_page function itself if we recognise
> page as UNUSED page we return immediately.
>
>>
>> 2.
>> + /* Check if it is an empty hash page. */
>> + if (PageIsEmpty(page))
>> + ereport(ERROR,
>> + (errcode(ERRCODE_INDEX_CORRUPTED),
>> + errmsg("index table contains empty page")));
>>
>>
>> Do we want to give a separate message for EMPTY and NEW pages? Isn't
>> it better that the same error message can be given for both of them as
>> from user perspective there is not much difference between both the
>> messages?
>
> I think we should show separate message because they are two different
> type of pages. In the sense like, one is initialised whereas other is
> completely zero.
>

I understand your point, but not sure if it makes any difference to user.

--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com