Re: [v9.3] OAT_POST_ALTER object access hooks

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>