Обсуждение: Existence check for suitable index in advance when concurrently refreshing.

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

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

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

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

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

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

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

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

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