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

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>