Re: [v9.3] OAT_POST_ALTER object access hooks

The attached patch adds a new event type of object_access_hook;
named OAT_POST_ALTER. As literal, it allows extensions to catch
controls just after system catalogs are updated.
It also adds sepgsql permission check capability on some ALTER
commands, but not any.

The hooks are designed to locate between heap update's and next
command-counter increment, to allow extensions to reference
both of older and newer version of catalog entry.
Unless CCI is called, we can reference older one with SnapshotNow,
and newer one with SnapshotSelf, as attached sepgsql code doing
to detect namespace / name changes.
As we discussed before, it is a significant point to pick-up which
information should be delivered to extensions through the hooks,
to avoid design too specific for a particular extension's requirement.
I believe it is the best way to inform which fields were updated on
the relevant ALTER command.

The OAT_POST_ALTER can take two arguments. The one is
"is_internal" flag similar to DROP or POST_CREATE hook.
The other is "auxiliary_id" field to identify a particular entry of
the catalog that takes a couple of IDs; E.g, pg_db_role_setting
or pg_inherits.

It might be noticed that some OAT_POST_CREATE hooks are
also added on the code path being invoked on some ALTER
commands, such as StoreAttrDefault, storeOperators or
storeProcedures. These routines insert / delete subsidiary
objects of others, but the "main" object is not touched in this
code path. So, it makes hard to inform what was updated in
this operation without these additional OAT_POST_CREATE.

Regarding to sepgsql portion, I added logic to check "setattr"
permission of only "main" object to simplify the patch.
Later, I will also add logic for subsidiary objects like triggers
or rules towards relation.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Sorry, I missed the attached version.
Please use this revision.

2012/11/15 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> The attached patch adds a new event type of object_access_hook;
> named OAT_POST_ALTER. As literal, it allows extensions to catch
> controls just after system catalogs are updated.
> It also adds sepgsql permission check capability on some ALTER
> commands, but not any.
>
> The hooks are designed to locate between heap update's and next
> command-counter increment, to allow extensions to reference
> both of older and newer version of catalog entry.
> Unless CCI is called, we can reference older one with SnapshotNow,
> and newer one with SnapshotSelf, as attached sepgsql code doing
> to detect namespace / name changes.
> As we discussed before, it is a significant point to pick-up which
> information should be delivered to extensions through the hooks,
> to avoid design too specific for a particular extension's requirement.
> I believe it is the best way to inform which fields were updated on
> the relevant ALTER command.
>
> The OAT_POST_ALTER can take two arguments. The one is
> "is_internal" flag similar to DROP or POST_CREATE hook.
> The other is "auxiliary_id" field to identify a particular entry of
> the catalog that takes a couple of IDs; E.g, pg_db_role_setting
> or pg_inherits.
>
> It might be noticed that some OAT_POST_CREATE hooks are
> also added on the code path being invoked on some ALTER
> commands, such as StoreAttrDefault, storeOperators or
> storeProcedures. These routines insert / delete subsidiary
> objects of others, but the "main" object is not touched in this
> code path. So, it makes hard to inform what was updated in
> this operation without these additional OAT_POST_CREATE.
>
> Regarding to sepgsql portion, I added logic to check "setattr"
> permission of only "main" object to simplify the patch.
> Later, I will also add logic for subsidiary objects like triggers
> or rules towards relation.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai wrote:
> Sorry, I missed the attached version.
> Please use this revision.

All those direct uses of object_access_hook make me think that the
InvokeObjectAccessHook() macro we have is insufficient. Maybe we could
have InvokeObjectAccessHookArg1() so that instead of

+ if (object_access_hook)
+ {
+ ObjectAccessPostCreate pc_arg;
+
+ memset(&pc_arg, 0, sizeof(ObjectAccessPostCreate));
+ pc_arg.is_internal = is_internal;
+ (*object_access_hook)(OAT_POST_CREATE, AttrDefaultRelationId,
+ RelationGetRelid(rel), attnum, (void *)&pc_arg);
+ }

we can have

InvokeObjectAccessHookWithArgs1(OAT_POST_CREATE, AttrDefaultRelationId,
RelationGetRelid(rel), attnum,
ObjectAccessPostCreate, is_internal, is_internal)

