Thread

1. security hooks on object creation

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-09T10:52:14Z

   The attached patch provides plugin modules a hook just after object
   creation time. In typical use cases, it enables to assign default
   security labels on object creation by the external security providers.
   
   As Robert suggested before, it provides a generic purpose main hook.
   It takes an enum of ObjectAccessType which informs plugins what kind
   of accesses are required, and identifier of the object to be referenced.
   But, in this version, no additional information, such as new name in
   ALTER xxx RENAME TO, are not supported.
   
   The ObjectAccessType is defined as follows:
   
     typedef enum ObjectAccessType {
       OAT_POST_CREATE,    /* Post creation fixups; such as security labeling */
     } ObjectAccessType;
   
   We will support more complete kind of access types in the future version,
   however, we focus on default labeling rather than DDL permissions right
   now, so only OAT_POST_CREATE is defined here.
   Perhaps, we will add OAT_ALTER, OAT_DROP, OAT_COMMENT and so on.
   
   In this patch, I put hooks on the place just after creation of database
   objects that we can assign security labels. (schema, relation, attribute,
   procedure, language, type, large object)
   
   However, I didn't touch or move CommandCounterIncrement() yet, although
   we had a long discussion MVCC visibility of new object.
   Because I'm not clear whether it is really preferable to inject CCIs
   onto random points such as TypeCreate() or ProcedureCreate() under
   development of the version killed by myself.
   (In other words, it was simply ugly...)
   
   At least, we can see the new entries with SnapshotSelf, although we will
   pay performance penalty. If so, it is an idea not to touch anything
   related to CCIs.
   The purpose of post creation hooks are assignment of default security
   labels, not DDL permissions. So, it is not a bad idea not to touch
   routines related to CCIs in the earlier version of external security
   provider.
   
   In this patch, we put InvokeObjectAccessHook0 on the following functions.
   
   - heap_create_with_catalog() for relations/attributes
   - ATExecAddColumn() for attributes
   - NamespaceCreate() for schemas
   - ProcedureCreate() for aggregates/functions
   - TypeCreate() and TypeShellMake() for types
   - create_proc_lang() for procedural languages
   - inv_create() for large objects
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@ak.jp.nec.com>
   

2. Re: security hooks on object creation

   Itagaki Takahiro <itagaki.takahiro@gmail.com> — 2010-11-09T11:34:51Z

   2010/11/9 KaiGai Kohei <kaigai@ak.jp.nec.com>:
   > The attached patch provides plugin modules a hook just after object
   > creation time. In typical use cases, it enables to assign default
   > security labels on object creation by the external security providers.
   
   It looks like "DDL Trigger" on other database products.
   Do we need to consider both security hooks and DDL triggers now?
   Or, is it enough to design DLL triggers after the hooks are merged?
   Low-level hooks might be better for security providers because
   SQL-level triggers could be uninstall by superusers.
   
   -- 
   Itagaki Takahiro
   
   

3. Re: security hooks on object creation

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-11-09T12:46:37Z

   (2010/11/09 20:34), Itagaki Takahiro wrote:
   > 2010/11/9 KaiGai Kohei<kaigai@ak.jp.nec.com>:
   >> The attached patch provides plugin modules a hook just after object
   >> creation time. In typical use cases, it enables to assign default
   >> security labels on object creation by the external security providers.
   >
   > It looks like "DDL Trigger" on other database products.
   > Do we need to consider both security hooks and DDL triggers now?
   > Or, is it enough to design DLL triggers after the hooks are merged?
   > Low-level hooks might be better for security providers because
   > SQL-level triggers could be uninstall by superusers.
   >
   An interesting viewpoint. Does the DDL trigger allow us to do something
   on CREATE/ALTER/DROP command?
   
   One thing we need to pay attention is that CREATE command is an exception
   from any other DDL commands, because the database object to be modified
   does not exist before the actual works. So, I'm saying we need both of
   prep/post creation hooks in the world of complete features.
   Meanwhile, I don't think we need security hooks post ALTER/DROP commands.
   Thus, we will put security hooks next to the existing permission checks,
   not after the actual works of these commands.
   Is it reasonable for DDL triggers (if it has something like BEFORE/AFTER)?
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

