Re: Add CREATE support to event triggers

On Fri, Sep 26, 2014 at 6:59 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Actually here's a different split of these patches, which I hope makes
> more sense. My intention here is that patches 0001 to 0004 are simple
> changes that can be pushed right away; they are not directly related to
> the return-creation-command feature. Patches 0005 to 0027 implement
> that feature incrementally. You can see in patch 0005 the DDL commands
> that are still not implemented in deparse (they are the ones that have
> an elog(ERROR) rather than a "command = NULL"). Patch 0006 adds calls
> in ProcessUtilitySlow() to each command, so that the object(s) being
> touched are added to the event trigger command stash.
>
> Patches from 0007 to 0027 (excepting patch 0017) implement one or a
> small number of commands in deparse. Patch 0017 is necessary
> infrastructure in ALTER TABLE to support deparsing that one.
>
> My intention with the later patches is that they would all be pushed as
> a single commit, i.e. the deparse support would be implemented for all
> commands in a fell swoop rather than piecemeal -- except possibly patch
> 0017 (the ALTER TABLE infrastructure). I split them up only for ease of
> review. Of course, before pushing we (I) need to implement deparsing
> for all the remaining commands.

Thanks for the update. This stuff is still on my TODO list and I was
planning to look at it at some extent today btw :) Andres has already
sent some comments (20140925235724(dot)GH16581(at)awork2(dot)anarazel(dot)de) though,
I'll try to not overlap with what has already been mentioned.

Patch 1: I still like this patch as it gives a clear separation of the
built-in functions and the sub-functions of ruleutils.c that are
completely independent. Have you considered adding the external
declaration of quote_all_identifiers as well? It is true that this
impacts extensions (some of my stuff as well), but my point is to bite
the bullet and make the separation cleaner between builtins.h and
ruleutils.h. Attached is a patch that can be applied on top of patch 1
doing so... Feel free to discard for the potential breakage this would
create though.

Patch 2:
1) Declarations of renameatt are inconsistent:
@tablecmds.c:
-renameatt(RenameStmt *stmt)
+renameatt(RenameStmt *stmt, int32 *objsubid)
@tablecmds.h:
-extern Oid renameatt(RenameStmt *stmt);
+extern Oid renameatt(RenameStmt *stmt, int *attnum);
Looking at this code, for correctness renameatt should use everywhere
"int *attnum" and ExecRenameStmt should use "int32 *subobjid".
2) Just a smallish picky formatting remark, I would have done that on
a single line:
+ attnum =
+ renameatt_internal(relid,
3) in ExecRenameStmt, I think that you should set subobjid for
aggregate, collations, fdw, etc. There is an access to ObjectAddress
so that's easy to set using address.objectSubId.
4) This comment is misleading, as for an attribute what is returned is
actually an attribute number:
+ * Return value is the OID of the renamed object. The objectSubId, if any,
+ * is returned in objsubid.
*/
5) Perhaps it is worth mentioning at the top of renameatt that the
attribute number is returned as well?

Patch 3: Looks fine, a bit of testing is showing up that this works as expected:
=# CREATE ROLE foo;
CREATE ROLE
=# CREATE TABLE aa (a int);
CREATE TABLE
=# CREATE OR REPLACE FUNCTION abort_grant()
RETURNS event_trigger AS $$
BEGIN
RAISE NOTICE 'Event: %', tg_event;
RAISE EXCEPTION 'Execution of command % forbidden', tg_tag;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_start
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# GRANT SELECT ON aa TO foo;
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command GRANT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
=# DROP EVENT TRIGGER abort_grant_trigger;
DROP EVENT TRIGGER
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_end
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# REVOKE SELECT ON aa FROM foo;
NOTICE: 00000: Event: ddl_command_end
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command REVOKE forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068

Patch 4: Shouldn't int32 be used instead of uint32 in the declaration
of CommentObject? And yes, adding support for only ddl_command_start
and ddl_command_end is enough. Let's not play with dropped objects in
this area... Similarly to the test above:
=# comment on table aa is 'test';
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command COMMENT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
Regards,
--
Michael