Thread

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

   Robert Haas <robertmhaas@gmail.com> — 2010-01-22T16:27:56Z

   On Thu, Dec 17, 2009 at 10:09 PM, Stephen Frost <sfrost@snowman.net> wrote:
   > * Tom Lane (tgl@sss.pgh.pa.us) wrote:
   >> KaiGai Kohei <kaigai@ak.jp.nec.com> writes:
   >> > [ patch to remove EnableDisableRule's permissions check ]
   >>
   >> I don't particularly like this patch, mainly because I disagree with
   >> randomly removing permissions checks without any sort of plan about
   >> where they ought to go.
   >
   > The goal of this was to increase consistancy with the rest of the code,
   > in particular, ATPrepCmd checks ownership rights on the table, and
   > anything which wants to check permissions beyond that has to do it
   > independently.  Do I like that?  No, not really.
   
   After you posted this, I agreed with you a time or two that this was
   kooky, but I spent some time yesterday and today reading through
   tablecmds.c, and I've somewhat revised my opinion.  It seems to me
   that the ALTER TABLE permissions checks can be basically divided into
   two parts: (A) checks on the relation that is being altered, and (B)
   checks on other objects that are involved in whatever operation is
   being performed.
   
   Right now, the part-A logic is partially centralized in ATPrepCmd()
   and partially spread throughout the rest of the code, and the part-B
   logic is all over the place.  If we want ALL the permissions checking
   for any given ALTER TABLE command to happen in ONE place, we're going
   to have to do one of two things, neither of which is currently making
   sense to me.  One, we could move all of the checks that are now done
   in ATPrepCmd() down into the constituent ATPrep* functions and do all
   the checks at once at the point when we have enough information.  But
   that almost seems like it's going backwards - we're spreading out
   checks that are now (kind of) centralized.  And, as Tom has pointed
   out, it means doing more work before we decide whether we even had
   permission to take the relation lock.  Two, we could try to pull all
   the permission checks that are NOT in ATPrepCmd() back in.  But that
   hardly seems feasible - we don't have enough information at that
   point.
   
   So, what if we treat these two cases separately?  The part-B checks -
   on the other operations involved in ALTER TABLE - are by definition
   idiosyncratic.  What type of object we're checking and what permission
   we're checking for is inextricably bound up with what kind of ALTER
   TABLE operation we're trying to perform.  So, that's going to be hard
   to centralize.  But the part-A checks - on the relation itself - seem
   like they could be consolidated.  The reason why they are spread out
   right now is mostly because of relatively minor deviations from what
   ATSimplePermissions does.
   
   A1. ATSimplePermissionsRelationOrIndex(), which a few of the ALTER
   TABLE subcommands use, is exactly the same as ATSimplePermissions(),
   except that it allows indices as well.
   A2. ATSetStatistics() and ATSetDistinct() are also similar, but they
   both allow indices and also skip the defenses against system table
   modification.
   A3. ATExecChangeOwner() emits a warning indices and then performs no
   action (for backwards compatibility), rather than emitting an error as
   ATSimplePermissions() would do.  It also doesn't check permission if
   the old and new owner are the same.
   A4. ATExecEnableDisableTrigger() and ATExecEnableDisableRule() call,
   respectively, EnableDisableTrigger and EnableDisableRule, each of
   which does a redundant permissions check.
   
   I believe that everything else just calls ATSimplePermissions(),
   though it's possible I've missed something.  It strikes me that if we
   changed the signature for ATSimplePermissions, we could eliminate
   A1-A3 (and A4 is trivial):
   
   static void ATSimplePermissions(Relation rel, AlterTableCmd *cmd);
   
   The plan would be to move the knowledge of which operations require
   special treatment (allowing indices, system tables, etc.) into
   ATSimplePermissions() and then just calling it unconditionally for ALL
   object types.  ATSimplePermissionsRelationOrIndex() would go away.
   ATExecChangeOwner() would require some refactoring, but I think it
   would end up being simpler than it is now.  I also think it would be
   more clear which checks are being applied to which object types.
   
   Just to enumerate the part-B permissions checks, that is, permissions
   checks on objects other than the table to which ALTER TABLE is being
   directly applied, the ones I found were:
   
   B1. ATAddForeignKeyConstrants() checks for REFERENCES permission on
   the two tables involved.
   B2. ATExecDropColumn() and ATExecDropConstraint() check for permission
   to perform the drop on the child tables to which they decide to
   recurse.
   B3. ATExecAddInherit() checks permissions on the new parent.
   B4. ATPrepSetTablespace() checks for permission to use the new tablespace.
   B5. ATExecAddIndex() calls DefineIndex(), which also checks for rights
   on the new namespace.
   
   B2 and B3 are actually implemented at present using
   ATSimplePermissions, and I think we could keep it that way under the
   proposed signature change, with only minor refactoring.  The others
   are basically all special cases, but there aren't actually that many
   of them.
   
   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.
   
   We also have a few bits and pieces of ALTER TABLE that do not go
   through ATPrepCmd() at all and therefore need to be considered
   separately from the above.  It looks like ALTER TABLE ... RENAME and
   ALTER TABLE ... RENAME COLUMN go through ExecRenameStmt().
   ExecRenameStmt() generally leaves permissions-checking to the
   RenameWhatever() functions, but for RenameRelation(), renameatt(), and
   renametrig() it makes an exception and does some of the work here.  I
   think we should push that logic back down into the constituent
   functions so that this goes back to being just a dispatch function;
   while we're at it, I think we should change renameatt() and
   renametrig() to use the same naming convention as the rest of these
   functions.  It's not really buying us anything to do the check here;
   it's just making it harder to find all the checks - for example, it
   looks to me like the check done here is redundant with one being
   performed in renameatt() anyway.  Interestingly, renameatt() also has
   a comment that says "this would normally be done in utility.c, but
   this particular routine is recursive".  That comment doesn't reflect
   the way things are actually laid out, though.
   
   ALTER TABLE ... SET SCHEMA has the same issue:
   ExecAlterObjectSchemaStmt normally just calls
   AlterWhateverNamespace(), but in the case where Whatever is Table, it
   first calls CheckRelationOwnership().  So I think we should go ahead
   and push this check down too, to match all the others.
   
   Why do this?  Well, I think the end game is that it would be nice to
   provide a finite number of well-defined control points that need to be
   modified to add or change permission checks.  Of course, a lot more
   work will be needed to do that in its entirety - this email only
   addresses the ALTER TABLE stuff, and I'm not even 100% positive that
   I've found everything.  But I think that's part of the problem too -
   as we clean this up, it will become feasible to actually list what the
   control points for different permissions checks are.
   
   Thoughts?  Comments?  Blistering flames of agonizing death?
   
   ...Robert
   
   

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

   Tom Lane <tgl@sss.pgh.pa.us> — 2010-01-22T16:54:42Z

   Robert Haas <robertmhaas@gmail.com> writes:
   >> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
   >>>> I don't particularly like this patch, mainly because I disagree with
   >>>> randomly removing permissions checks without any sort of plan about
   >>>> where they ought to go.
   
   > [ a plan for rearranging ALTER TABLE's checks ]
   
   Works for me.  All I asked for was plan first, code second, and you've
   satisfied the precondition.
   
   (I would venture that changing this stuff is probably 9.1 material at
   this point, though.)
   
   			regards, tom lane
   
   

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

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-01-23T07:17:19Z

   (2010/01/23 1:27), Robert Haas wrote:
   > So, what if we treat these two cases separately?  The part-B checks -
   > on the other operations involved in ALTER TABLE - are by definition
   > idiosyncratic.  What type of object we're checking and what permission
   > we're checking for is inextricably bound up with what kind of ALTER
   > TABLE operation we're trying to perform.  So, that's going to be hard
   > to centralize.  But the part-A checks - on the relation itself - seem
   > like they could be consolidated.  The reason why they are spread out
   > right now is mostly because of relatively minor deviations from what
   > ATSimplePermissions does.
   
   I agree with the top half of this proposition. ALTER TABLE has most
   various kind of options in PostgreSQL, so some of options are not
   feasible to complete all the needed checks in ATPrepCmd() stage,
   such as AT_AddIndex.
   
   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?
   
   > A1. ATSimplePermissionsRelationOrIndex(), which a few of the ALTER
   > TABLE subcommands use, is exactly the same as ATSimplePermissions(),
   > except that it allows indices as well.
   > A2. ATSetStatistics() and ATSetDistinct() are also similar, but they
   > both allow indices and also skip the defenses against system table
   > modification.
   > A3. ATExecChangeOwner() emits a warning indices and then performs no
   > action (for backwards compatibility), rather than emitting an error as
   > ATSimplePermissions() would do.  It also doesn't check permission if
   > the old and new owner are the same.
   > A4. ATExecEnableDisableTrigger() and ATExecEnableDisableRule() call,
   > respectively, EnableDisableTrigger and EnableDisableRule, each of
   > which does a redundant permissions check.
   >
   > I believe that everything else just calls ATSimplePermissions(),
   > though it's possible I've missed something.  It strikes me that if we
   > changed the signature for ATSimplePermissions, we could eliminate
   > A1-A3 (and A4 is trivial):
   >
   > static void ATSimplePermissions(Relation rel, AlterTableCmd *cmd);
   >
   > The plan would be to move the knowledge of which operations require
   > special treatment (allowing indices, system tables, etc.) into
   > ATSimplePermissions() and then just calling it unconditionally for ALL
   > object types.  ATSimplePermissionsRelationOrIndex() would go away.
   > ATExecChangeOwner() would require some refactoring, but I think it
   > would end up being simpler than it is now.  I also think it would be
   > more clear which checks are being applied to which object types.
   
   I have a different plan to clean up these differences, especially A1 and A2.
   
   The existing ATSimplePermissions() can be also divided into three parts,
   as source code comments mentioned.
     Aa) It ensures the relation has an expected relkind
     Ab) It ensures ownership of the relation to be altered
     Ac) It ensures the relation is not system catalog, if not allowSystemTableMods.
   
   If we provide these three checks in separated helper function, like:
     Aa) ATCheckRelkind(Relation rel, bool viewOK, bool indexOK)
     Ab) ATCheckOwnership(Relation rel)
     Ac) ATCheckNoCatalog(Relation rel)
   
   The above A1 and A2 can be rewritten using combination of them, then we can
   eliminate these functions to apply special treatments.
   For example, the A2 can be replaced by ATCheckRelkind(rel, false, true) and
   ATCheckOwnership(rel).
   
   I think it allows to put a logic to decide whether we should apply permission
   checks at the ATPrepCmd() stage, or not, in the existing swich ... case branch.
   
   For some of exceptions, we can apply checks for them in the later stage
   after the code gathered enough information to make access control decision.
   
   > Just to enumerate the part-B permissions checks, that is, permissions
   > checks on objects other than the table to which ALTER TABLE is being
   > directly applied, the ones I found were:
   >
   > B1. ATAddForeignKeyConstrants() checks for REFERENCES permission on
   > the two tables involved.
   > B2. ATExecDropColumn() and ATExecDropConstraint() check for permission
   > to perform the drop on the child tables to which they decide to
   > recurse.
   > B3. ATExecAddInherit() checks permissions on the new parent.
   > B4. ATPrepSetTablespace() checks for permission to use the new tablespace.
   > B5. ATExecAddIndex() calls DefineIndex(), which also checks for rights
   > on the new namespace.
   >
   > B2 and B3 are actually implemented at present using
   > ATSimplePermissions, and I think we could keep it that way under the
   > proposed signature change, with only minor refactoring.  The others
   > are basically all special cases, but there aren't actually that many
   > of them.
   >
   > 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.
   
   > We also have a few bits and pieces of ALTER TABLE that do not go
   > through ATPrepCmd() at all and therefore need to be considered
   > separately from the above.  It looks like ALTER TABLE ... RENAME and
   > ALTER TABLE ... RENAME COLUMN go through ExecRenameStmt().
   > ExecRenameStmt() generally leaves permissions-checking to the
   > RenameWhatever() functions, but for RenameRelation(), renameatt(), and
   > renametrig() it makes an exception and does some of the work here.  I
   > think we should push that logic back down into the constituent
   > functions so that this goes back to being just a dispatch function;
   > while we're at it, I think we should change renameatt() and
   > renametrig() to use the same naming convention as the rest of these
   > functions.  It's not really buying us anything to do the check here;
   > it's just making it harder to find all the checks - for example, it
   > looks to me like the check done here is redundant with one being
   > performed in renameatt() anyway.  Interestingly, renameatt() also has
   > a comment that says "this would normally be done in utility.c, but
   > this particular routine is recursive".  That comment doesn't reflect
   > the way things are actually laid out, though.
   
   They calls CheckRelationOwnership() just before RenameRelation(),
   renameatt() and renametrig(). It applies same checks except for sanity
   checks in relkind.
   But here is no reason why we cannot rewrite them using ATCheckOwnership()
   and ATCheckNoCatalog() in tablecmd.c, if ATSimplePermissions() are divided.
   
   > ALTER TABLE ... SET SCHEMA has the same issue:
   > ExecAlterObjectSchemaStmt normally just calls
   > AlterWhateverNamespace(), but in the case where Whatever is Table, it
   > first calls CheckRelationOwnership().  So I think we should go ahead
   > and push this check down too, to match all the others.
   
   I agree.
   
   > Why do this?  Well, I think the end game is that it would be nice to
   > provide a finite number of well-defined control points that need to be
   > modified to add or change permission checks.  Of course, a lot more
   > work will be needed to do that in its entirety - this email only
   > addresses the ALTER TABLE stuff, and I'm not even 100% positive that
   > I've found everything.  But I think that's part of the problem too -
   > as we clean this up, it will become feasible to actually list what the
   > control points for different permissions checks are.
   
   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.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

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

   Robert Haas <robertmhaas@gmail.com> — 2010-01-24T00:08:32Z

   On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei <kaigai@kaigai.gr.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
   
   

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

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-01-24T01:33:40Z

   (2010/01/24 9:08), Robert Haas wrote:
   > On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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.
   
   Hmm. Indeed, the existing ATPrepCmd() closely combines the permission
   checks and controls of code path (inheritance recursion and AT_PASS_*),
   and it is worthwhile to divide these independent logic into two.
   
   In your plan, where the new ATSimplePermissions() should be called?
   
   If we still call it from the ATPrepCmd() stage, it needs to apply
   special treatments some of operations with part-B logic.
   
   One other candidate of the entrypoint is the head of each ATExecXXXX()
   functions. In this case, we have already gathered enough information
   in B2(ATExecDropColumn, ATExecDropConstraint), B3(ATExecAddInherit)
   and B4(ATPrepSetTablespace). In B1(ATAddForeignKeyConstrants), we
   will need a bit more steps to collect information, but we can call
   it just after all the needed information.
   We can also have enough information at the head of B5(ATExecAddIndex).
   However, my preference is to apply checks at the DefineIndex() when
   "check_rights" equals true, because it also allows to consolidate
   permission checks in ProcessUtility() with T_IndexStmt.
   
   >>> 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.
   
   I'd like to introduce it using a pseudo code.
   
   Currently, in ATPrepCmd(),
   
   |    case AT_AddConstraint:  /* ADD CONSTRAINT */
   |        ATSimplePermissions(rel, false);
   |        /* Recursion occurs during execution phase */
   |        /* No command-specific prep needed except saving recurse flag */
   |        if (recurse)
   |            cmd->subtype = AT_AddConstraintRecurse;
   |        pass = AT_PASS_ADD_CONSTR;
   |        break;
   
   What I said is:
   
   |    case AT_AddConstraint:  /* ADD CONSTRAINT */
   |    {
   |        Constraint   *newCons = (Constraint *)cmd->def;
   |        if (newCons->contype == CONSTR_CHECK)
   |        {
   |            ATSimplePermissions(rel, false);
   |            if (recurse)
   |                ATSimpleRecursion(wqueue, rel, cmd, recurse);
   |            cmd->subtype = AT_AddCheckContraint;
   |        }
   |        else if (newCond->contype == CONSTR_FOREIGN)
   |        {
   |            /* Permission checks are applied during execution stage */
   |            /* And, it never recurse */
   |            cmd->subtype = AT_AddFKConstraint;
   |        }
   |        else
   |            elog(ERROR, "unrecognized constraint type");
   |
   |        pass = AT_PASS_ADD_CONSTR;
   |        break;
   |    }
   
   It will allow to eliminate self recursion in ATAddCheckConstraint() and
   to apply permission checks for new CHECK constraint in ATPrepCmd() phase.
   
   Perhaps, it may be consolidated to ATPrepAddConstraint().
   
   >> 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.
   
   I agree.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

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

   Robert Haas <robertmhaas@gmail.com> — 2010-01-24T02:27:30Z

   On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
   > (2010/01/24 9:08), Robert Haas wrote:
   >>
   >> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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.
   >
   > Hmm. Indeed, the existing ATPrepCmd() closely combines the permission
   > checks and controls of code path (inheritance recursion and AT_PASS_*),
   > and it is worthwhile to divide these independent logic into two.
   
   Yeah, that's what I thought, too.
   
   > In your plan, where the new ATSimplePermissions() should be called?
   
   From ATPrepCmd(), just before the big switch statement.
   
   > If we still call it from the ATPrepCmd() stage, it needs to apply
   > special treatments some of operations with part-B logic.
   
   Not sure if I understand what you mean.  ATSimplePermissions() won't
   be responsible for applying permissions checks related to other
   objects upon which the command is operating (e.g. the other table, if
   adding a foreign key).  It will however be responsible for knowing
   everything about which permission checks to apply to the main table
   involved, which will require some special-case logic for certain
   command types.
   
   > One other candidate of the entrypoint is the head of each ATExecXXXX()
   > functions. [...snip...]
   
   I don't think this is a good idea.  Calling it in just one place seems
   less error-prone and easier to audit.
   
   >>>> 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.
   >
   > I'd like to introduce it using a pseudo code.
   >
   > Currently, in ATPrepCmd(),
   >
   > |    case AT_AddConstraint:  /* ADD CONSTRAINT */
   > |        ATSimplePermissions(rel, false);
   > |        /* Recursion occurs during execution phase */
   > |        /* No command-specific prep needed except saving recurse flag */
   > |        if (recurse)
   > |            cmd->subtype = AT_AddConstraintRecurse;
   > |        pass = AT_PASS_ADD_CONSTR;
   > |        break;
   >
   > What I said is:
   >
   > |    case AT_AddConstraint:  /* ADD CONSTRAINT */
   > |    {
   > |        Constraint   *newCons = (Constraint *)cmd->def;
   > |        if (newCons->contype == CONSTR_CHECK)
   > |        {
   > |            ATSimplePermissions(rel, false);
   > |            if (recurse)
   > |                ATSimpleRecursion(wqueue, rel, cmd, recurse);
   > |            cmd->subtype = AT_AddCheckContraint;
   > |        }
   > |        else if (newCond->contype == CONSTR_FOREIGN)
   > |        {
   > |            /* Permission checks are applied during execution stage */
   > |            /* And, it never recurse */
   > |            cmd->subtype = AT_AddFKConstraint;
   > |        }
   > |        else
   > |            elog(ERROR, "unrecognized constraint type");
   > |
   > |        pass = AT_PASS_ADD_CONSTR;
   > |        break;
   > |    }
   >
   > It will allow to eliminate self recursion in ATAddCheckConstraint() and
   > to apply permission checks for new CHECK constraint in ATPrepCmd() phase.
   >
   > Perhaps, it may be consolidated to ATPrepAddConstraint().
   
   I don't really see that this gains us anything.
   
   ...Robert
   
   

