Re: Add CREATE support to event triggers

On Tue, Aug 26, 2014 at 8:10 AM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> Well, I like the patch series for what it counts as long as you can
> submit it as such. That's far easier to test and certainly helps in
> spotting issues when kicking different code paths.

So, for patch 2, which is a cosmetic change for
pg_event_trigger_dropped_objects:
=# select pg_event_trigger_dropped_objects();
ERROR: 0A000: pg_event_trigger_dropped_objects can only be called in
a sql_drop event trigger function
LOCATION: pg_event_trigger_dropped_objects, event_trigger.c:1220
This drops "()" from the error message with the function name. I guess
that this is fine. But PG_FUNCNAME_MACRO is used nowhere except
elog.h, and can as well be NULL. So if that's really necessary
shouldn't we use FunctionCallInfo instead. It is not as well not that
bad to hardcode the function name in the error message as well IMO.

For patch 5:
+1 for this move. When working on Postgres-XC a couple of years back I
wondered why this distinction was not made. Wouldn't it make sense to
move as well the declaration of quote_all_identifiers to ruleutils.h.
That's a GUC and moving it out of builtins.h would make sense IMO.

Patch 8 needs a rebase (patch independent on 1~6 it seems):
1 out of 57 hunks FAILED -- saving rejects to file
src/backend/commands/tablecmds.c.rej
(Stripping trailing CRs from patch.)

Patch 9:
1) It needs a rebase, AlterTableMoveAllStmt has been renamed to
AlterTableMoveAllStmt by commit 3c4cf08
2) Table summarizing event trigger firing needs to be updated with the
new command supported (src/sgml/event-trigger.sgml)

Patch 10, similar problems as patch 9:
1) Needs a rebase
2) table summarizing supported commands should be updated.
You could directly group patches 9 and 10 in the final commit IMO.
GRANT/REVOKE would also be the first command that would be supported
by event triggers that is not of the type CREATE/DROP/ALTER, hence
once it is rebased I would like to do some testing with it (same with
patch 9 btw) and see how it reacts with the event sql_drop
particularly (it should error out but still).

Patch 11: applies with some hunks.
So... This patch introduces an operation performing doing reverse
engineering of format_type_internal... I think that this would be more
adapted with a couple of new routines in lsyscache.[c|h] instead:
- One for the type name
- One for typmodout
- One for is_array
- One for its namespace
TBH, I wanted those routines a couple of times when working on XC and
finished coding them at the end, but in XC only :)

Patch 12: patch applies correctly.
Form_pg_sequence is already exposed in sequence.h even if it is only
used in sequence.c, so yes it seems to be the correct way to do it
here assuming that we need this data to rebuild a DDL. Why is
ACL_UPDATE needed when checking permissions? This new routine only
reads the values and does not update it. And a confirmation: ACL_USAGE
is used to make this routine usable for PL languages in this case,
right?
I think that you should mention at the top of get_sequence_values that
it returns a palloc'd result, and that it is the responsibility of
caller to free it.

That's all I have for now.
Regards,
--
Michael