Re: [HACKERS] WAL Consistency checking for hash indexes
On Fri, Mar 17, 2017 at 12:30 PM, Ashutosh Sharma wrote: > On Fri, Mar 17, 2017 at 9:03 AM, Amit Kapila wrote: >> On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Fri, Mar 17, 2017 at 9:03 AM, Amit Kapila wrote: > On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma > 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 0001-Allow-WAL-consistency-tool-to-mask-LH_PAGE_HAS_DEAD_.patch Description: binary/octet-stream -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Thu, Mar 16, 2017 at 1:15 PM, Ashutosh Sharma 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
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 wrote: > On Wed, Mar 15, 2017 at 12:32 AM, Robert Haas wrote: >> On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma >> 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 heap, >>> +heap2, btree, gin, >>> +gist, sequence, spgist, >>> +brin, and 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 0001-Allow-WAL-consistency-tool-to-mask-LH_PAGE_HAS_DEAD_.patch Description: application/download -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Wed, Mar 15, 2017 at 12:32 AM, Robert Haas wrote: > On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma > 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 heap, >> +heap2, btree, gin, >> +gist, sequence, spgist, >> +brin, and 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Mon, Mar 13, 2017 at 10:36 AM, Ashutosh Sharma 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 heap, > +heap2, btree, gin, > +gist, sequence, spgist, > +brin, and 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
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 heap,
+heap2, btree, gin,
+gist, sequence, spgist,
+brin, and 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 wrote:
> On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila wrote:
>> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
>> 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
>
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila wrote:
> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
> 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
0001-wal_consistency_checking-for-hash-index_v1.patch
Description: binary/octet-stream
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Sat, Mar 4, 2017 at 2:29 PM, Robert Haas wrote: > On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila wrote: >> On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Fri, Mar 3, 2017 at 9:44 AM, Amit Kapila wrote: > On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh > 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] WAL Consistency checking for hash indexes
On Tue, Feb 28, 2017 at 11:06 AM, Kuntal Ghosh
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] WAL Consistency checking for hash indexes
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 0001-wal_consistency_checking-for-hash-index.patch Description: application/download -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
