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

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(at)kaigai(dot)gr(dot)jp>

On Tue, Jan 10, 2012 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/17 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jan 10, 2012 at 7:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Tue, Jan 17, 2012 at 10:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Jan 17, 2012 at 10:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)jp>:
> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Jan 19, 2012 at 3:51 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 2012/1/19 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>>> On Wed, Jan 18, 2012 at 9:50 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jan 22, 2012 at 9:54 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/25 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 22, 2012 at 9:54 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/1/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Sat, Jan 28, 2012 at 1:53 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 2012/1/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

2012/2/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Jan 28, 2012 at 1:53 PM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 2012/1/26 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Thu, Jan 26, 2012 at 7:27 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

On Sat, Feb 4, 2012 at 10:54 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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

On Fri, Mar 9, 2012 at 2:41 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Feb 4, 2012 at 10:54 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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