Re: [v9.3] OAT_POST_ALTER object access hooks

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>