Re: restructuring "alter table" privilege checks (was: remove redundant ownership checks)

On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> However, it is unclear for me whether the revised ATSimplePermissions()
> provide cleaner code than currently we have, because it also needs
> a big switch ... case statement within.
>
> Am I misunderstanding something?

Well, not everyone is going to agree on what "cleaner code" means in
every case, but the reason that I like my design better is because it
moves all of the decision making out of ATPrepCmd() into
ATSimplePermissions(). What you're proposing would mean that
ATPrepCmd() would basically continue to know everything about which
operations need which permissions checks, which I don't think is going
to scale very well to alternative security providers, if we eventually
decide to support such a thing.

>> It may also be worth refactoring is ATAddCheckConstraint(), which
>> currently does its own recursion only so that the constraint name at
>> the top of the inheritance hierarchy propagates all the way down
>> unchanged.  I wonder if it would be cleaner/possible to work out the
>> constraint name earlier and then just use the standard recursion
>> mechanism.
>
> Isn't it possible to check whether the given constraint is CHECK()
> or FOREIGN KEY in the ATPrepCmd() stage, and assign individual
> cmd->subtype? If CONSTR_FOREIGN, it will never recursion.
>
> In this case, we can apply checks in ATPrepCmd() stage for CONSTR_CHECK.

I don't understand what you're saying here.

> It is a good idea to consolidate permission check into tablecmd.c from
> various kind of core routines.
> And, it also makes the basis clear. The permission check should be applied
> after the code path gathered enough information to make its access control
> decision as early as possible.

And just as importantly, in a consistent place.

...Robert