On Tue, Aug 26, 2014 at 8:10 AM, Michael Paquier <michael.paqu...@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. That's all I have for now. Regards, -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
