From: | Robert Haas <robertmhaas(at)gmail(dot)com> |
---|---|
To: | Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> |
Cc: | PgHacker <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [v9.2] sepgsql's DROP Permission checks |
Date: | 2012-02-03 14:35:11 |
Message-ID: | CA+TgmoYN4r1Jwf6SQ1G7y93y3t2=MCCCR0OoUFemAVppYG32BA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Duncan Rance | 2012-02-03 15:24:50 | Re: BUG #6425: Bus error in slot_deform_tuple |
Previous Message | Robert Haas | 2012-02-03 12:52:34 | Re: patch for parallel pg_dump |