Thread: Existence check for suitable index in advance when concurrently refreshing.
Existence check for suitable index in advance when concurrently refreshing.
From
Masahiko Sawada
Date:
Hi all, In concurrently refreshing materialized view, we check whether that materialized view has suitable index(unique and not having WHERE condition), after filling data to new snapshot (refresh_matview_datafill()). This logic leads to taking a lot of time until postgres returns ERROR log if that table doesn't has suitable index and table is large. it wastes time. I think we should check whether that materialized view can use concurrently refreshing or not in advance. The patch is attached. Please give me feedbacks. -- Regards, -- Masahiko Sawada
Attachment
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Fujii Masao
Date:
On Tue, Jan 26, 2016 at 9:33 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > Hi all, > > In concurrently refreshing materialized view, we check whether that > materialized view has suitable index(unique and not having WHERE > condition), after filling data to new snapshot > (refresh_matview_datafill()). > This logic leads to taking a lot of time until postgres returns ERROR > log if that table doesn't has suitable index and table is large. it > wastes time. > I think we should check whether that materialized view can use > concurrently refreshing or not in advance. +1 > The patch is attached. > > Please give me feedbacks. + indexRel = index_open(indexoid, RowExclusiveLock); Can we use AccessShareLock here, instead? + if (indexStruct->indisunique && + IndexIsValid(indexStruct) && + RelationGetIndexExpressions(indexRel) == NIL && + RelationGetIndexPredicate(indexRel) == NIL) + hasUniqueIndex = true; + + index_close(indexRel, RowExclusiveLock); In the case where hasUniqueIndex = true, ISTM that we can get out of the loop immediately just after calling index_close(). No? + /* Must have at least one unique index */ + Assert(foundUniqueIndex); Can we guarantee that there is at least one valid unique index here? If yes, it's better to write the comment about that. Regards, -- Fujii Masao
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Masahiko Sawada
Date:
On Wed, Jan 27, 2016 at 4:42 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Tue, Jan 26, 2016 at 9:33 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >> Hi all, >> >> In concurrently refreshing materialized view, we check whether that >> materialized view has suitable index(unique and not having WHERE >> condition), after filling data to new snapshot >> (refresh_matview_datafill()). >> This logic leads to taking a lot of time until postgres returns ERROR >> log if that table doesn't has suitable index and table is large. it >> wastes time. >> I think we should check whether that materialized view can use >> concurrently refreshing or not in advance. > > +1 > >> The patch is attached. >> >> Please give me feedbacks. Thank you for having look at this patch. > + indexRel = index_open(indexoid, RowExclusiveLock); > > Can we use AccessShareLock here, instead? Yeah, I think we can use it. Fixed. > + if (indexStruct->indisunique && > + IndexIsValid(indexStruct) && > + RelationGetIndexExpressions(indexRel) == NIL && > + RelationGetIndexPredicate(indexRel) == NIL) > + hasUniqueIndex = true; > + > + index_close(indexRel, RowExclusiveLock); > > In the case where hasUniqueIndex = true, ISTM that we can get out of > the loop immediately just after calling index_close(). No? Fixed. > + /* Must have at least one unique index */ > + Assert(foundUniqueIndex); > > Can we guarantee that there is at least one valid unique index here? > If yes, it's better to write the comment about that. > Added. Attached latest patch. Please review it. Regards, -- Masahiko Sawada
Attachment
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Fujii Masao
Date:
On Thu, Jan 28, 2016 at 1:01 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Wed, Jan 27, 2016 at 4:42 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Tue, Jan 26, 2016 at 9:33 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote: >>> Hi all, >>> >>> In concurrently refreshing materialized view, we check whether that >>> materialized view has suitable index(unique and not having WHERE >>> condition), after filling data to new snapshot >>> (refresh_matview_datafill()). >>> This logic leads to taking a lot of time until postgres returns ERROR >>> log if that table doesn't has suitable index and table is large. it >>> wastes time. >>> I think we should check whether that materialized view can use >>> concurrently refreshing or not in advance. >> >> +1 >> >>> The patch is attached. >>> >>> Please give me feedbacks. > > Thank you for having look at this patch. > >> + indexRel = index_open(indexoid, RowExclusiveLock); >> >> Can we use AccessShareLock here, instead? > > Yeah, I think we can use it. Fixed. > >> + if (indexStruct->indisunique && >> + IndexIsValid(indexStruct) && >> + RelationGetIndexExpressions(indexRel) == NIL && >> + RelationGetIndexPredicate(indexRel) == NIL) >> + hasUniqueIndex = true; >> + >> + index_close(indexRel, RowExclusiveLock); >> >> In the case where hasUniqueIndex = true, ISTM that we can get out of >> the loop immediately just after calling index_close(). No? > > Fixed. > >> + /* Must have at least one unique index */ >> + Assert(foundUniqueIndex); >> >> Can we guarantee that there is at least one valid unique index here? >> If yes, it's better to write the comment about that. >> > > Added. > > Attached latest patch. Please review it. Thanks for updating the patch! Attached is the updated version of the patch. I removed unnecessary assertion check and change of source code that you added, and improved the source comment. Barring objection, I'll commit this patch. Regards, -- Fujii Masao
Attachment
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Michael Paquier
Date:
On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote: > Thanks for updating the patch! > Attached is the updated version of the patch. > I removed unnecessary assertion check and change of source code > that you added, and improved the source comment. > Barring objection, I'll commit this patch. So, this code basically duplicates what is already in refresh_by_match_merge to check if there is a UNIQUE index defined. If we are sure that an error is detected earlier in the code as done in this patch, wouldn't it be better to replace the error message in refresh_by_match_merge() by an assertion? Just wondering, I would think that once this patch is applied the existing error message of refresh_by_match_merge() is just dead code. -- Michael
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Fujii Masao
Date:
On Tue, Feb 9, 2016 at 9:11 PM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >> Thanks for updating the patch! >> Attached is the updated version of the patch. >> I removed unnecessary assertion check and change of source code >> that you added, and improved the source comment. >> Barring objection, I'll commit this patch. > > So, this code basically duplicates what is already in > refresh_by_match_merge to check if there is a UNIQUE index defined. If > we are sure that an error is detected earlier in the code as done in > this patch, wouldn't it be better to replace the error message in > refresh_by_match_merge() by an assertion? I'm OK with an assertion if we add source comment about why refresh_by_match_merge() can always guarantee that there is a unique index on the matview. Probably it's because the matview is locked with exclusive lock at the start of ExecRefreshMatView(), i.e., it's guaranteed that we cannot drop any indexes on the matview after the first check is passed. Also it might be better to add another comment about that the caller of refresh_by_match_merge() must always check that there is a unique index on the matview before calling that function, just in the case where it's called elsewhere in the future. OTOH, I don't think it's not so bad idea to just emit an error, instead. Regards, -- Fujii Masao
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Fujii Masao
Date:
On Wed, Feb 10, 2016 at 2:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Tue, Feb 9, 2016 at 9:11 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >> On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>> Thanks for updating the patch! >>> Attached is the updated version of the patch. >>> I removed unnecessary assertion check and change of source code >>> that you added, and improved the source comment. >>> Barring objection, I'll commit this patch. >> >> So, this code basically duplicates what is already in >> refresh_by_match_merge to check if there is a UNIQUE index defined. If >> we are sure that an error is detected earlier in the code as done in >> this patch, wouldn't it be better to replace the error message in >> refresh_by_match_merge() by an assertion? > > I'm OK with an assertion if we add source comment about why > refresh_by_match_merge() can always guarantee that there is > a unique index on the matview. Probably it's because the matview > is locked with exclusive lock at the start of ExecRefreshMatView(), > i.e., it's guaranteed that we cannot drop any indexes on the matview > after the first check is passed. Also it might be better to add > another comment about that the caller of refresh_by_match_merge() > must always check that there is a unique index on the matview before > calling that function, just in the case where it's called elsewhere > in the future. > > OTOH, I don't think it's not so bad idea to just emit an error, instead. Sorry, s/it's not/it's Regards, -- Fujii Masao
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Michael Paquier
Date:
On Wed, Feb 10, 2016 at 2:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote: > On Wed, Feb 10, 2016 at 2:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Tue, Feb 9, 2016 at 9:11 PM, Michael Paquier >> <michael.paquier@gmail.com> wrote: >>> On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>>> Thanks for updating the patch! >>>> Attached is the updated version of the patch. >>>> I removed unnecessary assertion check and change of source code >>>> that you added, and improved the source comment. >>>> Barring objection, I'll commit this patch. >>> >>> So, this code basically duplicates what is already in >>> refresh_by_match_merge to check if there is a UNIQUE index defined. If >>> we are sure that an error is detected earlier in the code as done in >>> this patch, wouldn't it be better to replace the error message in >>> refresh_by_match_merge() by an assertion? >> >> I'm OK with an assertion if we add source comment about why >> refresh_by_match_merge() can always guarantee that there is >> a unique index on the matview. Probably it's because the matview >> is locked with exclusive lock at the start of ExecRefreshMatView(), >> i.e., it's guaranteed that we cannot drop any indexes on the matview >> after the first check is passed. Also it might be better to add >> another comment about that the caller of refresh_by_match_merge() >> must always check that there is a unique index on the matview before >> calling that function, just in the case where it's called elsewhere >> in the future. >> >> OTOH, I don't think it's not so bad idea to just emit an error, instead. > > Sorry, s/it's not/it's Well, refresh_by_match_merge is called only by ExecRefreshMatView() and it is not exposed externally in matview.h, so it is not like we are going to break any extension-related code by having an assertion instead of an error message, and that's less code duplication to maintain if this extra error message is removed. But feel free to withdraw my comment if you think that's not worth it, I won't deadly insist on that either :) -- Michael
Re: Existence check for suitable index in advance when concurrently refreshing.
From
Fujii Masao
Date:
On Wed, Feb 10, 2016 at 10:35 AM, Michael Paquier <michael.paquier@gmail.com> wrote: > On Wed, Feb 10, 2016 at 2:23 AM, Fujii Masao <masao.fujii@gmail.com> wrote: >> On Wed, Feb 10, 2016 at 2:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote: >>> On Tue, Feb 9, 2016 at 9:11 PM, Michael Paquier >>> <michael.paquier@gmail.com> wrote: >>>> On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote: >>>>> Thanks for updating the patch! >>>>> Attached is the updated version of the patch. >>>>> I removed unnecessary assertion check and change of source code >>>>> that you added, and improved the source comment. >>>>> Barring objection, I'll commit this patch. >>>> >>>> So, this code basically duplicates what is already in >>>> refresh_by_match_merge to check if there is a UNIQUE index defined. If >>>> we are sure that an error is detected earlier in the code as done in >>>> this patch, wouldn't it be better to replace the error message in >>>> refresh_by_match_merge() by an assertion? >>> >>> I'm OK with an assertion if we add source comment about why >>> refresh_by_match_merge() can always guarantee that there is >>> a unique index on the matview. Probably it's because the matview >>> is locked with exclusive lock at the start of ExecRefreshMatView(), >>> i.e., it's guaranteed that we cannot drop any indexes on the matview >>> after the first check is passed. Also it might be better to add >>> another comment about that the caller of refresh_by_match_merge() >>> must always check that there is a unique index on the matview before >>> calling that function, just in the case where it's called elsewhere >>> in the future. >>> >>> OTOH, I don't think it's not so bad idea to just emit an error, instead. >> >> Sorry, s/it's not/it's > > Well, refresh_by_match_merge is called only by ExecRefreshMatView() > and it is not exposed externally in matview.h, so it is not like we > are going to break any extension-related code by having an assertion > instead of an error message, and that's less code duplication to > maintain if this extra error message is removed. But feel free to > withdraw my comment if you think that's not worth it, I won't deadly > insist on that either :) Okay, I revived Sawada's original assertion code and pushed the patch. Thanks! Regards, -- Fujii Masao