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

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