4. Re: security hooks on object creation

   Robert Haas <robertmhaas@gmail.com> — 2010-11-10T04:06:50Z

   2010/11/9 KaiGai Kohei <kaigai@ak.jp.nec.com>:
   > The attached patch provides plugin modules a hook just after object
   > creation time. In typical use cases, it enables to assign default
   > security labels on object creation by the external security providers.
   >
   > As Robert suggested before, it provides a generic purpose main hook.
   > It takes an enum of ObjectAccessType which informs plugins what kind
   > of accesses are required, and identifier of the object to be referenced.
   > But, in this version, no additional information, such as new name in
   > ALTER xxx RENAME TO, are not supported.
   >
   > The ObjectAccessType is defined as follows:
   >
   >  typedef enum ObjectAccessType {
   >    OAT_POST_CREATE,    /* Post creation fixups; such as security labeling */
   >  } ObjectAccessType;
   >
   > We will support more complete kind of access types in the future version,
   > however, we focus on default labeling rather than DDL permissions right
   > now, so only OAT_POST_CREATE is defined here.
   > Perhaps, we will add OAT_ALTER, OAT_DROP, OAT_COMMENT and so on.
   >
   > In this patch, I put hooks on the place just after creation of database
   > objects that we can assign security labels. (schema, relation, attribute,
   > procedure, language, type, large object)
   >
   > However, I didn't touch or move CommandCounterIncrement() yet, although
   > we had a long discussion MVCC visibility of new object.
   > Because I'm not clear whether it is really preferable to inject CCIs
   > onto random points such as TypeCreate() or ProcedureCreate() under
   > development of the version killed by myself.
   > (In other words, it was simply ugly...)
   >
   > At least, we can see the new entries with SnapshotSelf, although we will
   > pay performance penalty. If so, it is an idea not to touch anything
   > related to CCIs.
   > The purpose of post creation hooks are assignment of default security
   > labels, not DDL permissions. So, it is not a bad idea not to touch
   > routines related to CCIs in the earlier version of external security
   > provider.
   >
   > In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >
   > - heap_create_with_catalog() for relations/attributes
   > - ATExecAddColumn() for attributes
   > - NamespaceCreate() for schemas
   > - ProcedureCreate() for aggregates/functions
   > - TypeCreate() and TypeShellMake() for types
   > - create_proc_lang() for procedural languages
   > - inv_create() for large objects
   
   I think you ought to try to arrange to avoid the overhead of a
   function call in the common case where nobody's using the hook.
   That's why I originally suggested making InvokeObjectAccessHook() a
   macro around the actual function call.
   
   I don't want to refer to this as a framework for enhanced security
   providers.  Let's stick with the term "object access hook".  Calling
   it an enhanced security provider overspecifies; it could equally well
   be used for, say, logging.
   
   Is there any compelling reason not to apply this to every object type
   in the system (e.g. all the ones COMMENT can apply to)?  I don't see
   any reason to restrict it to the set of objects to which it's sensible
   to apply security labels.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

