Re: [PATCH] remove redundant ownership checks

(2009/12/18 6:38), Stephen Frost wrote:
> KaiGai,
>
> * KaiGai Kohei (kaigai(at)ak(dot)jp(dot)nec(dot)com) wrote:
>> The patch was not attached...
>
> This patch either does too much, or not enough.
>
> I would either leave the Assert() in-place as a double-check (I presume
> that's why it was there in the first place, and if that Assert() fails
> then our assumption about the permissions check being already done on
> the object in question would be wrong, since the check is done against
> the passed-in 'rel' and the assert is that 'rel' and 'ruletup->ev_class'
> are the same; if they're not, then we might need to do perms checking on
> ruletup->ev_class)
>
> Or
>
> Remove the now-unused variable eventRelationOid.

My preference is the later option, because the pg_rewrite entry to be
checked is fetched using RULERELNAME syscache which takes OID of the
relation and name of the rule.
If this Assert() failed, it implies syscache mechanism has problem,
not only integrity of pg_rewrite system catalog.

The attached patch is an revised one.

Thanks,

> Overall, I agree with removing this check as it's already done by
> ATSimplePermissions() and we don't double-check the permissions in the
> other things called through ATExecCmd() (though there are cases where
> specific commands have to do *additional* checks beyond what
> ATSimplePermissions() does.. it might be worth looking into what those
> are and thinking about if we should move/consolidate/etc them, or if it
> makes sense to leave them where they are).
>
> Thanks,
>
> Stephen

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>