Re: Add CREATE support to event triggers

On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Here's a new version of this series. The main change is that I've
> changed deparse_utility.c to generate JSON, and the code that was in
> commands/event_trigger.c to decode that JSON, so that it uses the new
> Jsonb API instead. In addition, I've moved the new code that was in
> commands/event_trigger.c to utils/adt/ddl_json.c. (The only entry point
> of the new file is the SQL-callable pg_event_trigger_expand_command()
> function, and its purpose is to expand a JSON object emitted by the
> deparse_utility.c code back into a plain text SQL command.)
> I have also cleaned up the code per comments from Michael Paquier and
> Andres Freund:
>
> * the GRANT support for event triggers now correctly ignores global
> objects.
>
> * COMMENT ON .. IS NULL no longer causes a crash
Why would this not fail? Patch 3 in this set is identical to the last
one. I tested that and it worked well...

> * renameatt() and ExecRenameStmt are consistent in returning the
> objSubId as an "int" (not int32). This is what is used as objectSubId
> in ObjectAddress, which is what we're using this value for.

In patch 1, ExecRenameStmt returns objsubid for an attribute name when
rename is done on an attribute. Now could you clarify why we skip this
list of objects even if their sub-object ID is available with
address.objectSubId?
case OBJECT_AGGREGATE:
case OBJECT_COLLATION:
case OBJECT_CONVERSION:
case OBJECT_EVENT_TRIGGER:
case OBJECT_FDW:
case OBJECT_FOREIGN_SERVER:
case OBJECT_FUNCTION:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
case OBJECT_LANGUAGE:
case OBJECT_TSCONFIGURATION:
case OBJECT_TSDICTIONARY:
case OBJECT_TSPARSER:
case OBJECT_TSTEMPLATE:

Patch 2 fails on make check for the tests privileges and foreign_data
by showing up double amount warnings for some object types:
*** /Users/ioltas/git/postgres/src/test/regress/expected/privileges.out
Fri Oct 10 14:34:10 2014
--- /Users/ioltas/git/postgres/src/test/regress/results/privileges.out
Thu Oct 16 15:47:42 2014
***************
*** 118,123 ****
--- 118,124 ----
ERROR: permission denied for relation atest2
GRANT ALL ON atest1 TO PUBLIC; -- fail
WARNING: no privileges were granted for "atest1"
+ WARNING: no privileges were granted for "atest1"
-- checks in subquery, both ok
SELECT * FROM atest1 WHERE ( b IN ( SELECT col1 FROM atest2 ) );
a | b
EventTriggerSupportsGrantObjectType is fine to remove
ACL_OBJECT_DATABASE and ACL_OBJECT_TABLESPACE from the list of
supported objects. That's as well in line with the current firing
matrix. I think that it would be appropriate to add a comment on top
of this function.

Patch 3 looks good.
Regards,
--
Michael