Re: deparsing utility commands

On Thu, Apr 9, 2015 at 9:44 PM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)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