Thread

1. [v9.2] sepgsql's DROP Permission checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-10T12:51:23Z

   The attached patch adds OAT_DROP object-access-hook around permission
   checks of object deletion.
   Due to the previous drop statement reworks, the number of places to
   put this hook is limited to these six points: RemoveObjects,
   RemoveRelations, ATExecDropColumn, dropdb, DropTableSpace and
   shdepDropOwned().
   
   In sepgsql side, it checks {drop} permission for each object types,
   and {remove_name} permission to the schema that owning the object
   being removed. I'm still considering whether the drop permission
   should be applied on objects being removed in cascade mode.
   It is not difficult to implement. We can determine the bahavior on
   object deletion with same manner of creation; that saves contextual
   information using ProcessUtility hook.
   
   At this moment, the current proposed patch does not apply checks on
   cascaded deletion, according to SQL behavior. However, my concern is
   that user can automatically have right to remove all the objects
   underlying a partidular schema being removable, even if individual
   tables or functions are not able to be removed.
   
   So, my preference is sepgsql references dependency tables to check
   {drop} permissions of involved objects, not only the target object.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

2. Re: [v9.2] sepgsql's DROP Permission checks

   Robert Haas <robertmhaas@gmail.com> — 2012-01-17T02:11:46Z

   On Tue, Jan 10, 2012 at 7:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > The attached patch adds OAT_DROP object-access-hook around permission
   > checks of object deletion.
   > Due to the previous drop statement reworks, the number of places to
   > put this hook is limited to these six points: RemoveObjects,
   > RemoveRelations, ATExecDropColumn, dropdb, DropTableSpace and
   > shdepDropOwned().
   >
   > In sepgsql side, it checks {drop} permission for each object types,
   > and {remove_name} permission to the schema that owning the object
   > being removed. I'm still considering whether the drop permission
   > should be applied on objects being removed in cascade mode.
   > It is not difficult to implement. We can determine the bahavior on
   > object deletion with same manner of creation; that saves contextual
   > information using ProcessUtility hook.
   >
   > At this moment, the current proposed patch does not apply checks on
   > cascaded deletion, according to SQL behavior. However, my concern is
   > that user can automatically have right to remove all the objects
   > underlying a partidular schema being removable, even if individual
   > tables or functions are not able to be removed.
   >
   > So, my preference is sepgsql references dependency tables to check
   > {drop} permissions of involved objects, not only the target object.
   
   Hmm.  I kind of wonder if we shouldn't just have the OAT_DROP hook get
   invoked for every actual drop, rather than only for the top-level
   object.  It's somewhat appealing to have the hook more-or-less match
   up the permissions checks, as you have it here, but in general it
   seems more useful to have a callback for each thing dropped than to
   have a callback for each thing named to be dropped.  What is your
   opinion?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

3. Re: [v9.2] sepgsql's DROP Permission checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-17T15:55:56Z

   2012/1/17 Robert Haas <robertmhaas@gmail.com>:
   > On Tue, Jan 10, 2012 at 7:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >> The attached patch adds OAT_DROP object-access-hook around permission
   >> checks of object deletion.
   >> Due to the previous drop statement reworks, the number of places to
   >> put this hook is limited to these six points: RemoveObjects,
   >> RemoveRelations, ATExecDropColumn, dropdb, DropTableSpace and
   >> shdepDropOwned().
   >>
   >> In sepgsql side, it checks {drop} permission for each object types,
   >> and {remove_name} permission to the schema that owning the object
   >> being removed. I'm still considering whether the drop permission
   >> should be applied on objects being removed in cascade mode.
   >> It is not difficult to implement. We can determine the bahavior on
   >> object deletion with same manner of creation; that saves contextual
   >> information using ProcessUtility hook.
   >>
   >> At this moment, the current proposed patch does not apply checks on
   >> cascaded deletion, according to SQL behavior. However, my concern is
   >> that user can automatically have right to remove all the objects
   >> underlying a partidular schema being removable, even if individual
   >> tables or functions are not able to be removed.
   >>
   >> So, my preference is sepgsql references dependency tables to check
   >> {drop} permissions of involved objects, not only the target object.
   >
   > Hmm.  I kind of wonder if we shouldn't just have the OAT_DROP hook get
   > invoked for every actual drop, rather than only for the top-level
   > object.  It's somewhat appealing to have the hook more-or-less match
   > up the permissions checks, as you have it here, but in general it
   > seems more useful to have a callback for each thing dropped than to
   > have a callback for each thing named to be dropped.  What is your
   > opinion?
   >
   I think it is more ideal design.
   
   If we could track object deletion only top-level, we have no option to
   implement security features based on the object-access-hook, even
   if security model is suitable to apply checks all the object deletion.
   In addition, I believe it should be used to clean-up something set up
   by external modules, not only permission checks.
   
   Do I modify the patch to place object-access-hook on deleteOneObject
   (probably, it is the best position to track actual deletion)?
   One problem is case of deletion of columns by ALTER TABLE.
   It just marks "attisdropped" flag; without removing catalog entry.
   Do we ought to put this hook on ATExecDropColumn exceptionally?
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