7. Re: restructuring "alter table" privilege checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-01-24T03:11:34Z

   (2010/01/24 11:27), Robert Haas wrote:
   > On Sat, Jan 23, 2010 at 8:33 PM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
   >> (2010/01/24 9:08), Robert Haas wrote:
   >>>
   >>> On Sat, Jan 23, 2010 at 2:17 AM, KaiGai Kohei<kaigai@kaigai.gr.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.
   >>
   >> Hmm. Indeed, the existing ATPrepCmd() closely combines the permission
   >> checks and controls of code path (inheritance recursion and AT_PASS_*),
   >> and it is worthwhile to divide these independent logic into two.
   >
   > Yeah, that's what I thought, too.
   >
   >> In your plan, where the new ATSimplePermissions() should be called?
   >
   >> From ATPrepCmd(), just before the big switch statement.
   >
   >> If we still call it from the ATPrepCmd() stage, it needs to apply
   >> special treatments some of operations with part-B logic.
   >
   > Not sure if I understand what you mean.  ATSimplePermissions() won't
   > be responsible for applying permissions checks related to other
   > objects upon which the command is operating (e.g. the other table, if
   > adding a foreign key).  It will however be responsible for knowing
   > everything about which permission checks to apply to the main table
   > involved, which will require some special-case logic for certain
   > command types.
   >
   >> One other candidate of the entrypoint is the head of each ATExecXXXX()
   >> functions. [...snip...]
   >
   > I don't think this is a good idea.  Calling it in just one place seems
   > less error-prone and easier to audit.
   
   Yes, most of the ALTER TABLE options runs ATPrepCmd() except for RENAME
   TO and SET SCHEMA, so it is a good candidate to apply less error-prone
   permission checks.
   
   The reason why I introduced this alternative idea is from the perspective
   of simple basis/concept about where we should apply permission checks,
   although it needs larger number of entrypoints compared to head of the
   ATPrepCmd().
   
   If we put the new ATSimplePermissions() with all the needed information
   just after gathering them at the execution stage, we don't need to have
   some of exceptions which takes additional checks except for ownership
   on the relation to be altered.
   
   Of course, code simpleness is important. Likewise, I think simpleness in
   basis/concept (less number of special treatments) is also important.
   
   
   >> I'd like to introduce it using a pseudo code.
      :
   >> It will allow to eliminate self recursion in ATAddCheckConstraint() and
   >> to apply permission checks for new CHECK constraint in ATPrepCmd() phase.
   >>
   >> Perhaps, it may be consolidated to ATPrepAddConstraint().
   >
   > I don't really see that this gains us anything.
   
   Hmm, indeed, here is no more benefit except for eliminating one self recursion.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