5. Re: security hooks on object creation

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-11-10T13:33:30Z

   (2010/11/10 13:06), Robert Haas wrote:
   >> In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >>
   >> - heap_create_with_catalog() for relations/attributes
   >> - ATExecAddColumn() for attributes
   >> - NamespaceCreate() for schemas
   >> - ProcedureCreate() for aggregates/functions
   >> - TypeCreate() and TypeShellMake() for types
   >> - create_proc_lang() for procedural languages
   >> - inv_create() for large objects
   >
   > I think you ought to try to arrange to avoid the overhead of a
   > function call in the common case where nobody's using the hook.
   > That's why I originally suggested making InvokeObjectAccessHook() a
   > macro around the actual function call.
   >
   Hmm. Although I have little preference here, the penalty to call
   an empty function (when no plugins are installed) is not visible,
   because frequency of DDL commands are not high.
   Even so, is it necessary to replace them by macros?
   
   > I don't want to refer to this as a framework for enhanced security
   > providers.  Let's stick with the term "object access hook".  Calling
   > it an enhanced security provider overspecifies; it could equally well
   > be used for, say, logging.
   >
   OK. As Itagaki-san also pointed out, we may be able to utilize the hooks
   in other purposes. Although I designed it in similar manner with security
   label provider, I'll revise it like as other hooks doing.
   
   > Is there any compelling reason not to apply this to every object type
   > in the system (e.g. all the ones COMMENT can apply to)?  I don't see
   > any reason to restrict it to the set of objects to which it's sensible
   > to apply security labels.
   >
   Because I thought too many hooks within one patch gives burden to reviewers,
   so I restricted it on a part of object classes in this version.
   However,it is not a compelling reason.
   OK, I'll try to revise the patch soon.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

6. Re: security hooks on object creation

   Robert Haas <robertmhaas@gmail.com> — 2010-11-10T18:00:03Z

   On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei <kaigai@kaigai.gr.jp> wrote:
   > (2010/11/10 13:06), Robert Haas wrote:
   >>>
   >>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >>>
   >>> - heap_create_with_catalog() for relations/attributes
   >>> - ATExecAddColumn() for attributes
   >>> - NamespaceCreate() for schemas
   >>> - ProcedureCreate() for aggregates/functions
   >>> - TypeCreate() and TypeShellMake() for types
   >>> - create_proc_lang() for procedural languages
   >>> - inv_create() for large objects
   >>
   >> I think you ought to try to arrange to avoid the overhead of a
   >> function call in the common case where nobody's using the hook.
   >> That's why I originally suggested making InvokeObjectAccessHook() a
   >> macro around the actual function call.
   >>
   > Hmm. Although I have little preference here, the penalty to call
   > an empty function (when no plugins are installed) is not visible,
   > because frequency of DDL commands are not high.
   > Even so, is it necessary to replace them by macros?
   
   It's a fair point.  I'm open to other opinions but my vote is to shove
   a macro in there.  A pointer test is cheaper than a function call, and
   doesn't really complicate things much.
   
   -- 
   Robert Haas
   EnterpriseDB: http://www.enterprisedb.com
   The Enterprise PostgreSQL Company
   
   

7. Re: security hooks on object creation

   Kohei KaiGai <kaigai@kaigai.gr.jp> — 2010-11-10T22:41:11Z

   (2010/11/11 3:00), Robert Haas wrote:
   > On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
   >> (2010/11/10 13:06), Robert Haas wrote:
   >>>>
   >>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >>>>
   >>>> - heap_create_with_catalog() for relations/attributes
   >>>> - ATExecAddColumn() for attributes
   >>>> - NamespaceCreate() for schemas
   >>>> - ProcedureCreate() for aggregates/functions
   >>>> - TypeCreate() and TypeShellMake() for types
   >>>> - create_proc_lang() for procedural languages
   >>>> - inv_create() for large objects
   >>>
   >>> I think you ought to try to arrange to avoid the overhead of a
   >>> function call in the common case where nobody's using the hook.
   >>> That's why I originally suggested making InvokeObjectAccessHook() a
   >>> macro around the actual function call.
   >>>
   >> Hmm. Although I have little preference here, the penalty to call
   >> an empty function (when no plugins are installed) is not visible,
   >> because frequency of DDL commands are not high.
   >> Even so, is it necessary to replace them by macros?
   >
   > It's a fair point.  I'm open to other opinions but my vote is to shove
   > a macro in there.  A pointer test is cheaper than a function call, and
   > doesn't really complicate things much.
   >
   Since I have no strong preference function call here, so, I'll revise my
   patch according to your vote.
   
   Thanks,
   -- 
   KaiGai Kohei <kaigai@kaigai.gr.jp>
   
   

