Revised patches are attached.
At the part-1, I adjusted macro definitions to have NULL check for
object_access_hook prior to its invocation, as follows:
+#define InvokeObjectPostCreateHook(classId,objectId,subId) \
+ InvokeObjectPostCreateHookArg((classId),(objectId),(subId),false)
+#define InvokeObjectPostCreateHookArg(classId,objectId,subId,is_internal) \
+ do { \
+ if (object_access_hook) \
+ RunObjectPostCreateHook((classId),(objectId),(subId), \
+ (is_internal)); \
+ } while(0)
At the part-2, I adjusted invocation points of the hook, to allow
extensions to get control even if no fields are actually updated.
Typically, it was moved to outside of the if-block that checks
whether old-owner is identical with new-owner on ALTER OWNER
TO commands.
Also, I added OAT_PREP_ALTER event type to handle case of
ALTER ... SET TABLESPACE command well, to prevent unauthorized
execution of whole of the table rewrite, even though it shall be eventually
blocked by OAT_POST_ALTER event.
Thanks,
2012/12/3 Kohei KaiGai <kaigai@kaigai.gr.jp>:
> 2012/12/3 Robert Haas <robertmhaas@gmail.com>:
>> On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai <kaigai@kaigai.gr.jp> wrote:
>>> * Do we need OAT_POST_ALTER hook even if no fields were updated
>>> actually? In case when ALTER SET OWNER, it checks object's ownership
>>> only when current and new user-id is not same. Right now, I follow this
>>> manner on OAT_POST_ALTER invocation.
>>> However, I'm inclined to consider the hook should be invoked when no
>>> fields are actually updated also. (It allows extension-side to determine
>>> necessity of processing something.)
>>
>> I agree. I think it should always be called.
>>
> OK, I'll move the point to invoke hooks into outside of the if-block.
>
>>> * When tablespace of relation was changed, it seems to me the point to
>>> invoke OAT_POST_ALTER hook should be "after" ATRewriteTable().
>>> However, it usually long time to rewrite whole the table if it already have
>>> large number of rows. I'm not 100% certain to put hook here, so this
>>> version does not support hook when tablespace changes.
>>
>> Well, if it's a post-alter hook, it should presumably happen as close
>> to the end of processing as possible. But are you sure that's really
>> what you want? I would think that for SE-Linux you'd be wanting to
>> get control much earlier in the process.
>>
> In my preference, an error shall be raised prior to whole of the table
> rewrite stuff; it can take exclusive table lock during possible TB or PB
> of disk-i/o. Even if sepgsql got control after all and raise an access
> control error, I'm not happy so much. :-(
>
> As we discussed before, it is hard to determine which attributes shall
> be informed to extension via object_access_hook, so the proposed
> post-alter hook (that allows to compare older and newer versions)
> works fine on 99% cases.
> However, I'm inclined to handle SET TABLESPACE as an exception
> of this scenario. For example, an idea is to define OAT_PREP_ALTER
> event additionally, but only invoked very limited cases that takes
> unignorable side-effects until system catalog updates.
> For consistency of hook, OAT_POST_ALTER event may also ought
> to be invoked just after catalog updates of pg_class->reltablespace,
> but is_internal=true.
>
> How about your opinion?
>
> Thanks,
> --
> KaiGai Kohei <kaigai@kaigai.gr.jp>
--
KaiGai Kohei <kaigai@kaigai.gr.jp>
