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

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>