Re: Existence check for suitable index in advance when concurrently refreshing.

On Wed, Feb 10, 2016 at 10:35 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Feb 10, 2016 at 2:23 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Wed, Feb 10, 2016 at 2:21 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>>> On Tue, Feb 9, 2016 at 9:11 PM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Tue, Feb 9, 2016 at 4:27 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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