Re: [v9.3] OAT_POST_ALTER object access hooks - Mailing list pgsql-hackers

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@kaigai.gr.jp>:
> Thanks for your reviewing.
>
> 2013/3/7 Robert Haas <robertmhaas@gmail.com>:
>> On Sun, Jan 27, 2013 at 1:55 AM, Kohei KaiGai <kaigai@kaigai.gr.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@kaigai.gr.jp>