8. Re: security hooks on object creation

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-12T10:34:42Z

   I revised my patch according to the prior suggestions.
   
   Invocation of the hooks is encapsulated within macro, not function:
   
     + #define InvokeObjectAccessHook0(access,classId,objectId,subId)        \
     +   do {                                                                \
     +       if (object_access_hook)                                         \
     +           (*object_access_hook)((access),(classId),(objectId),(subId)); \
     +   } while(0)
   
   The "0" of tail means that it does not takes any arguments except for
   object-ids, like syscache code.
   It will reduce impact when we want to add arguments of the hooks.
   
   
   In the previous version, it just support seven object classes that is
   allowed to assign security labels. But, Robert pointed out the purpose
   of post-creation hook is limited to security labeling. So, I expand its
   coverage into all the commentable object classes.
   
    - relations:	heap_create_with_catalog()
    - constraint:	CreateConstraintEntry()
    - conversion:	ConversionCreate()
    - schema:	NamespaceCreate()
    - operator:	OperatorCreate() and OperatorShellMake()
    - procedure:	ProcedureCreate()
    - type:	TypeCreate() and TypeShellMake()
    - database:	createdb()
    - cast:	CreateCast()
    - opfamily:	CreateOpFamily()
    - opclass:	DefineOpClass()
    - language:	create_proc_lang()
    - attribute:	ATExecAddColumn()
    - tablespace:	CreateTableSpace()
    - trigger:	CreateTrigger()
    - ts_parser:	DefineTSParser()
    - ts_dict:	DefineTSDictionary()
    - ts_template:	DefineTSTemplate()
    - ts_config:	DefineTSConfiguration()
    - role:	CreateRole()
    - rule:	InsertRule()
    - largeobject:	inv_create()
   
   The post-creation hooks are put on the place just after adding dependency
   of the new object, if the object class uses dependency mechanism.
   I believe it will be a clear guidance for the future maintenance works.
   
   Thanks,
   
   (2010/11/11 7:41), KaiGai Kohei wrote:
   > (2010/11/11 3:00), Robert Haas wrote:
   >> On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai@kaigai.gr.jp> wrote:
   >>> (2010/11/10 13:06), Robert Haas wrote:
   >>>>>
   >>>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >>>>>
   >>>>> - heap_create_with_catalog() for relations/attributes
   >>>>> - ATExecAddColumn() for attributes
   >>>>> - NamespaceCreate() for schemas
   >>>>> - ProcedureCreate() for aggregates/functions
   >>>>> - TypeCreate() and TypeShellMake() for types
   >>>>> - create_proc_lang() for procedural languages
   >>>>> - inv_create() for large objects
   >>>>
   >>>> I think you ought to try to arrange to avoid the overhead of a
   >>>> function call in the common case where nobody's using the hook.
   >>>> That's why I originally suggested making InvokeObjectAccessHook() a
   >>>> macro around the actual function call.
   >>>>
   >>> Hmm. Although I have little preference here, the penalty to call
   >>> an empty function (when no plugins are installed) is not visible,
   >>> because frequency of DDL commands are not high.
   >>> Even so, is it necessary to replace them by macros?
   >>
   >> It's a fair point. I'm open to other opinions but my vote is to shove
   >> a macro in there. A pointer test is cheaper than a function call, and
   >> doesn't really complicate things much.
   >>
   > Since I have no strong preference function call here, so, I'll revise my
   > patch according to your vote.
   > 
   > Thanks,
   
   
   -- 
   KaiGai Kohei <kaigai@ak.jp.nec.com>
   
   