which would expand into the above. (Since ObjectAccessPostCreate has
two members, we presumably need InvokeObjectAccessHookWithArgs2 as
well. But that doesn't seem to be used anywhere, so maybe that struct
member is not necessary?)

The second hunk to alter.c does not apply anymore; please rebase.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

2012/11/19 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai wrote:
>> Sorry, I missed the attached version.
>> Please use this revision.
>
> All those direct uses of object_access_hook make me think that the
> InvokeObjectAccessHook() macro we have is insufficient. Maybe we could
> have InvokeObjectAccessHookArg1() so that instead of
>
> + if (object_access_hook)
> + {
> + ObjectAccessPostCreate pc_arg;
> +
> + memset(&pc_arg, 0, sizeof(ObjectAccessPostCreate));
> + pc_arg.is_internal = is_internal;
> + (*object_access_hook)(OAT_POST_CREATE, AttrDefaultRelationId,
> + RelationGetRelid(rel), attnum, (void *)&pc_arg);
> + }
>
> we can have
>
> InvokeObjectAccessHookWithArgs1(OAT_POST_CREATE, AttrDefaultRelationId,
> RelationGetRelid(rel), attnum,
> ObjectAccessPostCreate, is_internal, is_internal)
>
> which would expand into the above. (Since ObjectAccessPostCreate has
> two members, we presumably need InvokeObjectAccessHookWithArgs2 as
> well. But that doesn't seem to be used anywhere, so maybe that struct
> member is not necessary?)
>
I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
than doing all the stuffs with macros. It allows to use local variables, unlike
macros; that has a risk of naming conflict with temporary variable for
ObjectAccessPostCreate.

So, how about to have a following definition for example?

void
InvokePostAlterHookArgs(Oid classId, Oid objectId, int subId,
Oid auxiliaryId, bool is_internal)
{
if (object_access_hook)
{
ObjectAccessPostAlter pa_arg;

memset(&pa_arg, 0, sizeof(ObjectAccessPostAlter));
pa_arg.auxiliary_id = auxiliary_id;
pa_arg.is_internal = is_internal;
(*object_access_hook)(OAT_POST_ALTER,
classId, objectId, subId,
(void *) &pa_arg);
}
}

and

#define InvokePostAlterHook(classId, objectId, subId) \
InvokePostAlterHookArgs(classId, objectId, subId, InvalidOid, false)

for most points to call.

Even if ObjectAccessPostCreate is extended in the future version, it does not
take changes on most of hook invocation points.
We will be also able to have same semantics on OAT_POST_CREATE and
OAT_DROP as well.

> The second hunk to alter.c does not apply anymore; please rebase.
>
OK,

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

Kohei KaiGai wrote:

> I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
> than doing all the stuffs with macros. It allows to use local variables, unlike
> macros; that has a risk of naming conflict with temporary variable for
> ObjectAccessPostCreate.

No objection from me.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

>> The second hunk to alter.c does not apply anymore; please rebase.
>>
> OK,

Oops, I assumed the patch for ALTER RENAME TO reworks. Sorry.

2012/11/20 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
> Kohei KaiGai wrote:
>
>> I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
>> than doing all the stuffs with macros. It allows to use local variables, unlike
>> macros; that has a risk of naming conflict with temporary variable for
>> ObjectAccessPostCreate.
>
> No objection from me.
>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/11/20 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>>> The second hunk to alter.c does not apply anymore; please rebase.
>>>
>> OK,
>
> Oops, I assumed the patch for ALTER RENAME TO reworks. Sorry.
>
> 2012/11/20 Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>:
>> Kohei KaiGai wrote:
>>
>>> I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
>>> than doing all the stuffs with macros. It allows to use local variables, unlike
>>> macros; that has a risk of naming conflict with temporary variable for
>>> ObjectAccessPostCreate.
>>
>> No objection from me.
>>
The attached patches are revised version of the previous one.
Please note that it assumes my reworks patch on ALTER ... RENAME TO
being applied on top of this patch.

The part-1 stuff simply adds wrap-up functions for object_access_hook.
InvokeObjectPostCreateHookArg() and InvokeObjectDropHookArg() replace
existing invocations of hooks with OAT_POST_CREATE and OAT_DROP.
In case of invocation without argument, callers can use their shorten form
defined in objectaccess.h.

