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