Horton

Thread

Commits

  1. Another fix for single-page hash index vacuum.

  2. hash: Support WAL consistency checking.

  1. WAL Consistency checking for hash indexes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-02-28T05:36:04Z

    Hello everyone,

I've attached a patch which implements WAL consistency checking for
hash indexes. This feature is going to be useful for developing and
testing of WAL logging for hash index.

Note that, this patch should be applied on top of the following WAL
logging for hash index patch:
https://www.postgresql.org/message-id/CAA4eK1%2B%2BP%2BjVZC7MC3xzw5uLCpva9%2BgsBpd3semuWffWAftr5Q%40mail.gmail.com

-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  2. Re: WAL Consistency checking for hash indexes

    Amit Kapila <amit.kapila16@gmail.com> — 2017-03-03T04:14:11Z

    On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> Hello everyone,
>
> I've attached a patch which implements WAL consistency checking for
> hash indexes. This feature is going to be useful for developing and
> testing of WAL logging for hash index.
>

I think it is better if you base your patch on (Microvacuum support
for hash index - https://commitfest.postgresql.org/13/835/).

1.
There are some hints which we might want to mask that are used in that
patch.  For ex, I think you need to take care of Dead marking at page
level. Refer below code in patch "Microvacuum support for hash index".
+ if (killedsomething)
+ {
+ opaque->hasho_flag |= LH_PAGE_HAS_DEAD_TUPLES;


2.
+ else if ((opaque->hasho_flag & LH_BUCKET_PAGE) ||
+ (opaque->hasho_flag & LH_OVERFLOW_PAGE))
+ {
+ /*
+ * In btree bucket and overflow pages, it is possible to modify the
+ * LP_FLAGS without emitting any WAL record. Hence, mask the line
+ * pointer flags.
+ * See hashgettuple() for details.
+ */
+ mask_lp_flags(page);
+ }

Again, this mechanism is also modified by patch "Microvacuum support
for hash index", so above changes needs to be adjusted accordingly.
Comment referring to btree is wrong, you need to refer hash.

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

  3. Re: WAL Consistency checking for hash indexes

    Robert Haas <robertmhaas@gmail.com> — 2017-03-04T08:59:37Z

    On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> Hello everyone,
>>
>> I've attached a patch which implements WAL consistency checking for
>> hash indexes. This feature is going to be useful for developing and
>> testing of WAL logging for hash index.
>>
>
> I think it is better if you base your patch on (Microvacuum support
> for hash index - https://commitfest.postgresql.org/13/835/).

I'd rather have this based on top of the WAL logging patch, and have
any subsequent patches that tinker with the WAL logging include the
necessary consistency checking changes also.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  4. Re: WAL Consistency checking for hash indexes

    Amit Kapila <amit.kapila16@gmail.com> — 2017-03-04T09:47:20Z

    On Sat, Mar 4, 2017 at 2:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
>> <kuntalghosh.2007@gmail.com> wrote:
>>> Hello everyone,
>>>
>>> I've attached a patch which implements WAL consistency checking for
>>> hash indexes. This feature is going to be useful for developing and
>>> testing of WAL logging for hash index.
>>>
>>
>> I think it is better if you base your patch on (Microvacuum support
>> for hash index - https://commitfest.postgresql.org/13/835/).
>
> I'd rather have this based on top of the WAL logging patch, and have
> any subsequent patches that tinker with the WAL logging include the
> necessary consistency checking changes also.
>

Fair point.  I thought as the other patch has been proposed before
this patch, so it might be better to tackle the changes related to
that patch in this patch. However, changing the MicroVacuum or any
other patch to consider what needs to be masked for that patch sounds
sensible.


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

  5. Re: WAL Consistency checking for hash indexes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-03-08T09:02:22Z

    On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
> <kuntalghosh.2007@gmail.com> wrote:
>> Hello everyone,
>>
>> I've attached a patch which implements WAL consistency checking for
>> hash indexes. This feature is going to be useful for developing and
>> testing of WAL logging for hash index.
>>
>
> 2.
> + else if ((opaque->hasho_flag & LH_BUCKET_PAGE) ||
> + (opaque->hasho_flag & LH_OVERFLOW_PAGE))
> + {
> + /*
> + * In btree bucket and overflow pages, it is possible to modify the
> + * LP_FLAGS without emitting any WAL record. Hence, mask the line
> + * pointer flags.
> + * See hashgettuple() for details.
> + */
> + mask_lp_flags(page);
> + }
>
> Again, this mechanism is also modified by patch "Microvacuum support
> for hash index", so above changes needs to be adjusted accordingly.
> Comment referring to btree is wrong, you need to refer hash.
I've corrected the text in the comment and re-based the patch on the
latest hash index patch for WAL logging[1]. As discussed in the
thread, Microvacuum patch can be re-based on top of this patch.


[1] https://www.postgresql.org/message-id/CAA4eK1%2BmvCucroWQwX3S7aBR%3D0yBJGF%2BjQz4x4Cx9QVsMFTZUw%40mail.gmail.com
-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  6. Re: WAL Consistency checking for hash indexes

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2017-03-13T14:36:25Z

    Couple of review comments,,

You may also need to update the documentation as now we are also going
to support wal consistency check for hash index. The current
documentation does not include hash index.

+        only records originating from those resource managers.  Currently,
+        the supported resource managers are <literal>heap</>,
+        <literal>heap2</>, <literal>btree</>, <literal>gin</>,
+        <literal>gist</>, <literal>sequence</>, <literal>spgist</>,
+        <literal>brin</>, and <literal>generic</>. Only


Following comment in hash_mask() may require changes if patch for
'Microvacuum support for Hash Index - [1]' gets committed.

+       /*
+        * In hash bucket and overflow pages, it is possible to modify the
+        * LP_FLAGS without emitting any WAL record. Hence, mask the line
+        * pointer flags.
+        * See hashgettuple() for details.
+        */


[1] - https://www.postgresql.org/message-id/CAE9k0PmXyQpHX8%3DL_hFV7HfPV8qrit19xoUB86waQ87rKYzmYQ%40mail.gmail.com

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

On Wed, Mar 8, 2017 at 2:32 PM, Kuntal Ghosh <kuntalghosh.2007@gmail.com> wrote:
> On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
>> <kuntalghosh.2007@gmail.com> wrote:
>>> Hello everyone,
>>>
>>> I've attached a patch which implements WAL consistency checking for
>>> hash indexes. This feature is going to be useful for developing and
>>> testing of WAL logging for hash index.
>>>
>>
>> 2.
>> + else if ((opaque->hasho_flag & LH_BUCKET_PAGE) ||
>> + (opaque->hasho_flag & LH_OVERFLOW_PAGE))
>> + {
>> + /*
>> + * In btree bucket and overflow pages, it is possible to modify the
>> + * LP_FLAGS without emitting any WAL record. Hence, mask the line
>> + * pointer flags.
>> + * See hashgettuple() for details.
>> + */
>> + mask_lp_flags(page);
>> + }
>>
>> Again, this mechanism is also modified by patch "Microvacuum support
>> for hash index", so above changes needs to be adjusted accordingly.
>> Comment referring to btree is wrong, you need to refer hash.
> I've corrected the text in the comment and re-based the patch on the
> latest hash index patch for WAL logging[1]. As discussed in the
> thread, Microvacuum patch can be re-based on top of this patch.
>
>
> [1] https://www.postgresql.org/message-id/CAA4eK1%2BmvCucroWQwX3S7aBR%3D0yBJGF%2BjQz4x4Cx9QVsMFTZUw%40mail.gmail.com
> --
> 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
>

  7. Re: WAL Consistency checking for hash indexes

    Robert Haas <robertmhaas@gmail.com> — 2017-03-14T19:02:04Z

    On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Couple of review comments,,
>
> You may also need to update the documentation as now we are also going
> to support wal consistency check for hash index. The current
> documentation does not include hash index.
>
> +        only records originating from those resource managers.  Currently,
> +        the supported resource managers are <literal>heap</>,
> +        <literal>heap2</>, <literal>btree</>, <literal>gin</>,
> +        <literal>gist</>, <literal>sequence</>, <literal>spgist</>,
> +        <literal>brin</>, and <literal>generic</>. Only

Did that, committed this.  Also ran pgindent over hash_mask() and
fixed an instance of dubious capitalization.

> Following comment in hash_mask() may require changes if patch for
> 'Microvacuum support for Hash Index - [1]' gets committed.
>
> +       /*
> +        * In hash bucket and overflow pages, it is possible to modify the
> +        * LP_FLAGS without emitting any WAL record. Hence, mask the line
> +        * pointer flags.
> +        * See hashgettuple() for details.
> +        */

If that's so, then that patch is responsible for updating these
comments (and the code, if necessary).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  8. Re: WAL Consistency checking for hash indexes

    Kuntal Ghosh <kuntalghosh.2007@gmail.com> — 2017-03-15T05:57:10Z

    On Wed, Mar 15, 2017 at 12:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> Couple of review comments,,
>>
>> You may also need to update the documentation as now we are also going
>> to support wal consistency check for hash index. The current
>> documentation does not include hash index.
>>
>> +        only records originating from those resource managers.  Currently,
>> +        the supported resource managers are <literal>heap</>,
>> +        <literal>heap2</>, <literal>btree</>, <literal>gin</>,
>> +        <literal>gist</>, <literal>sequence</>, <literal>spgist</>,
>> +        <literal>brin</>, and <literal>generic</>. Only
>
> Did that, committed this.  Also ran pgindent over hash_mask() and
> fixed an instance of dubious capitalization.
Thanks Robert for the commit.


-- 
Thanks & Regards,
Kuntal Ghosh
EnterpriseDB: http://www.enterprisedb.com

  9. Re: WAL Consistency checking for hash indexes

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2017-03-16T07:45:19Z

    Hi,

Attached is the patch that allows WAL consistency tool to mask
'LH_PAGE_HAS_DEAD_TUPLES' flag in hash index. The flag got added as a
part of 'Microvacuum support for Hash index' patch . I have already
tested it using Kuntal's WAL consistency tool and it works fine.

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

On Wed, Mar 15, 2017 at 11:27 AM, Kuntal Ghosh
<kuntalghosh.2007@gmail.com> wrote:
> On Wed, Mar 15, 2017 at 12:32 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>> Couple of review comments,,
>>>
>>> You may also need to update the documentation as now we are also going
>>> to support wal consistency check for hash index. The current
>>> documentation does not include hash index.
>>>
>>> +        only records originating from those resource managers.  Currently,
>>> +        the supported resource managers are <literal>heap</>,
>>> +        <literal>heap2</>, <literal>btree</>, <literal>gin</>,
>>> +        <literal>gist</>, <literal>sequence</>, <literal>spgist</>,
>>> +        <literal>brin</>, and <literal>generic</>. Only
>>
>> Did that, committed this.  Also ran pgindent over hash_mask() and
>> fixed an instance of dubious capitalization.
> Thanks Robert for the commit.
>
>
> --
> Thanks & Regards,
> Kuntal Ghosh
> EnterpriseDB: http://www.enterprisedb.com

  10. Re: WAL Consistency checking for hash indexes

    Amit Kapila <amit.kapila16@gmail.com> — 2017-03-17T03:33:02Z

    On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Hi,
>
> Attached is the patch that allows WAL consistency tool to mask
> 'LH_PAGE_HAS_DEAD_TUPLES' flag in hash index. The flag got added as a
> part of 'Microvacuum support for Hash index' patch . I have already
> tested it using Kuntal's WAL consistency tool and it works fine.
>

+ * unlogged. So, mask it. See _hash_kill_items(), MarkBufferDirtyHint()
+ * for
details.
+ */

I think in above comment, a reference to _hash_kill_items is
sufficient.  Other than that patch looks okay.

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

  11. Re: WAL Consistency checking for hash indexes

    Ashutosh Sharma <ashu.coek88@gmail.com> — 2017-03-17T07:00:19Z

    On Fri, Mar 17, 2017 at 9:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>> Hi,
>>
>> Attached is the patch that allows WAL consistency tool to mask
>> 'LH_PAGE_HAS_DEAD_TUPLES' flag in hash index. The flag got added as a
>> part of 'Microvacuum support for Hash index' patch . I have already
>> tested it using Kuntal's WAL consistency tool and it works fine.
>>
>
> + * unlogged. So, mask it. See _hash_kill_items(), MarkBufferDirtyHint()
> + * for
> details.
> + */
>
> I think in above comment, a reference to _hash_kill_items is
> sufficient.  Other than that patch looks okay.

Okay, I have removed the reference to MarkBufferDirtyHint() from above
comment. Attached is the v2 version of patch.

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

  12. Re: WAL Consistency checking for hash indexes

    Amit Kapila <amit.kapila16@gmail.com> — 2017-03-17T12:30:52Z

    On Fri, Mar 17, 2017 at 12:30 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> On Fri, Mar 17, 2017 at 9:03 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>>> Hi,
>>>
>>> Attached is the patch that allows WAL consistency tool to mask
>>> 'LH_PAGE_HAS_DEAD_TUPLES' flag in hash index. The flag got added as a
>>> part of 'Microvacuum support for Hash index' patch . I have already
>>> tested it using Kuntal's WAL consistency tool and it works fine.
>>>
>>
>> + * unlogged. So, mask it. See _hash_kill_items(), MarkBufferDirtyHint()
>> + * for
>> details.
>> + */
>>
>> I think in above comment, a reference to _hash_kill_items is
>> sufficient.  Other than that patch looks okay.
>
> Okay, I have removed the reference to MarkBufferDirtyHint() from above
> comment. Attached is the v2 version of patch.
>

Thanks, this version looks good to me.



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