9. Re: security hooks on object creation

   Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-12T12:43:20Z

   (2010/11/12 19:34), KaiGai Kohei wrote:
   > I revised my patch according to the prior suggestions.
   > 
   I'm sorry. I revised my patch, but not attached.
   
   Please see this attached one.
   
   Thanks,
   
   > Invocation of the hooks is encapsulated within macro, not function:
   > 
   >    + #define InvokeObjectAccessHook0(access,classId,objectId,subId)        \
   >    +   do {                                                                \
   >    +       if (object_access_hook)                                         \
   >    +           (*object_access_hook)((access),(classId),(objectId),(subId)); \
   >    +   } while(0)
   > 
   > The "0" of tail means that it does not takes any arguments except for
   > object-ids, like syscache code.
   > It will reduce impact when we want to add arguments of the hooks.
   > 
   > 
   > In the previous version, it just support seven object classes that is
   > allowed to assign security labels. But, Robert pointed out the purpose
   > of post-creation hook is limited to security labeling. So, I expand its
   > coverage into all the commentable object classes.
   > 
   >   - relations:	heap_create_with_catalog()
   >   - constraint:	CreateConstraintEntry()
   >   - conversion:	ConversionCreate()
   >   - schema:	NamespaceCreate()
   >   - operator:	OperatorCreate() and OperatorShellMake()
   >   - procedure:	ProcedureCreate()
   >   - type:	TypeCreate() and TypeShellMake()
   >   - database:	createdb()
   >   - cast:	CreateCast()
   >   - opfamily:	CreateOpFamily()
   >   - opclass:	DefineOpClass()
   >   - language:	create_proc_lang()
   >   - attribute:	ATExecAddColumn()
   >   - tablespace:	CreateTableSpace()
   >   - trigger:	CreateTrigger()
   >   - ts_parser:	DefineTSParser()
   >   - ts_dict:	DefineTSDictionary()
   >   - ts_template:	DefineTSTemplate()
   >   - ts_config:	DefineTSConfiguration()
   >   - role:	CreateRole()
   >   - rule:	InsertRule()
   >   - largeobject:	inv_create()
   > 
   > The post-creation hooks are put on the place just after adding dependency
   > of the new object, if the object class uses dependency mechanism.
   > I believe it will be a clear guidance for the future maintenance works.
   > 
   > Thanks,
   > 
   > (2010/11/11 7:41), KaiGai Kohei wrote:
   >> (2010/11/11 3:00), Robert Haas wrote:
   >>> On Wed, Nov 10, 2010 at 8:33 AM, KaiGai Kohei<kaigai@kaigai.gr.jp>  wrote:
   >>>> (2010/11/10 13:06), Robert Haas wrote:
   >>>>>>
   >>>>>> In this patch, we put InvokeObjectAccessHook0 on the following functions.
   >>>>>>
   >>>>>> - heap_create_with_catalog() for relations/attributes
   >>>>>> - ATExecAddColumn() for attributes
   >>>>>> - NamespaceCreate() for schemas
   >>>>>> - ProcedureCreate() for aggregates/functions
   >>>>>> - TypeCreate() and TypeShellMake() for types
   >>>>>> - create_proc_lang() for procedural languages
   >>>>>> - inv_create() for large objects
   >>>>>
   >>>>> I think you ought to try to arrange to avoid the overhead of a
   >>>>> function call in the common case where nobody's using the hook.
   >>>>> That's why I originally suggested making InvokeObjectAccessHook() a
   >>>>> macro around the actual function call.
   >>>>>
   >>>> Hmm. Although I have little preference here, the penalty to call
   >>>> an empty function (when no plugins are installed) is not visible,
   >>>> because frequency of DDL commands are not high.
   >>>> Even so, is it necessary to replace them by macros?
   >>>
   >>> It's a fair point. I'm open to other opinions but my vote is to shove
   >>> a macro in there. A pointer test is cheaper than a function call, and
   >>> doesn't really complicate things much.
   >>>
   >> Since I have no strong preference function call here, so, I'll revise my
   >> patch according to your vote.
   >>
   >> Thanks,
   > 
   > 
   
   
   -- 
   KaiGai Kohei <kaigai@ak.jp.nec.com>
   

