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