On Thu, Apr 9, 2015 at 9:44 PM, Alvaro Herrera <alvhe...@2ndquadrant.com> wrote: > > Alvaro Herrera wrote: > > Executive summary: > > > > There is now a CommandDeparse_hook; > > deparse_utility_command is provided as an extension, intended for 9.6; > > rest of patch would be pushed to 9.5. > > Actually here's another approach I like better: use a new pseudotype, > pg_ddl_command, which internally is just a pointer to a CollectedCommand > struct. We cannot give access to the pointer directly in SQL, so much > like type internal or pg_node_tree the in/out functions should just > error out. But the type can be returned as a column in > pg_event_trigger_ddl_command. An extension can create a function that > receives that type as argument, and return a different representation of > the command; the third patch creates a function ddl_deparse_to_json() > which does that. > > You can have as many extensions as you want, and your event triggers can > use the column as many times as necessary. This removes the limitation > of the previous approach that you could only have a single extension > providing a CommandDeparse_hook function. >
Few Comments/Questions regrading first 2 patches: Regarding Patch 0001-deparse-infrastructure-needed-for-command-deparsing 1. + * Currently, sql_drop, table_rewrite, ddL_command_end events are the only /ddL_command_end/ddl_command_end 'L' should be in lower case. 2. + * FIXME this API isn't considering the possibility that a xact/subxact is + * aborted partway through. Probably it's best to add an + * AtEOSubXact_EventTriggers() to fix this. + */ +void +EventTriggerAlterTableEnd(void) { .. } Wouldn't the same fix be required for RollbackToSavePoint case as well? Do you intend to fix this in separate patch? 3. +/*------------------------------------------------------------------------- + * + * aclchk_internal.h + * + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group +/*------------------------------------------------------------------------- + * + * deparse_utility.h + * + * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group Copyright notice years should be same. 4. + /* + * copying the node is moderately challenging ... should we consider + * changing InternalGrant into a full-fledged node instead? + */ + icopy = palloc(sizeof(InternalGrant)); + memcpy(icopy, istmt, sizeof(InternalGrant)); + icopy->objects = list_copy(istmt->objects); Don't we need to copy (AclMode privileges;)? 5. -static void AlterOpFamilyAdd(List *opfamilyname, Oid amoid, Oid opfamilyoid, +static void AlterOpFamilyAdd(AlterOpFamilyStmt *stmt, + List *opfamilyname, Oid amoid, Oid opfamilyoid, int maxOpNumber, int maxProcNumber, List *items); -static void AlterOpFamilyDrop(List *opfamilyname, Oid amoid, Oid opfamilyoid, +static void AlterOpFamilyDrop(AlterOpFamilyStmt *stmt, + List *opfamilyname, Oid amoid, Oid opfamilyoid, int maxOpNumber, int maxProcNumber, List *items); Now that both the above functions have parameter AlterOpFamilyStmt *stmt, so can't we get rid of second parameter List *opfamilyname as that is part of structure AlterOpFamilyStmt? 6. @@ -1175,204 +1229,258 @@ ProcessUtilitySlow(Node *parsetree, .. + EventTriggerAlterTableStart(parsetree); + address = + DefineIndex(relid, /* OID of heap relation */ + stmt, + InvalidOid, /* no predefined OID */ + false, /* is_alter_table */ + true, /* check_rights */ + false, /* skip_build */ + false); /* quiet */ + /* + * Add the CREATE INDEX node itself to stash right away; if + * there were any commands stashed in the ALTER TABLE code, + * we need them to appear after this one. + */ + EventTriggerCollectSimpleCommand(address, secondaryObject, + parsetree); + commandCollected = true; + EventTriggerAlterTableEnd(); Is there a reason why EventTriggerAlterTableRelid() is not called after EventTriggerAlterTableStart() in this flow? 7. +void +EventTriggerInhibitCommandCollection(void) +void +EventTriggerUndoInhibitCommandCollection(void) These function names are understandable, some alternative names could be InhibitEventTriggerCommandCollection(), PreventEventTriggerCommandCollection(), or ProhibitEventTriggerCommandCollection() if you prefer anyone of these over others. 8. case T_CreateOpClassStmt: - DefineOpClass((CreateOpClassStmt *) parsetree); + address = DefineOpClass((CreateOpClassStmt *) parsetree); + /* command is stashed in DefineOpClass */ + commandCollected = true; Is there a need to remember address if command is already collected? Regarding Patch 0002-changes-to-core-to-support-the-deparse-extension: 9. +char * +format_procedure_args(Oid procedure_oid, bool force_qualify) Is there a reason of exposing new API instead of using existing API format_procedure_parts()? As far as I can see the only difference is that in new API, you have control of whether to schema_qualify it, is that the main reason of exposing new API? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com