10. Re: security hooks on object creation

    Robert Haas <robertmhaas@gmail.com> — 2010-11-22T03:11:43Z

    2010/11/12 KaiGai Kohei <kaigai@ak.jp.nec.com>:
    > (2010/11/12 19:34), KaiGai Kohei wrote:
    >> I revised my patch according to the prior suggestions.
    >>
    > I'm sorry. I revised my patch, but not attached.
    >
    > Please see this attached one.
    
    I'm satisfied with this approach, although I intend to change
    InvokeObjectAccessHook0 to simply InvokeObjectAccessHook before
    committing it; and correct your use of AttributeRelationId to
    RelationRelationId for consistency with the rest of the code.  What
    I'm not quite sure about is where to put the definitions you've added
    to a new file utils/hooks.h; I don't feel that's a very appropriate
    location.  It's tempting to put them in utils/acl.h just because this
    is vaguely access-control related and that header is already included
    in most of the right places, but maybe that's too much of a stretch;
    or perhaps catalog/catalog.h, although that doesn't feel quite right
    either.  If we are going to add a new header file, I still don't like
    utils/hooks.h much - it's considerably more generic than can be
    justified by its contents.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

11. Re: security hooks on object creation

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-24T02:54:47Z

    Thanks for your reviewing, and sorry for the late reply.
    I've not been available for a few days.
    
    (2010/11/22 12:11), Robert Haas wrote:
    > 2010/11/12 KaiGai Kohei<kaigai@ak.jp.nec.com>:
    >> (2010/11/12 19:34), KaiGai Kohei wrote:
    >>> I revised my patch according to the prior suggestions.
    >>>
    >> I'm sorry. I revised my patch, but not attached.
    >>
    >> Please see this attached one.
    > 
    > I'm satisfied with this approach, although I intend to change
    > InvokeObjectAccessHook0 to simply InvokeObjectAccessHook before
    > committing it;
    
    OK. We have no other object-access-type which takes any arguments
    right now. It is quite cosmetic things, so we may be able to add
    the number of arguments later, such as SysCache.
    
    > and correct your use of AttributeRelationId to
    > RelationRelationId for consistency with the rest of the code.
    
    Oops, it was my bug. I'll fix it.
    
    > What
    > I'm not quite sure about is where to put the definitions you've added
    > to a new file utils/hooks.h; I don't feel that's a very appropriate
    > location.  It's tempting to put them in utils/acl.h just because this
    > is vaguely access-control related and that header is already included
    > in most of the right places, but maybe that's too much of a stretch;
    > or perhaps catalog/catalog.h, although that doesn't feel quite right
    > either.  If we are going to add a new header file, I still don't like
    > utils/hooks.h much - it's considerably more generic than can be
    > justified by its contents.
    > 
    I don't think utils/acl.h is long-standing right place, because we
    intended not to restrict the purpose of this hooks to access controls
    as you mentioned.
    
    I think somewhere under the catalog/ directory is a good idea because
    it hooks events that user wants (eventually) to modify system catalogs.
    How about catalog/hooks.h, instead of utils/hooks.h?
    
    Thanks,
    -- 
    KaiGai Kohei <kaigai@ak.jp.nec.com>
    
    