The part-2 stuff is a main portion of this patch.
It newly adds OAT_POST_ALTER event type that shall be invoked just
after catalog updates. Extension side can see the identified object with
SnapshotNow for older version and SnapshotSelf for newer version, thus,
it can know which fields were updated around this event.

I still have a few points to be discussed.
* 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.)

* 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.

Any comments please.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Nov 20, 2012 at 8:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
> than doing all the stuffs with macros. It allows to use local variables, unlike
> macros; that has a risk of naming conflict with temporary variable for
> ObjectAccessPostCreate.
>
> So, how about to have a following definition for example?
>
> void
> InvokePostAlterHookArgs(Oid classId, Oid objectId, int subId,
> Oid auxiliaryId, bool is_internal)
> {
> if (object_access_hook)
> {
> ObjectAccessPostAlter pa_arg;
>
> memset(&pa_arg, 0, sizeof(ObjectAccessPostAlter));
> pa_arg.auxiliary_id = auxiliary_id;
> pa_arg.is_internal = is_internal;
> (*object_access_hook)(OAT_POST_ALTER,
> classId, objectId, subId,
> (void *) &pa_arg);
> }
> }
>
> and
>
> #define InvokePostAlterHook(classId, objectId, subId) \
> InvokePostAlterHookArgs(classId, objectId, subId, InvalidOid, false)
>
> for most points to call.

This has the disadvantage of incurring the overhead of a function call
even if (as will typically be the case) there is no object access
hook. I still prefer having the if (object_access_hook) test in the
macro, though of course I have no problem with having the macro call
a function if it's set.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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.
> * 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.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/12/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Nov 20, 2012 at 8:43 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> I'd like to have catalog/objectaccess.c to wrap-up invocation of hooks, rather
>> than doing all the stuffs with macros. It allows to use local variables, unlike
>> macros; that has a risk of naming conflict with temporary variable for
>> ObjectAccessPostCreate.
>>
>> So, how about to have a following definition for example?
>>
>> void
>> InvokePostAlterHookArgs(Oid classId, Oid objectId, int subId,
>> Oid auxiliaryId, bool is_internal)
>> {
>> if (object_access_hook)
>> {
>> ObjectAccessPostAlter pa_arg;
>>
>> memset(&pa_arg, 0, sizeof(ObjectAccessPostAlter));
>> pa_arg.auxiliary_id = auxiliary_id;
>> pa_arg.is_internal = is_internal;
>> (*object_access_hook)(OAT_POST_ALTER,
>> classId, objectId, subId,
>> (void *) &pa_arg);
>> }
>> }
>>
>> and
>>
>> #define InvokePostAlterHook(classId, objectId, subId) \
>> InvokePostAlterHookArgs(classId, objectId, subId, InvalidOid, false)
>>
>> for most points to call.
>
> This has the disadvantage of incurring the overhead of a function call
> even if (as will typically be the case) there is no object access
> hook. I still prefer having the if (object_access_hook) test in the
> macro, though of course I have no problem with having the macro call
> a function if it's set.
>
OK, I'll adjust the macro definitions to reduce empty function calls.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/12/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

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(at)kaigai(dot)gr(dot)jp>:
> 2012/12/3 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sat, Dec 1, 2012 at 2:57 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)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(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Mon, Dec 3, 2012 at 9:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> 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?

I don't really like it - I think POST should mean POST. You can
define some other hook that gets called somewhere else, but after
means after. There are other plausible uses of these hooks besides
sepgsql; for example, logging the completion time of an action.
Putting the hooks in the "wrong" places because that happens to be
more convenient for sepgsql is going to render them useless for any
other purpose. Maybe nobody else will use them anyway, but I'd rather
not render it impossible before we get off the ground.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2012/12/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Mon, Dec 3, 2012 at 9:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> 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?
>
> I don't really like it - I think POST should mean POST. You can
> define some other hook that gets called somewhere else, but after
> means after. There are other plausible uses of these hooks besides
> sepgsql; for example, logging the completion time of an action.
> Putting the hooks in the "wrong" places because that happens to be
> more convenient for sepgsql is going to render them useless for any
> other purpose. Maybe nobody else will use them anyway, but I'd rather
> not render it impossible before we get off the ground.
>
I have to admit "PREP" hook is problematic to determine which information
should be delivered, and which should not, as we had discussed before.
Yes, a hook convenient for sepgsql, might be unconvenient for others.

So, which alternatives do we have? I can list up a few ideas.

1) Putting POST_ALTER hook after existing whole table rewrite.
good: no need to have significant code change
bad: it takes heavy i/o prior to hook invocation

