Re: matview patch readability/correctness gripe

Kevin Grittner <kgrittn(at)ymail(dot)com> wrote:
> Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
>> diff --git a/src/backend/rewrite/rewriteDefine.c
>> b/src/backend/rewrite/rewriteDefine.c
>> index a1a9808e5d94959218b415ed34c46579c478c177..896326615753f2344b466eb180080174ddeda31d 100644
>> *** a/src/backend/rewrite/rewriteDefine.c
>> --- b/src/backend/rewrite/rewriteDefine.c
>> *************** DefineQueryRewrite(char *rulename,
>> *** 356,362 ****
>>            */
>>            checkRuleResultList(query->targetList,
>>                                RelationGetDescr(event_relation),
>> !                            true);
>>  
>>            /*
>>            * ... there must not be another ON SELECT rule already ...
>> --- 357,364 ----
>>            */
>>            checkRuleResultList(query->targetList,
>>                                RelationGetDescr(event_relation),
>> !                            event_relation->rd_rel->relkind !=
>> !                                RELKIND_MATVIEW);
>>  
>>            /*
>>            * ... there must not be another ON SELECT rule already ...
>>
>> IMO this is either flat-out wrong, or an unmaintainable crock, or both.
>
>> So this either needs to be reverted, or refactored into two arguments
>> not one, with checkRuleResultList being updated to account honestly
>> and readably for whatever it's supposed to be doing here.
>
> Will review in light of your comments.

Yeah, it was flat-out wrong.  I managed to misread the comment
which describes the parameter, although in retrospect it seems
clear enough.  Reverted.

Thanks for noticing that.

--
Kevin Grittner
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company