12. Re: security hooks on object creation

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

    2010/11/23 KaiGai Kohei <kaigai@ak.jp.nec.com>:
    >> What
    >> I'm not quite sure about is where to put the definitions you've added
    >> to a new file utils/hooks.h; I don't feel that's a very appropriate
    >> location.  It's tempting to put them in utils/acl.h just because this
    >> is vaguely access-control related and that header is already included
    >> in most of the right places, but maybe that's too much of a stretch;
    >> or perhaps catalog/catalog.h, although that doesn't feel quite right
    >> either.  If we are going to add a new header file, I still don't like
    >> utils/hooks.h much - it's considerably more generic than can be
    >> justified by its contents.
    >>
    > I don't think utils/acl.h is long-standing right place, because we
    > intended not to restrict the purpose of this hooks to access controls
    > as you mentioned.
    >
    > I think somewhere under the catalog/ directory is a good idea because
    > it hooks events that user wants (eventually) to modify system catalogs.
    > How about catalog/hooks.h, instead of utils/hooks.h?
    
    Well, if we're going to create a new header file for this, I think it
    should be called something like catalog/objectaccess.h, rather than
    just hooks.h.  But I'd rather reuse something that's already there,
    all things being equal.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company
    
    

13. Re: security hooks on object creation

    Kouhei Kaigai <kaigai@ak.jp.nec.com> — 2010-11-25T05:03:48Z

    The attached patch is a revised patch.
    
    - The utils/hooks.h was renamed to catalog/objectaccess.h
    - Numeric in the tail of InvokeObjectAccessHook0() has gone.
    - Fixed bug in ATExecAddColumn; it gave AttributeRelationId
      to the hook instead of RelationRelationId.
    
    In addition, I found that we didn't put post-creation hook
    on foreign data wrapper, foreign server and user mapping
    exceptionally. So, I put this hook around their command
    handler like any other object classes.
    
    Thanks,
    
    (2010/11/24 12:07), Robert Haas wrote:
    > 2010/11/23 KaiGai Kohei<kaigai@ak.jp.nec.com>:
    >>> What
    >>> I'm not quite sure about is where to put the definitions you've added
    >>> to a new file utils/hooks.h; I don't feel that's a very appropriate
    >>> location.  It's tempting to put them in utils/acl.h just because this
    >>> is vaguely access-control related and that header is already included
    >>> in most of the right places, but maybe that's too much of a stretch;
    >>> or perhaps catalog/catalog.h, although that doesn't feel quite right
    >>> either.  If we are going to add a new header file, I still don't like
    >>> utils/hooks.h much - it's considerably more generic than can be
    >>> justified by its contents.
    >>>
    >> I don't think utils/acl.h is long-standing right place, because we
    >> intended not to restrict the purpose of this hooks to access controls
    >> as you mentioned.
    >>
    >> I think somewhere under the catalog/ directory is a good idea because
    >> it hooks events that user wants (eventually) to modify system catalogs.
    >> How about catalog/hooks.h, instead of utils/hooks.h?
    > 
    > Well, if we're going to create a new header file for this, I think it
    > should be called something like catalog/objectaccess.h, rather than
    > just hooks.h.  But I'd rather reuse something that's already there,
    > all things being equal.
    > 
    
    
    -- 
    KaiGai Kohei <kaigai@ak.jp.nec.com>
    

14. Re: security hooks on object creation

    Robert Haas <robertmhaas@gmail.com> — 2010-11-25T16:52:38Z

    2010/11/25 KaiGai Kohei <kaigai@ak.jp.nec.com>:
    > The attached patch is a revised patch.
    >
    > - The utils/hooks.h was renamed to catalog/objectaccess.h
    > - Numeric in the tail of InvokeObjectAccessHook0() has gone.
    > - Fixed bug in ATExecAddColumn; it gave AttributeRelationId
    >  to the hook instead of RelationRelationId.
    >
    > In addition, I found that we didn't put post-creation hook
    > on foreign data wrapper, foreign server and user mapping
    > exceptionally. So, I put this hook around their command
    > handler like any other object classes.
    
    Committed with minor, mostly cosmetic adjustments.
    
    -- 
    Robert Haas
    EnterpriseDB: http://www.enterprisedb.com
    The Enterprise PostgreSQL Company