2) Updating reltablespace of pg_class prior to whole table rewrite,
and put POST_ALTER hook just after catalog update.
good: well fit with assumption of POST hook.
bad: it takes significant code changes on table rewriting

3) Using ProcessUtility_hook to track AKTER TABLE ... SET
TABLESPACE as an exception.
good: no need to have significant code change
bad: sepgsql also have to have analyzer of ALTER TABLE
commands.

I think (2) is the best way, if feasible. I need to investigate whether
the relevant code allows to implement catalog updates prior to
whole table rewriting.
However, (1) is also not so bad in a short term, as a first step towards
the idea (2) that takes additional code updates.
Of course, I can implement with (3), but not inclined with this idea.

Is there other idea?

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

2012/12/11 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/12/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Mon, Dec 3, 2012 at 9:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> 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?
>>
>> I don't really like it - I think POST should mean POST. You can
>> define some other hook that gets called somewhere else, but after
>> means after. There are other plausible uses of these hooks besides
>> sepgsql; for example, logging the completion time of an action.
>> Putting the hooks in the "wrong" places because that happens to be
>> more convenient for sepgsql is going to render them useless for any
>> other purpose. Maybe nobody else will use them anyway, but I'd rather
>> not render it impossible before we get off the ground.
>>
> I have to admit "PREP" hook is problematic to determine which information
> should be delivered, and which should not, as we had discussed before.
> Yes, a hook convenient for sepgsql, might be unconvenient for others.
>
> So, which alternatives do we have? I can list up a few ideas.
>
> 1) Putting POST_ALTER hook after existing whole table rewrite.
> good: no need to have significant code change
> bad: it takes heavy i/o prior to hook invocation
>
> 2) Updating reltablespace of pg_class prior to whole table rewrite,
> and put POST_ALTER hook just after catalog update.
> good: well fit with assumption of POST hook.
> bad: it takes significant code changes on table rewriting
>
> 3) Using ProcessUtility_hook to track AKTER TABLE ... SET
> TABLESPACE as an exception.
> good: no need to have significant code change
> bad: sepgsql also have to have analyzer of ALTER TABLE
> commands.
>
> I think (2) is the best way, if feasible. I need to investigate whether
> the relevant code allows to implement catalog updates prior to
> whole table rewriting.
> However, (1) is also not so bad in a short term, as a first step towards
> the idea (2) that takes additional code updates.
> Of course, I can implement with (3), but not inclined with this idea.
>
> Is there other idea?
>
I tried to adjust my patch based on the idea (1).

It does not do anything special on SET TABLESPACE, thus it follows
the manner of "POST_ALTER" command, but it also means permission
checks are applied after (possible) heavy i/o workload.

I'd like to revise the logic of whole table rewrite to move catalog update
prior to actual i/o works, however, it is not scope of this patch.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

I rebased the patch towards the latest master branch, but here is no
functionality changes.

The part-1 patch adds catalog/objectaccess.c to have entrypoints of
object_access_hook, instead of simple macro definition, to simplify
invocations with arguments. It is just a replacement of existing
OAT_POST_CREATE and OAT_DROP invocation with new style,

The part-2 patch adds new OAT_POST_ALTER event type, and
its relevant permission checks on contrib/sepgsql.

I hope part-1 patch at least being committed earlier, because it performs
prerequisite of other two simple patches to add permission checks to
sepgsql, even though I can understand part-2 takes a bit more power
of concentration for reviewing.

Thanks,