4. Re: [v9.2] sepgsql's DROP Permission checks

   Robert Haas <robertmhaas@gmail.com> — 2012-01-18T02:53:02Z

   On Tue, Jan 17, 2012 at 10:55 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > Do I modify the patch to place object-access-hook on deleteOneObject
   > (probably, it is the best position to track actual deletion)?
   > One problem is case of deletion of columns by ALTER TABLE.
   > It just marks "attisdropped" flag; without removing catalog entry.
   > Do we ought to put this hook on ATExecDropColumn exceptionally?
   
   +1.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

5. Re: [v9.2] sepgsql's DROP Permission checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-18T14:50:07Z

   2012/1/18 Robert Haas <robertmhaas@gmail.com>:
   > On Tue, Jan 17, 2012 at 10:55 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >> Do I modify the patch to place object-access-hook on deleteOneObject
   >> (probably, it is the best position to track actual deletion)?
   >> One problem is case of deletion of columns by ALTER TABLE.
   >> It just marks "attisdropped" flag; without removing catalog entry.
   >> Do we ought to put this hook on ATExecDropColumn exceptionally?
   >
   > +1.
   >
   The attached one is a revised version.
   It adds OAT_DROP on the following points:
   * deleteOneObject()
   * dropdb()
   * DropTableSpace()
   * DropRole()
   
   One thing I overlooked on the upthread is that ATExecDropColumn()
   also calls performDeletion, then RemoveAttributeById() set attisdropped.
   So, this function was not a point to be hooked exceptionally.
   
   On the other hand, as a source code comment on doDeletion() says,
   deletion of shared database objects (database, tablespace and role)
   are not hooked on deleteOneObject(), thus, I added a hook for each
   deletion code of these objects.
   
   In sepgsql side, it determines a case to apply permission checks
   according to the contextual information; that is same technique
   when we implemented create permission.
   Thus, it could checks db_xxx:{drop} permission correctly.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   

6. Re: [v9.2] sepgsql's DROP Permission checks

   Robert Haas <robertmhaas@gmail.com> — 2012-01-19T01:34:13Z

   On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > In sepgsql side, it determines a case to apply permission checks
   > according to the contextual information; that is same technique
   > when we implemented create permission.
   > Thus, it could checks db_xxx:{drop} permission correctly.
   
   Why do we need the contextual information in this case?  Why
   can't/shouldn't the decision be made solely on the basis of what
   object is targeted?
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

