Re: deparsing utility commands - Mailing list pgsql-hackers

Here's a cleaned up version of this patch; I threw together a very quick
test module, and updated a conflicting OID.  As far as I can tell, I'm
only missing the documentation updates before this is push-able.

One change to note is that the AlterTable support used to ignore
commands that didn't match the OID as set by
EventTriggerAlterTableRelid(); the comment there said that the point was
to avoid collecting the same commands to child tables as recursion
occured in execution.  I think that would be imposing such a decision;
perhaps some users of this infrastructure want to know about the
operations as they happen on child tables too.  With this definition it
is up to the user module to ignore the duplicates.

Thanks for your review.  In reply to your comments:

Amit Kapila wrote:

> 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.

True. Fixed.

> 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?

Acrually, I figured that this is not at issue.  When a subxact is rolled
back, the whole currentEventTriggerState thing is reverted to a previous
item in the stack; if an event trigger is fired by ALTER TABLE, and the
resulting function invokes ALTER TABLE again, they collect their
commands in separate state elements, so there is never a conflict.
I can't think of any situation in which one event trigger function adds
elements to the list of the calling command.


> 3.
> + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> + * Portions Copyright (c) 1996-2012, PostgreSQL Global Development Group
>
> Copyright notice years should be same.

Yeah, fixed.

> 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;)?

AFAICT that's copied by memcpy.

> 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?

Yeah, I considered that as I wrote the code but dropped it out of
laziness.  I have done so now.

> 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?

All paths that go through AlterTableInternal() have the Oid set by that
function.

> 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.

Hmm, most of the reason I picked these names is the EventTrigger prefix.

> 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?

None.  Removed that.

> Regarding Patch 0002-changes-to-core-to-support-the-deparse-extension:

I decided against committing 0002 patch for now, as it's mostly code
that would not have any callers in core.  If needed, in BDR we can just
duplicate the ruleutils.c code ... it's not a huge problem.  We can
reconsider later.


Note: for BDR, the JSON representation is pointless complication.  All
it wants is the "normalized" command string unchanged, i.e. add schema
names everywhere and such.  We came up with the JSON representation as a
way to appease reviewers here who wanted truly generalized access to the
parse tree.  As far as BDR is concerned, we could just as well remove
all the JSON stuff and go back to some simpler representation ...  The
only reason to keep it is the expectation that someday (9.6 hopefully)
we will include the whole thing in core.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services