Re: [PATCH] remove redundant ownership checks

On Wed, Jan 13, 2010 at 1:34 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Mon, Jan 11, 2010 at 8:27 PM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
>>> I agree.  Why are arbitrary restrictions being placed on code
>>> improvements?  If code has no purpose, why not remove it, or at least
>>> mark it as NOT_USED.
>
>> So, where do we go from here?  Any other opinions?  I'm not sure how
>> much it's really worth fighting over a six line patch, but there's
>> something in me that rails against the idea of telling someone who
>> took the trouble to write a patch "no" when the only argument against
>> it is that we might change our mind at some point in the future.  Of
>> course, I will accept the consensus of the community whatever it is,
>> but the only people who have expressed a clear opinion on this version
>> of the patch are Tom, Bruce, and myself, and 2-1 is not a consensus.
>
> I still say that this patch is putting the cart before the horse.
> What we need before we do any significant amount of rearrangement
> of security checks is to have a plan for where they should go.
> Making a change here and a change there without a plan isn't an
> improvement, it just risks creating bugs.
>
> If I thought this patch represented incremental movement in the
> direction of a better security-check factorization, I'd be fine with it,
> but that's not clear either.  The argument for it is that these checks
> are redundant with some other ones, but why should we remove these and
> not the other ones instead?

That's a good question, and I have an answer. Most of the ALTER TABLE
operations use ATSimplePermissions() or
ATSimplePermissionsRelationOrIndex() to check permissions. The
exceptions are AT_SetDistinct (and I'm wondering if we shouldn't
remove that exception, see my recent message on attoptions) and
AT_ChangeOwner (see comments for why). ATSimplePermissions() checks
(1) that we are operating on the right sort of relkind, (2) that the
current user owns the object, and (3) that we are operating on a
system catalog only if allowSystemTableMods. When we are enabling or
disabling a rule (but not in other cases), we then recheck only the
second of those conditions. Removing the "other check" (the call to
ATSimplePermissions) would require copying the other two bits of logic
into EnableDisableRule - so we'd have duplicate code floating around,
and the EnableDisableRule operations would have bespoke code instead
of sharing the common mechanism used by the remaining ALTER TABLE
commands.

...Robert