2012/12/13 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> 2012/12/11 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
>> 2012/12/11 Robert Haas <robertmhaas(at)gmail(dot)com>:
>>> On Mon, Dec 3, 2012 at 9:59 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>>> 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?
>>>
>>> I don't really like it - I think POST should mean POST. You can
>>> define some other hook that gets called somewhere else, but after
>>> means after. There are other plausible uses of these hooks besides
>>> sepgsql; for example, logging the completion time of an action.
>>> Putting the hooks in the "wrong" places because that happens to be
>>> more convenient for sepgsql is going to render them useless for any
>>> other purpose. Maybe nobody else will use them anyway, but I'd rather
>>> not render it impossible before we get off the ground.
>>>
>> I have to admit "PREP" hook is problematic to determine which information
>> should be delivered, and which should not, as we had discussed before.
>> Yes, a hook convenient for sepgsql, might be unconvenient for others.
>>
>> So, which alternatives do we have? I can list up a few ideas.
>>
>> 1) Putting POST_ALTER hook after existing whole table rewrite.
>> good: no need to have significant code change
>> bad: it takes heavy i/o prior to hook invocation
>>
>> 2) Updating reltablespace of pg_class prior to whole table rewrite,
>> and put POST_ALTER hook just after catalog update.
>> good: well fit with assumption of POST hook.
>> bad: it takes significant code changes on table rewriting
>>
>> 3) Using ProcessUtility_hook to track AKTER TABLE ... SET
>> TABLESPACE as an exception.
>> good: no need to have significant code change
>> bad: sepgsql also have to have analyzer of ALTER TABLE
>> commands.
>>
>> I think (2) is the best way, if feasible. I need to investigate whether
>> the relevant code allows to implement catalog updates prior to
>> whole table rewriting.
>> However, (1) is also not so bad in a short term, as a first step towards
>> the idea (2) that takes additional code updates.
>> Of course, I can implement with (3), but not inclined with this idea.
>>
>> Is there other idea?
>>
> I tried to adjust my patch based on the idea (1).
>
> It does not do anything special on SET TABLESPACE, thus it follows
> the manner of "POST_ALTER" command, but it also means permission
> checks are applied after (possible) heavy i/o workload.
>
> I'd like to revise the logic of whole table rewrite to move catalog update
> prior to actual i/o works, however, it is not scope of this patch.
>
> Thanks,
> --
> KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Sun, Jan 27, 2013 at 1:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The part-1 patch adds catalog/objectaccess.c to have entrypoints of
> object_access_hook, instead of simple macro definition, to simplify
> invocations with arguments. It is just a replacement of existing
> OAT_POST_CREATE and OAT_DROP invocation with new style,

Looks good. Committed.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sun, Jan 27, 2013 at 1:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The part-2 patch adds new OAT_POST_ALTER event type, and
> its relevant permission checks on contrib/sepgsql.

This documentation hunk is unclear:

+ On <xref linkend="sql-createfunction">, <literal>install</> permission
+ will be checked if <literal>leakproof</> attribute was given, not only
+ <literal>create</> on the new function.

Based on previous experience reading your patches, I'm guessing that
what you actually mean is that both things are checked, but the
wording doesn't make that clear. Also, if permissions are now checked
on functions, doesn't the previous sentence need an update?

+ In addition, <literal>add_name</> and <literal>remove_name</> permission
+ will be checked towards relevant schema when we try to rename or set
+ new schema on the altered object.

Suggest: In addition, <literal>remove_name</> and <literal>add_name</>
will be checked on the old and new schemas, respectively, when an
object is moved to a new schema.

+ A few additional checks are applied depending on object types.

For certain object types, additional checks are performed.

+ On <xref linkend="sql-alterfunction">, <literal>install</> permission
+ will be checked if <literal>leakproof</> attribute was turned on, not
+ only <literal>setattr</> on the new function.

This is a near-duplicate of the previous hunk and suffers from the
same awkwardness.

+ * is_internal: TRUE if constraint is constructed unless user's intention

I dunno what this means. What's the difference between an internal
constraint and a non-internal constraint, and why do we need that
distinction? This seems to percolate to a lot of places; I'd rather
not do that without a real good reason.

+ /* XXX - should be checked at caller side */

XXX should be used only for things that really ought to be revisited
and changed. See the wording I used in the just-committed part 1
patch.

+#include "catalog/objectaccess.h"

This is the only hunk in collationcmds.c, hence presumably not needed.