8. Re: restructuring "alter table" privilege checks

   Robert Haas <robertmhaas@gmail.com> — 2010-01-24T03:16:26Z

   On Sat, Jan 23, 2010 at 10:11 PM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
   > If we put the new ATSimplePermissions() with all the needed information
   > just after gathering them at the execution stage, we don't need to have
   > some of exceptions which takes additional checks except for ownership
   > on the relation to be altered.
   
   Maybe I'm still not understanding, but I don't see how you're going to
   do this without a massive pile of spaghetti code and a function with
   about 12 parameters.  Feel free to show some code, but I think this is
   a non-starter.
   
   ...Robert
   
   

9. Re: restructuring "alter table" privilege checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-01-24T03:40:37Z

   (2010/01/24 12:16), Robert Haas wrote:
   > On Sat, Jan 23, 2010 at 10:11 PM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
   >> If we put the new ATSimplePermissions() with all the needed information
   >> just after gathering them at the execution stage, we don't need to have
   >> some of exceptions which takes additional checks except for ownership
   >> on the relation to be altered.
   >
   > Maybe I'm still not understanding, but I don't see how you're going to
   > do this without a massive pile of spaghetti code and a function with
   > about 12 parameters.  Feel free to show some code, but I think this is
   > a non-starter.
   
   Hhmmm,...
   
   Indeed, indeed, if a single function tries to handle all the ALTER TABLE
   options, it needs many function arguments, because ALTER TABLE is one of
   the most functional statement in PostgreSQL...
   
   If the basis is head of the execution phase, it does not need a big switch
   ... case branch in ATSimplePermissions, because it is already branched in
   ATExecCmd(). However, it also has tradeoff that we have multiple minor version
   of functions to check ALTER TABLE permissions.
   
   Perhaps, it may be a good idea to make two conceptual patches both head of
   the ATPrepCmd() and ATExec*(). They will make clear good/bad points between
   two approaches.
   
   Is it waste of efforts?
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

