On Thu, Mar 23, 2017 at 6:46 PM, Ashutosh Sharma <ashu.coe...@gmail.com> wrote:
>> 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?
> Yes, I got your point. I have done that but then i had to remove the
> check for PageIsEmpty(). Anyways, I think PageIsEmpty() condition will
> only be true for one page in entire hash index table and can be
> ignored. If you wish, I could mention about it in the documentation.

Yeah, I think it is worth adding a Note in docs, but we can do that
separately if required.

>>> 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,
>>>> + 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.
> okay, I have now anyways removed the check for PageIsEmpty. Please
> refer to the attached '0002
> allow_pageinspect_handle_UNUSED_hash_pages.patch'

        if (pageopaque->hasho_page_id != HASHO_PAGE_ID)

spurious white space.

> Also, I have attached
> '0001-Mark-freed-overflow-page-as-UNUSED-pagetype-v2.patch' that
> handles your comment mentioned in [1].

In general, we have to initialize prevblock with max_bucket, but here
it is okay, because we anyway initialize it after this page is
allocated as overflow page.

Your patches look good to me except for small white space change.

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:

Reply via email to