Thread: [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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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
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
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
Attachment
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 >
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
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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