10. Re: restructuring "alter table" privilege checks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-01-24T09:32:30Z

    (2010/01/24 12:40), KaiGai Kohei wrote:
    > Perhaps, it may be a good idea to make two conceptual patches both head of
    > the ATPrepCmd() and ATExec*(). They will make clear good/bad points between
    > two approaches.
    
    I tried to make two conceptual patches.
    
    * pgsql-at-rework-prep.1.patch
    
    It adds ATPermCmd(Relation rel, AlterTableCmd *cmd) that is called from the
    head of ATPrepCmd(). This function enables to divide the logic of permission
    checks depending on cmd->subtype from ATPrepCmd().
    In most of subcommand (it does not check permission except for ownership of
    the relation to be altered), it calls ATSimplePermissions() or similar.
    Or, it does not anything at the stage for rest of exceptions.
    
    Good: Here is only one entrypoint to call ATPermCmd().
      Bad: Although most of logics are consolidated into ATPremCmd(), we need to
           put individual checks on some of exception cases.
    
    Was it matching with what you suggested? Or, am I missing something?
    
    * pgsql-at-rework-exec.2.patch
    
    It moves permission checks into the head (or just after all needed information
    was gathered) of ATExec*() functions. The ATPrepCmd() checks only correctness
    of relkind and ensure the relation is not system catalog.
    This basis/concept can be applied to ALTER TABLE RENAME TO/SET SCHEMA cases also.
    
    Good: Concept is clear, and less exceptions.
    Good: We can apply this concept (just before execution) on other database
           objects which does not have explicit preparation stage.
      Bad: All the ATExec*() function has to call the permission checks.
    
    My preference is the later approach. Indeed, it has larger number of entrypoints
    compared to the ATPermCom() functions, but its concept is clear and we can also
    apply same basis on the code path that does not go through ATPrepCmd().
    
    P.S In the right way, this patch should also touch CheckRelationOwnership() or
         DefineIndex() logic, but I omitted them because of simplifies.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>