+ /* Post create hook of this access method operator */
+ InvokeObjectPostCreateHook(AccessMethodOperatorRelationId,
+ entryoid, 0);

I suggest uniformly adding a blank line before each of these hunks,
rather than adding it for some and not others. I think, though, that
we could probably ditch the comments throughout. They don't add
anything, really.

@@ -3330,7 +3342,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
Relation rel,
*/
break;
case AT_SetTableSpace: /* SET TABLESPACE */
-
/*
* Nothing to do here; Phase 3 does the work
*/

Spurious whitespace hunk.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Thanks for your reviewing.

2013/3/7 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Sun, Jan 27, 2013 at 1:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The part-2 patch adds new OAT_POST_ALTER event type, and
>> its relevant permission checks on contrib/sepgsql.
>
> This documentation hunk is unclear:
>
> + On <xref linkend="sql-createfunction">, <literal>install</> permission
> + will be checked if <literal>leakproof</> attribute was given, not only
> + <literal>create</> on the new function.
>
> Based on previous experience reading your patches, I'm guessing that
> what you actually mean is that both things are checked, but the
> wording doesn't make that clear. Also, if permissions are now checked
> on functions, doesn't the previous sentence need an update?
>
Your guess is right. When user defines a function with leakproof attribute,
sepgsql checks both of "create" and "install" permissions on the new
function being labeled according to the default security labeling rules.

The previous section introduces the common behavior when user create
a database object, not particular object class. So, it mention about "create"
permission only on creation of object.
On the other hand, the later session introduces special checks depending
on object classes, such as schema objects.
This section says as below on top of the secsion:
| A few additional checks are applied depending on object types.

And, the sentence says "not only <literal>create</>".

Please give me idea to make the sentence not misleading.

> + In addition, <literal>add_name</> and <literal>remove_name</> permission
> + will be checked towards relevant schema when we try to rename or set
> + new schema on the altered object.
>
> Suggest: In addition, <literal>remove_name</> and <literal>add_name</>
> will be checked on the old and new schemas, respectively, when an
> object is moved to a new schema.
>
> + A few additional checks are applied depending on object types.
>
> For certain object types, additional checks are performed.
>
Thanks, I applied it.

> + On <xref linkend="sql-alterfunction">, <literal>install</> permission
> + will be checked if <literal>leakproof</> attribute was turned on, not
> + only <literal>setattr</> on the new function.
>
> This is a near-duplicate of the previous hunk and suffers from the
> same awkwardness.
>
The above section introduces about behavior when user create an object
of particular object classes. Do I revise it to introduce the behavior

> + * is_internal: TRUE if constraint is constructed unless user's intention
>
> I dunno what this means. What's the difference between an internal
> constraint and a non-internal constraint, and why do we need that
> distinction? This seems to percolate to a lot of places; I'd rather
> not do that without a real good reason.
>
"is_internal" is not a property of constraint itself, but reflects the nature
of its invocation context. Unfortunately, some invocation path requires
to handle the event when a constraint is created or altered as internal
one.
For example, make_new_heap() that also calls heap_create_with_catalog()
is called to construct a clone empty relation to rewrite whole table on
some ALTER TABLE command and others. This table creation is purely
internal stuff (in other words, object was constructed because of just
implementation reason). The heap_create_with_catalog() also calls
StoreConstraints() that adds a new constraint with hook invocation.
It is a situation that extension wants to distinct an internal one from
non-internal one.
Otherwise, in case when AT_ReAddConstraint command tries to add
a constraint, it is constructed due to data type changes in primary
ALTER TABLE command, even existing one is internally dropped.

So, it is a reason why I had to add is_internal flag for constraint.

> + /* XXX - should be checked at caller side */
>
> XXX should be used only for things that really ought to be revisited
> and changed. See the wording I used in the just-committed part 1
> patch.
>
OK, I'll fix it.

> +#include "catalog/objectaccess.h"
>
> This is the only hunk in collationcmds.c, hence presumably not needed.
>
> + /* Post create hook of this access method operator */
> + InvokeObjectPostCreateHook(AccessMethodOperatorRelationId,
> + entryoid, 0);
>
> I suggest uniformly adding a blank line before each of these hunks,
> rather than adding it for some and not others. I think, though, that
> we could probably ditch the comments throughout. They don't add
> anything, really.
>
OK, I'll follow the manner. The comment about hook might make sense
in the previous version, but these comments does not introduce something
more than function-name.

