Re: [v9.3] OAT_POST_ALTER object access hooks

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?

+    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