Re: Add CREATE support to event triggers - Mailing list pgsql-hackers

On Wed, Aug 27, 2014 at 1:10 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Tue, Aug 26, 2014 at 8:10 AM, Michael Paquier
> <michael.paquier@gmail.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.

And a last one before lunch, closing the review for all the basic things...
Patch 13: Could you explain why this is necessary?
+extern PGDLLIMPORT bool creating_extension;
It may make sense by looking at the core features (then why isn't it
with the core features?), but I am trying to review the patches in
order.
-- 
Michael