7. Re: [v9.2] sepgsql's DROP Permission checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-19T08:51:50Z

   2012/1/19 Robert Haas <robertmhaas@gmail.com>:
   > On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >> In sepgsql side, it determines a case to apply permission checks
   >> according to the contextual information; that is same technique
   >> when we implemented create permission.
   >> Thus, it could checks db_xxx:{drop} permission correctly.
   >
   > Why do we need the contextual information in this case?  Why
   > can't/shouldn't the decision be made solely on the basis of what
   > object is targeted?
   >
   Several code-paths to remove a particular objects are not appropriate
   to apply permission checks. For example...
   
   [1] Cleanup of temporary objects
   on_shmem_eixt() registers RemoveTempRelationsCallback(), then
   it eventually calls deleteWhatDependsOn() that have an invocation
   of deleteOneObject().
   This code path is just an internal cleanup process, not related to
   permission of the client.
   
   [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
   rebuild_relation() creates a temporary table with make_new_heap(),
   then it copies the contents of original table according to the order of
   index, and calls finish_heap_swap() that swaps relation files and
   removes the temporary table using performDeletion().
   This code actually create and drop a table, however, it is quite
   internal design and not related to permission of the client.
   
   So, I think sepgsql should only applied to permission checks
   object deletion invoked by user's operations, such as DropStmt.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

8. Re: [v9.2] sepgsql's DROP Permission checks

   Robert Haas <robertmhaas@gmail.com> — 2012-01-19T16:06:37Z

   On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   > 2012/1/19 Robert Haas <robertmhaas@gmail.com>:
   >> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >>> In sepgsql side, it determines a case to apply permission checks
   >>> according to the contextual information; that is same technique
   >>> when we implemented create permission.
   >>> Thus, it could checks db_xxx:{drop} permission correctly.
   >>
   >> Why do we need the contextual information in this case?  Why
   >> can't/shouldn't the decision be made solely on the basis of what
   >> object is targeted?
   >>
   > Several code-paths to remove a particular objects are not appropriate
   > to apply permission checks. For example...
   >
   > [1] Cleanup of temporary objects
   > on_shmem_eixt() registers RemoveTempRelationsCallback(), then
   > it eventually calls deleteWhatDependsOn() that have an invocation
   > of deleteOneObject().
   > This code path is just an internal cleanup process, not related to
   > permission of the client.
   >
   > [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
   > rebuild_relation() creates a temporary table with make_new_heap(),
   > then it copies the contents of original table according to the order of
   > index, and calls finish_heap_swap() that swaps relation files and
   > removes the temporary table using performDeletion().
   > This code actually create and drop a table, however, it is quite
   > internal design and not related to permission of the client.
   >
   > So, I think sepgsql should only applied to permission checks
   > object deletion invoked by user's operations, such as DropStmt.
   
   I agree with that theory, but isn't this method of implementing that a
   pretty horrible kludge?  For example, if you'd implemented it this way
   for 9.1, the recent drop-statement refactoring would have broken it.
   Or if, in the future, we add another type of statement that can drop
   things, this code will still compile just fine but will no longer work
   correctly.  ISTM that we need a way to either (1) not call the hook at
   all unless the operation is user-initiated, or (2) call the hook, but
   pass a flag indicating what sort of operation this is?
   
   Let's imagine another possible use of this hook: we want to emit some
   kind of log message every time a database object gets dropped.  I
   think that's a plausible use-case, and in that case what we'd want is:
   (1) VACUUM FULL or CLUSTER shouldn't call the hook at all, (2) cleanup
   of temporary objects should probably call the hook, but ideally with a
   flag to indicate that it's an internal (DB-initiated) operation, and
   (3) user activity should definitely call the hook.
   
   I'm not sure how we can cleanly get that behavior, but ISTM that's
   what we want...
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

9. Re: [v9.2] sepgsql's DROP Permission checks

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-21T09:28:33Z

   2012/1/19 Robert Haas <robertmhaas@gmail.com>:
   > On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >> 2012/1/19 Robert Haas <robertmhaas@gmail.com>:
   >>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
   >>>> In sepgsql side, it determines a case to apply permission checks
   >>>> according to the contextual information; that is same technique
   >>>> when we implemented create permission.
   >>>> Thus, it could checks db_xxx:{drop} permission correctly.
   >>>
   >>> Why do we need the contextual information in this case?  Why
   >>> can't/shouldn't the decision be made solely on the basis of what
   >>> object is targeted?
   >>>
   >> Several code-paths to remove a particular objects are not appropriate
   >> to apply permission checks. For example...
   >>
   >> [1] Cleanup of temporary objects
   >> on_shmem_eixt() registers RemoveTempRelationsCallback(), then
   >> it eventually calls deleteWhatDependsOn() that have an invocation
   >> of deleteOneObject().
   >> This code path is just an internal cleanup process, not related to
   >> permission of the client.
   >>
   >> [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
   >> rebuild_relation() creates a temporary table with make_new_heap(),
   >> then it copies the contents of original table according to the order of
   >> index, and calls finish_heap_swap() that swaps relation files and
   >> removes the temporary table using performDeletion().
   >> This code actually create and drop a table, however, it is quite
   >> internal design and not related to permission of the client.
   >>
   >> So, I think sepgsql should only applied to permission checks
   >> object deletion invoked by user's operations, such as DropStmt.
   >
   > I agree with that theory, but isn't this method of implementing that a
   > pretty horrible kludge?  For example, if you'd implemented it this way
   > for 9.1, the recent drop-statement refactoring would have broken it.
   > Or if, in the future, we add another type of statement that can drop
   > things, this code will still compile just fine but will no longer work
   > correctly.  ISTM that we need a way to either (1) not call the hook at
   > all unless the operation is user-initiated, or (2) call the hook, but
   > pass a flag indicating what sort of operation this is?
   >
   Yes. This approach requires to continue code revising on sepgsql-side
   also for each major release.
   I think the approach (1) raise an issue similar to what we discussed
   when sepgsql implemented create permission; we have to know
   details of extension module to determine whether the hook should
   be called, or not. My preference is (2) that is more reasonable.
   
   > Let's imagine another possible use of this hook: we want to emit some
   > kind of log message every time a database object gets dropped.  I
   > think that's a plausible use-case, and in that case what we'd want is:
   > (1) VACUUM FULL or CLUSTER shouldn't call the hook at all, (2) cleanup
   > of temporary objects should probably call the hook, but ideally with a
   > flag to indicate that it's an internal (DB-initiated) operation, and
   > (3) user activity should definitely call the hook.
   >
   > I'm not sure how we can cleanly get that behavior, but ISTM that's
   > what we want...
   >
   I think the case (1) should also call the hook but with a flag that indicate
   database internal stuff, because make_new_heap() calls OAT_POST_CREATE
   hook, thus, we need to give extensions a chance to cleanup, if it did
   something on this timing.
   
   I'd like to propose to utilize DropBehavior argument of performDeletion()
   to inform dependency.c its invoked context.
   If we have a new flag DROP_INTERNAL that can be OR-masked with
   existing DROP_RESTRICT or DROP_CASCADE, it can be used to
   inform the current context, then, it shall be used to the flag to the hook.
   It seems to me this approach is minimum invasive.
   
   In addition, we may have a case when extension want to know whether
   the deletion is cascaded, or explicitly specified by users. If they want to
   implement same security model on this hook.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

10. Re: [v9.2] sepgsql's DROP Permission checks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-22T14:54:08Z

    I tried to implement based on the idea to call object-access-hook with
    flag; that
    informs extensions contexts of objects being removed.
    Because I missed DROP_RESTRICT and DROP_CASCADE are enum type,
    this patch added performInternalDeletion() instead of OR-masked DROP_INTERNAL.
    All its difference from performDeletion() is a flag (OAT_DROP_FLAGS_INTERNAL)
    shall be delivered to extension module. I replaced several performDeletion() by
    performInternalDeletion() that clean-up objects due to internal stuff.
    
    How about this approach?
    
    Thanks,
    
    2012/1/21 Kohei KaiGai <kaigai@kaigai.gr.jp>:
    > 2012/1/19 Robert Haas <robertmhaas@gmail.com>:
    >> On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> 2012/1/19 Robert Haas <robertmhaas@gmail.com>:
    >>>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>>>> In sepgsql side, it determines a case to apply permission checks
    >>>>> according to the contextual information; that is same technique
    >>>>> when we implemented create permission.
    >>>>> Thus, it could checks db_xxx:{drop} permission correctly.
    >>>>
    >>>> Why do we need the contextual information in this case?  Why
    >>>> can't/shouldn't the decision be made solely on the basis of what
    >>>> object is targeted?
    >>>>
    >>> Several code-paths to remove a particular objects are not appropriate
    >>> to apply permission checks. For example...
    >>>
    >>> [1] Cleanup of temporary objects
    >>> on_shmem_eixt() registers RemoveTempRelationsCallback(), then
    >>> it eventually calls deleteWhatDependsOn() that have an invocation
    >>> of deleteOneObject().
    >>> This code path is just an internal cleanup process, not related to
    >>> permission of the client.
    >>>
    >>> [2] Cleanup of transient table at VACUUM FULL/CLUSTER command
    >>> rebuild_relation() creates a temporary table with make_new_heap(),
    >>> then it copies the contents of original table according to the order of
    >>> index, and calls finish_heap_swap() that swaps relation files and
    >>> removes the temporary table using performDeletion().
    >>> This code actually create and drop a table, however, it is quite
    >>> internal design and not related to permission of the client.
    >>>
    >>> So, I think sepgsql should only applied to permission checks
    >>> object deletion invoked by user's operations, such as DropStmt.
    >>
    >> I agree with that theory, but isn't this method of implementing that a
    >> pretty horrible kludge?  For example, if you'd implemented it this way
    >> for 9.1, the recent drop-statement refactoring would have broken it.
    >> Or if, in the future, we add another type of statement that can drop
    >> things, this code will still compile just fine but will no longer work
    >> correctly.  ISTM that we need a way to either (1) not call the hook at
    >> all unless the operation is user-initiated, or (2) call the hook, but
    >> pass a flag indicating what sort of operation this is?
    >>
    > Yes. This approach requires to continue code revising on sepgsql-side
    > also for each major release.
    > I think the approach (1) raise an issue similar to what we discussed
    > when sepgsql implemented create permission; we have to know
    > details of extension module to determine whether the hook should
    > be called, or not. My preference is (2) that is more reasonable.
    >
    >> Let's imagine another possible use of this hook: we want to emit some
    >> kind of log message every time a database object gets dropped.  I
    >> think that's a plausible use-case, and in that case what we'd want is:
    >> (1) VACUUM FULL or CLUSTER shouldn't call the hook at all, (2) cleanup
    >> of temporary objects should probably call the hook, but ideally with a
    >> flag to indicate that it's an internal (DB-initiated) operation, and
    >> (3) user activity should definitely call the hook.
    >>
    >> I'm not sure how we can cleanly get that behavior, but ISTM that's
    >> what we want...
    >>
    > I think the case (1) should also call the hook but with a flag that indicate
    > database internal stuff, because make_new_heap() calls OAT_POST_CREATE
    > hook, thus, we need to give extensions a chance to cleanup, if it did
    > something on this timing.
    >
    > I'd like to propose to utilize DropBehavior argument of performDeletion()
    > to inform dependency.c its invoked context.
    > If we have a new flag DROP_INTERNAL that can be OR-masked with
    > existing DROP_RESTRICT or DROP_CASCADE, it can be used to
    > inform the current context, then, it shall be used to the flag to the hook.
    > It seems to me this approach is minimum invasive.
    >
    > In addition, we may have a case when extension want to know whether
    > the deletion is cascaded, or explicitly specified by users. If they want to
    > implement same security model on this hook.
    >
    > Thanks,
    > --
    > KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

11. Re: [v9.2] sepgsql's DROP Permission checks

    Robert Haas <robertmhaas@gmail.com> — 2012-01-25T16:57:22Z

    On Sun, Jan 22, 2012 at 9:54 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > I tried to implement based on the idea to call object-access-hook with
    > flag; that
    > informs extensions contexts of objects being removed.
    > Because I missed DROP_RESTRICT and DROP_CASCADE are enum type,
    > this patch added performInternalDeletion() instead of OR-masked DROP_INTERNAL.
    > All its difference from performDeletion() is a flag (OAT_DROP_FLAGS_INTERNAL)
    > shall be delivered to extension module. I replaced several performDeletion() by
    > performInternalDeletion() that clean-up objects due to internal stuff.
    >
    > How about this approach?
    
    I generally agree with this line of attack, but I think you've failed
    to find all the cases where a drop should be considered internal, and
    I'd rather add a new parameter to an existing function than define a
    new one that someone might accidentally fail to use in some place
    where it's needed.  Here's a cut-down patch that *just* adds a
    PERFORM_DELETE_INTERNAL flag, plus some related comment additions.  If
    this looks reasonable to you, I'll commit it and then we can work out
    the remaining details.
    
    Since sepgsql doesn't seem to need the DropBehavior, I'm inclined to
    say we shouldn't go to any extra work to pass it just now.  We can
    always add that later if some other client needs it.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    

12. Re: [v9.2] sepgsql's DROP Permission checks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-26T12:27:38Z

    2012/1/25 Robert Haas <robertmhaas@gmail.com>:
    > On Sun, Jan 22, 2012 at 9:54 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> I tried to implement based on the idea to call object-access-hook with
    >> flag; that
    >> informs extensions contexts of objects being removed.
    >> Because I missed DROP_RESTRICT and DROP_CASCADE are enum type,
    >> this patch added performInternalDeletion() instead of OR-masked DROP_INTERNAL.
    >> All its difference from performDeletion() is a flag (OAT_DROP_FLAGS_INTERNAL)
    >> shall be delivered to extension module. I replaced several performDeletion() by
    >> performInternalDeletion() that clean-up objects due to internal stuff.
    >>
    >> How about this approach?
    >
    > I generally agree with this line of attack, but I think you've failed
    > to find all the cases where a drop should be considered internal, and
    > I'd rather add a new parameter to an existing function than define a
    > new one that someone might accidentally fail to use in some place
    > where it's needed.  Here's a cut-down patch that *just* adds a
    > PERFORM_DELETE_INTERNAL flag, plus some related comment additions.  If
    > this looks reasonable to you, I'll commit it and then we can work out
    > the remaining details.
    >
    > Since sepgsql doesn't seem to need the DropBehavior, I'm inclined to
    > say we shouldn't go to any extra work to pass it just now.  We can
    > always add that later if some other client needs it.
    >
    It seems to me reasonable design.
    The attached patch is rebased one according to your perform-deletion patch.
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

13. Re: [v9.2] sepgsql's DROP Permission checks

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T14:40:06Z

    On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > It seems to me reasonable design.
    > The attached patch is rebased one according to your perform-deletion patch.
    
    That looks pretty sensible.  But I don't think this is true any more:
    
    +    Please note that it shall not be checked on the objects removed by
    +    cascaded deletion according to the standard manner in SQL.
    
    I've been thinking more about the question of object access hooks with
    arguments as well.  In a couple of designs you've proposed, we've
    needed InvokeObjectAccessHook0..11 or whatever, and I don't like that
    design - it seems grotty and not type-safe.  However, looking at what
    you did in this patch, I have another idea.  Suppose that we add ONE
    additional argument to the object access hook function, as you've done
    here, and if a particular type of hook invocation needs multiple
    arguments, it can pass them with a struct.  In fact, let's use a
    struct regardless, for uniformity, and pass the value as a void *.
    That is:
    
    typedef struct ObjectAccessDrop {
        int dropflags;
    } ObjectAccessDrop;
    
    At the call site, we do this:
    
    if (object_access_hook)
    {
        ObjectAccessDrop arg;
        arg.dropflags = flags;
        InvokeObjectAccessHook(..., arg);
    }
    
    If there's no argument, then we can just do:
    
    InvokeObjectAccessHook(..., NULL);
    
    The advantage of this is that if we change the structure definition,
    loadable modules compiled against a newer code base should either (1)
    still work or (2) fail to compile.  The way we have it right now, if
    we decide to pass a second argument (say, the DropBehavior) to the
    hook, we're potentially going to silently break sepgsql no matter how
    we do it.  But if we enforce use of a struct, then the only thing the
    client should ever be doing with the argument it gets is casting it to
    ObjectAccessDrop *.  Then, if we've added fields to the struct, the
    code will still do the right thing even if the field order has been
    changed or whatever.   If we've removed fields or changed their data
    types, things should blow up fairly obviously instead of seeming to
    work but actually failing.
    
    Thoughts?
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

14. Re: [v9.2] sepgsql's DROP Permission checks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-01-28T18:53:08Z

    2012/1/26 Robert Haas <robertmhaas@gmail.com>:
    > On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> It seems to me reasonable design.
    >> The attached patch is rebased one according to your perform-deletion patch.
    >
    > That looks pretty sensible.  But I don't think this is true any more:
    >
    > +    Please note that it shall not be checked on the objects removed by
    > +    cascaded deletion according to the standard manner in SQL.
    >
    > I've been thinking more about the question of object access hooks with
    > arguments as well.  In a couple of designs you've proposed, we've
    > needed InvokeObjectAccessHook0..11 or whatever, and I don't like that
    > design - it seems grotty and not type-safe.  However, looking at what
    > you did in this patch, I have another idea.  Suppose that we add ONE
    > additional argument to the object access hook function, as you've done
    > here, and if a particular type of hook invocation needs multiple
    > arguments, it can pass them with a struct.  In fact, let's use a
    > struct regardless, for uniformity, and pass the value as a void *.
    > That is:
    >
    > typedef struct ObjectAccessDrop {
    >    int dropflags;
    > } ObjectAccessDrop;
    >
    > At the call site, we do this:
    >
    > if (object_access_hook)
    > {
    >    ObjectAccessDrop arg;
    >    arg.dropflags = flags;
    >    InvokeObjectAccessHook(..., arg);
    > }
    >
    > If there's no argument, then we can just do:
    >
    > InvokeObjectAccessHook(..., NULL);
    >
    > The advantage of this is that if we change the structure definition,
    > loadable modules compiled against a newer code base should either (1)
    > still work or (2) fail to compile.  The way we have it right now, if
    > we decide to pass a second argument (say, the DropBehavior) to the
    > hook, we're potentially going to silently break sepgsql no matter how
    > we do it.  But if we enforce use of a struct, then the only thing the
    > client should ever be doing with the argument it gets is casting it to
    > ObjectAccessDrop *.  Then, if we've added fields to the struct, the
    > code will still do the right thing even if the field order has been
    > changed or whatever.   If we've removed fields or changed their data
    > types, things should blow up fairly obviously instead of seeming to
    > work but actually failing.
    >
    > Thoughts?
    >
    I also think your idea is good; flexible and reliable toward future enhancement.
    
    If we have one point to improve this idea, is it needed to deliver
    "access", "classid",
    "objectid" and "subid" as separated argument?
    
    If we define a type to deliver information on object access hook as follows:
    
    typedef struct {
        ObjectAccessType    access;
        ObjectAddress          address;
        union {
            struct {
                int    flags;
            } drop;
        } arg;
    } ObjectAccessHookData;
    
    All the argument that object_access_hook takes should be a pointer of this
    structure only, and no need to type cast on the module side.
    
    One disadvantage is that it needs to set up this structure on caller
    side including
    ObjectAccessType and ObjectAddress information. However, it can be embedded
    within preprocessor macro to keep nums of lines as currently we do.
    
    example:
    #define InvaokeDropAccessHook(classid, objectid, objsubid, flags) \
      do {
          if (object_access_hook)
          {
              ObjectAccessHookData  __hook_data;
    
              __hook_data.access = OAT_DROP;
              __hook_data.address.classId  = (classid);
              __hook_data.address.objectId = (objectid);
              __hook_data.address.objectSubid = (objsubid);
              __hook_data.args.drop.flags = (flags);
    
              (*object_access_hook)(&__hook_data);
         }
      } while (0)
    
    How about your opinion?
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    
    

15. Re: [v9.2] sepgsql's DROP Permission checks

    Robert Haas <robertmhaas@gmail.com> — 2012-02-03T14:35:11Z

    On Sat, Jan 28, 2012 at 1:53 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
    >> On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>> It seems to me reasonable design.
    >>> The attached patch is rebased one according to your perform-deletion patch.
    >>
    >> That looks pretty sensible.  But I don't think this is true any more:
    >>
    >> +    Please note that it shall not be checked on the objects removed by
    >> +    cascaded deletion according to the standard manner in SQL.
    >>
    >> I've been thinking more about the question of object access hooks with
    >> arguments as well.  In a couple of designs you've proposed, we've
    >> needed InvokeObjectAccessHook0..11 or whatever, and I don't like that
    >> design - it seems grotty and not type-safe.  However, looking at what
    >> you did in this patch, I have another idea.  Suppose that we add ONE
    >> additional argument to the object access hook function, as you've done
    >> here, and if a particular type of hook invocation needs multiple
    >> arguments, it can pass them with a struct.  In fact, let's use a
    >> struct regardless, for uniformity, and pass the value as a void *.
    >> That is:
    >>
    >> typedef struct ObjectAccessDrop {
    >>    int dropflags;
    >> } ObjectAccessDrop;
    >>
    >> At the call site, we do this:
    >>
    >> if (object_access_hook)
    >> {
    >>    ObjectAccessDrop arg;
    >>    arg.dropflags = flags;
    >>    InvokeObjectAccessHook(..., arg);
    >> }
    >>
    >> If there's no argument, then we can just do:
    >>
    >> InvokeObjectAccessHook(..., NULL);
    >>
    >> The advantage of this is that if we change the structure definition,
    >> loadable modules compiled against a newer code base should either (1)
    >> still work or (2) fail to compile.  The way we have it right now, if
    >> we decide to pass a second argument (say, the DropBehavior) to the
    >> hook, we're potentially going to silently break sepgsql no matter how
    >> we do it.  But if we enforce use of a struct, then the only thing the
    >> client should ever be doing with the argument it gets is casting it to
    >> ObjectAccessDrop *.  Then, if we've added fields to the struct, the
    >> code will still do the right thing even if the field order has been
    >> changed or whatever.   If we've removed fields or changed their data
    >> types, things should blow up fairly obviously instead of seeming to
    >> work but actually failing.
    >>
    >> Thoughts?
    >>
    > I also think your idea is good; flexible and reliable toward future enhancement.
    >
    > If we have one point to improve this idea, is it needed to deliver
    > "access", "classid",
    > "objectid" and "subid" as separated argument?
    >
    > If we define a type to deliver information on object access hook as follows:
    >
    > typedef struct {
    >    ObjectAccessType    access;
    >    ObjectAddress          address;
    >    union {
    >        struct {
    >            int    flags;
    >        } drop;
    >    } arg;
    > } ObjectAccessHookData;
    >
    > All the argument that object_access_hook takes should be a pointer of this
    > structure only, and no need to type cast on the module side.
    >
    > One disadvantage is that it needs to set up this structure on caller
    > side including
    > ObjectAccessType and ObjectAddress information. However, it can be embedded
    > within preprocessor macro to keep nums of lines as currently we do.
    >
    > example:
    > #define InvaokeDropAccessHook(classid, objectid, objsubid, flags) \
    >  do {
    >      if (object_access_hook)
    >      {
    >          ObjectAccessHookData  __hook_data;
    >
    >          __hook_data.access = OAT_DROP;
    >          __hook_data.address.classId  = (classid);
    >          __hook_data.address.objectId = (objectid);
    >          __hook_data.address.objectSubid = (objsubid);
    >          __hook_data.args.drop.flags = (flags);
    >
    >          (*object_access_hook)(&__hook_data);
    >     }
    >  } while (0)
    >
    > How about your opinion?
    
    I don't see any real advantage of that.  One advantage of the current
    design is that any hook types which *don't* require extra arguments
    need not set up and pass a structure; they can just pass NULL.  So I
    suggest we keep classid, objid, and subid as separate arguments, and
    just add one new one which can be type-specific.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

16. Re: [v9.2] sepgsql's DROP Permission checks

    Kohei KaiGai <kaigai@kaigai.gr.jp> — 2012-02-04T15:54:30Z

    2012/2/3 Robert Haas <robertmhaas@gmail.com>:
    > On Sat, Jan 28, 2012 at 1:53 PM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> 2012/1/26 Robert Haas <robertmhaas@gmail.com>:
    >>> On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >>>> It seems to me reasonable design.
    >>>> The attached patch is rebased one according to your perform-deletion patch.
    >>>
    >>> That looks pretty sensible.  But I don't think this is true any more:
    >>>
    >>> +    Please note that it shall not be checked on the objects removed by
    >>> +    cascaded deletion according to the standard manner in SQL.
    >>>
    >>> I've been thinking more about the question of object access hooks with
    >>> arguments as well.  In a couple of designs you've proposed, we've
    >>> needed InvokeObjectAccessHook0..11 or whatever, and I don't like that
    >>> design - it seems grotty and not type-safe.  However, looking at what
    >>> you did in this patch, I have another idea.  Suppose that we add ONE
    >>> additional argument to the object access hook function, as you've done
    >>> here, and if a particular type of hook invocation needs multiple
    >>> arguments, it can pass them with a struct.  In fact, let's use a
    >>> struct regardless, for uniformity, and pass the value as a void *.
    >>> That is:
    >>>
    >>> typedef struct ObjectAccessDrop {
    >>>    int dropflags;
    >>> } ObjectAccessDrop;
    >>>
    >>> At the call site, we do this:
    >>>
    >>> if (object_access_hook)
    >>> {
    >>>    ObjectAccessDrop arg;
    >>>    arg.dropflags = flags;
    >>>    InvokeObjectAccessHook(..., arg);
    >>> }
    >>>
    >>> If there's no argument, then we can just do:
    >>>
    >>> InvokeObjectAccessHook(..., NULL);
    >>>
    >>> The advantage of this is that if we change the structure definition,
    >>> loadable modules compiled against a newer code base should either (1)
    >>> still work or (2) fail to compile.  The way we have it right now, if
    >>> we decide to pass a second argument (say, the DropBehavior) to the
    >>> hook, we're potentially going to silently break sepgsql no matter how
    >>> we do it.  But if we enforce use of a struct, then the only thing the
    >>> client should ever be doing with the argument it gets is casting it to
    >>> ObjectAccessDrop *.  Then, if we've added fields to the struct, the
    >>> code will still do the right thing even if the field order has been
    >>> changed or whatever.   If we've removed fields or changed their data
    >>> types, things should blow up fairly obviously instead of seeming to
    >>> work but actually failing.
    >>>
    >>> Thoughts?
    >>>
    >> I also think your idea is good; flexible and reliable toward future enhancement.
    >>
    >> If we have one point to improve this idea, is it needed to deliver
    >> "access", "classid",
    >> "objectid" and "subid" as separated argument?
    >>
    >> If we define a type to deliver information on object access hook as follows:
    >>
    >> typedef struct {
    >>    ObjectAccessType    access;
    >>    ObjectAddress          address;
    >>    union {
    >>        struct {
    >>            int    flags;
    >>        } drop;
    >>    } arg;
    >> } ObjectAccessHookData;
    >>
    >> All the argument that object_access_hook takes should be a pointer of this
    >> structure only, and no need to type cast on the module side.
    >>
    >> One disadvantage is that it needs to set up this structure on caller
    >> side including
    >> ObjectAccessType and ObjectAddress information. However, it can be embedded
    >> within preprocessor macro to keep nums of lines as currently we do.
    >>
    >> example:
    >> #define InvaokeDropAccessHook(classid, objectid, objsubid, flags) \
    >>  do {
    >>      if (object_access_hook)
    >>      {
    >>          ObjectAccessHookData  __hook_data;
    >>
    >>          __hook_data.access = OAT_DROP;
    >>          __hook_data.address.classId  = (classid);
    >>          __hook_data.address.objectId = (objectid);
    >>          __hook_data.address.objectSubid = (objsubid);
    >>          __hook_data.args.drop.flags = (flags);
    >>
    >>          (*object_access_hook)(&__hook_data);
    >>     }
    >>  } while (0)
    >>
    >> How about your opinion?
    >
    > I don't see any real advantage of that.  One advantage of the current
    > design is that any hook types which *don't* require extra arguments
    > need not set up and pass a structure; they can just pass NULL.  So I
    > suggest we keep classid, objid, and subid as separate arguments, and
    > just add one new one which can be type-specific.
    >
    # I send it again with "reply-all".
    
    OK, I modified the patch according to your suggestions.
    
    object_access_hook was extended to take an argument of void * pointer,
    and InvokeObjectAccessHook was also allows to deliver it.
    
    On OAT_DROP event, its invocation is enclosed by if-block as:
    
    +   /* DROP hook of the objects being removed */
    +   if (object_access_hook)
    +   {
    +       ObjectAccessDrop    drop_arg;
    +       drop_arg.dropflags = flags;
    +       InvokeObjectAccessHook(OAT_DROP, object->classId, object->objectId,
    +                              object->objectSubId, &drop_arg);
    +   }
    
    Should we have InvokeObjectDropHook() macro to provide
    a series of invocation process with OAT_DROP event, instead
    of the flat if-block?
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@kaigai.gr.jp>
    

17. Re: [v9.2] sepgsql's DROP Permission checks

    Robert Haas <robertmhaas@gmail.com> — 2012-03-09T19:41:40Z

    On Sat, Feb 4, 2012 at 10:54 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    > OK, I modified the patch according to your suggestions.
    >
    > object_access_hook was extended to take an argument of void * pointer,
    > and InvokeObjectAccessHook was also allows to deliver it.
    
    Sorry for the long radio silence on this patch.  The changes to the
    object-access hook stuff seem sound to me now, so I've gone ahead and
    committed that part of this.  I'll look at the rest next.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

18. Re: [v9.2] sepgsql's DROP Permission checks

    Robert Haas <robertmhaas@gmail.com> — 2012-03-09T20:19:24Z

    On Fri, Mar 9, 2012 at 2:41 PM, Robert Haas <robertmhaas@gmail.com> wrote:
    > On Sat, Feb 4, 2012 at 10:54 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
    >> OK, I modified the patch according to your suggestions.
    >>
    >> object_access_hook was extended to take an argument of void * pointer,
    >> and InvokeObjectAccessHook was also allows to deliver it.
    >
    > Sorry for the long radio silence on this patch.  The changes to the
    > object-access hook stuff seem sound to me now, so I've gone ahead and
    > committed that part of this.  I'll look at the rest next.
    
    That looks fine also, so committed that too.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company