Michael Paquier 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.
You're right. Dropped this patch.
> 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.
I had it that way initially, but decided against it, because it creates
too much churn; there are way too many .c files that depend on the
quoting stuff. I don't want to repeat the htup_details.h disaster ..
> 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.)
Fixed.
> Patch 9:
> 1) It needs a rebase, AlterTableMoveAllStmt has been renamed to
> AlterTableMoveAllStmt by commit 3c4cf08
Fixed.
> 2) Table summarizing event trigger firing needs to be updated with the
> new command supported (src/sgml/event-trigger.sgml)
Will add.
> Patch 10, similar problems as patch 9:
> 1) Needs a rebase
Done.
> 2) table summarizing supported commands should be updated.
Will add.
> 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).
Actually, I think I would commit most of this stuff not in the same way
I'm submitting; I've split it up to ease review only.
> 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 :)
Not real sure about this. Being able to do the whole thing in one fell
swoop seems more sensible to me. Did you need each of those things
separately, or together?
> 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?
Hm, I guess I just copied it from some other routine. Will fix.
> 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.
Will add.
> 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.
It's needed in MSVC build; I merged it with the next patch, which
actually uses it. The point is to detect whether a command is being
executed as part of an extension; we set a flag in the output for this.
In BDR, we emit only the CREATE EXTENSION command, not the individual
commands that the extension creates. I would guess that most other
replication systems will want to do the same.
I attach a rebased version of this. The patch structure is a bit
different; patch 4 (which used to be 14) introduces the infrastructure
for DDL deparsing to JSON, but no users of it; patch 5 introduces the
first few commands that actually produce deparse output.
I will add this patch series to the next commitfest. Thanks for
reviewing.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services