Postgres Pro
Downloads
Home   >   mailing lists

Thread: matview patch readability/correctness gripe

matview patch readability/correctness gripe

From
Tom Lane
Date:
While looking for the cause of Erikjan Rijkers' recent report, my
attention was drawn to this hunk of the matview patch:

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.
The third argument of checkRuleResultList is "bool isSelect", defined
thus:
* The targetList might be either a SELECT targetlist, or a RETURNING list;* isSelect tells which.  (This is mostly used
forchoosing error messages,* but also we don't enforce column name matching for RETURNING.)
 

So the above hunk has at the very least rendered the documentation of
checkRuleResultList a lie, but I suspect it's broken the logic too.
I see no very good reason why matviews should work differently from
regular views at all here.  But even if there is some aspect of
checkRuleResultList that should work differently for matviews, I really
doubt that we want error messages related to them to be saying
"RETURNING list" when the error is about the SELECT list.

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.
        regards, tom lane

Re: matview patch readability/correctness gripe

From
Kevin Grittner
Date:
Tom Lane <tgl@sss.pgh.pa.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.

Thanks!

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

Re: matview patch readability/correctness gripe

From
Kevin Grittner
Date:
Kevin Grittner <kgrittn@ymail.com> wrote:
> Tom Lane <tgl@sss.pgh.pa.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