> @@ -3330,7 +3342,6 @@ ATExecCmd(List **wqueue, AlteredTableInfo *tab,
> Relation rel,
> */
> break;
> case AT_SetTableSpace: /* SET TABLESPACE */
> -
> /*
> * Nothing to do here; Phase 3 does the work
> */
>
> Spurious whitespace hunk.
>
Sorry, fixed it.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

The attached patch is rebased one towards the latest master.
It newly added a hook being missed in the previous revision at ALTER
EVENT TRIGGER ENABLE / DISABLE, and adjusted argument of
finish_heap_swap() on REFRESH MATERIALIZED VIEW to handle
it as internal operations.

2013/3/8 Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>:
> Thanks for your reviewing.
>
> 2013/3/7 Robert Haas <robertmhaas(at)gmail(dot)com>:
>> On Sun, Jan 27, 2013 at 1:55 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>>> The part-2 patch adds new OAT_POST_ALTER event type, and
>>> its relevant permission checks on contrib/sepgsql.
>>
>> This documentation hunk is unclear:
>>
>> + On <xref linkend="sql-createfunction">, <literal>install</> permission
>> + will be checked if <literal>leakproof</> attribute was given, not only
>> + <literal>create</> on the new function.
>>
>> Based on previous experience reading your patches, I'm guessing that
>> what you actually mean is that both things are checked, but the
>> wording doesn't make that clear. Also, if permissions are now checked
>> on functions, doesn't the previous sentence need an update?
>>
> Your guess is right. When user defines a function with leakproof attribute,
> sepgsql checks both of "create" and "install" permissions on the new
> function being labeled according to the default security labeling rules.
>
> The previous section introduces the common behavior when user create
> a database object, not particular object class. So, it mention about "create"
> permission only on creation of object.
> On the other hand, the later session introduces special checks depending
> on object classes, such as schema objects.
> This section says as below on top of the secsion:
> | A few additional checks are applied depending on object types.
>
> And, the sentence says "not only <literal>create</>".
>
> Please give me idea to make the sentence not misleading.
>
I tried to revise the sentence as below:

+ On <xref linkend="sql-createfunction">, <literal>install</> permission
+ will be checked if <literal>leakproof</> attribute was given, not only
+ <literal>create</> on the new function. This permission will be also
+ checked when user tries to turn on <literal>leakproof</> attribute
+ using <xref linkend="sql-alterfunction"> command, with
+ <literal>setattr</> permission on the function being altered.

It eliminates same explanation around ALTER command.
But I'm not sure whether it gives an impression that only "install"
permission will be checked on leakproof functions, instead of
"create" permission, incorrectly.

>> + * is_internal: TRUE if constraint is constructed unless user's intention
>>
>> I dunno what this means. What's the difference between an internal
>> constraint and a non-internal constraint, and why do we need that
>> distinction? This seems to percolate to a lot of places; I'd rather
>> not do that without a real good reason.
>>
> "is_internal" is not a property of constraint itself, but reflects the nature
> of its invocation context. Unfortunately, some invocation path requires
> to handle the event when a constraint is created or altered as internal
> one.
> For example, make_new_heap() that also calls heap_create_with_catalog()
> is called to construct a clone empty relation to rewrite whole table on
> some ALTER TABLE command and others. This table creation is purely
> internal stuff (in other words, object was constructed because of just
> implementation reason). The heap_create_with_catalog() also calls
> StoreConstraints() that adds a new constraint with hook invocation.
> It is a situation that extension wants to distinct an internal one from
> non-internal one.
> Otherwise, in case when AT_ReAddConstraint command tries to add
> a constraint, it is constructed due to data type changes in primary
> ALTER TABLE command, even existing one is internally dropped.
>
> So, it is a reason why I had to add is_internal flag for constraint.
>
Right now, I don't touch this portion.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Mar 12, 2013 at 11:53 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
> The attached patch is rebased one towards the latest master.
> It newly added a hook being missed in the previous revision at ALTER
> EVENT TRIGGER ENABLE / DISABLE, and adjusted argument of
> finish_heap_swap() on REFRESH MATERIALIZED VIEW to handle
> it as internal operations.

Thanks. I committed the backend portions of this, but not the sepgsql
and related documentation changes yet. I made some improvements to
the comments along the way. I am not entirely happy with the grotty
hack around ALTER COLUMN SET/DROP DEFAULT. Is there a better
alternative? Do we really need a hook there at all?

The one non-cosmetic change I made was to adjust slightly the firing
point in swap_relation_files. It doesn't make any sense to me to
exclude pg_class here, rather arbitrarily, out of all objects in the
system. This does to some degree raise the question more broadly: is
the point just after CatalogUpdateIndexes() the right rule for where
to put this hook? But I've left it the way you have it for now. Part
of me thinks that what you really want here is a pre-alter hook rather
than a post-alter hook...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

2013/3/18 Robert Haas <robertmhaas(at)gmail(dot)com>:
> On Tue, Mar 12, 2013 at 11:53 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> The attached patch is rebased one towards the latest master.
>> It newly added a hook being missed in the previous revision at ALTER
>> EVENT TRIGGER ENABLE / DISABLE, and adjusted argument of
>> finish_heap_swap() on REFRESH MATERIALIZED VIEW to handle
>> it as internal operations.
>
> Thanks. I committed the backend portions of this, but not the sepgsql
> and related documentation changes yet. I made some improvements to
> the comments along the way. I am not entirely happy with the grotty
> hack around ALTER COLUMN SET/DROP DEFAULT. Is there a better
> alternative? Do we really need a hook there at all?
>
Here is no code point to update system catalog around ALTER COLUMN
SET/DROP DEFAULT, except for StoreAttrDefault(). In addition, it is not
easy job for extensions to know that column defaults were modified
without hook towards AttrDefaultRelationId.

ATExecColumnDefault() tries to drop an existing default at first, then
invokes AddRelationNewConstraints() that also invokes StoreAttrDefault().
Isn't it feasible to call RemoveAttrDefault() only when new expression
was given and StoreAttrDefault() insert or update a new expression
depending on existing entry. Of course, relevant entries on pg_depend
has to be managed by itself because we cannot use dependency
mechanism here.
My alternative might not be graceful. :-(

> The one non-cosmetic change I made was to adjust slightly the firing
> point in swap_relation_files. It doesn't make any sense to me to
> exclude pg_class here, rather arbitrarily, out of all objects in the
> system. This does to some degree raise the question more broadly: is
> the point just after CatalogUpdateIndexes() the right rule for where
> to put this hook?
>
We have put OAT_POST_CREATE/_ALTER hook after
CatalogUpdateIndexes() or recordDependencyOn() that should be
usually called after CatalogUpdateIndexes().
Does it make a problem when extension tries to search a newer
version of catalog using SnapshotSelf, without CatalogUpdateIndexes()
doesn't it?

I agree with your adjustment around swap_relation_files().

> But I've left it the way you have it for now. Part
> of me thinks that what you really want here is a pre-alter hook rather
> than a post-alter hook...
>
Yes. It is the reason why I wanted to put a pre-alter hook for this
code path exceptionally.

The attached patch is rebased version of contrib/sepgsql patch.
Even though I added nothing from the previous one, it might make
sense to add checks for the cases when user altered external
properties of tables (such as triggers, attribute-default, ...).
How about your opinion? It is middle of March now.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>

On Tue, Mar 19, 2013 at 9:28 AM, Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> wrote:
>> But I've left it the way you have it for now. Part
>> of me thinks that what you really want here is a pre-alter hook rather
>> than a post-alter hook...
>>
> Yes. It is the reason why I wanted to put a pre-alter hook for this
> code path exceptionally.
>
> The attached patch is rebased version of contrib/sepgsql patch.
> Even though I added nothing from the previous one, it might make
> sense to add checks for the cases when user altered external
> properties of tables (such as triggers, attribute-default, ...).
> How about your opinion? It is middle of March now.

It might make sense to rework this further in a future release, but I
don't think we can do that now.

Anyhow, I've committed the rest of this patch. Sorry that took so long.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company