Re: deparsing utility commands

Andres Freund wrote:

Here's a new set of patches. Several fixes have been merged into the
patches themselves, before rebasing (in particular, some more commands,
such as DefineRelation and AlterDomain routines are now returning
ObjectAddress rather than OID). I have accumulated some newer fixes as
fixup commits at the end of the series. I have handled some of your
comments that needed code changes; more changes pending. Additionally:

> On 2015-02-15 01:48:15 -0300, Alvaro Herrera wrote:
> > One line of defense which I just tought about is that instead of
> > sprinkling EventTriggerStashCommand() calls all over ProcessUtilitySlow,
> > we should add only one at the bottom.
>
> Doesn't sound like a bad idea, but I'm not sure whether it's realistic
> given that some commands do multiple EventTriggerStashCommand()s?

What the new code does is to have a boolean flag, default false, and
only auto-stash commands if the flag is false. Commands that process
stuff differently need to set the flag to true (meaning "already
stashed") and then the auto-stash step is skipped.

> > 1. ruleutils.c seems to deparse temp schemas as pg_temp_X instead of
> > just pg_temp; so some commands like CREATE VIEW..AS SELECT FROM temp_table
> > fail in an ugly way. Maybe the solution is to tell ruleutils that the
> > temp schema is always visible; or maybe the solution is to tell it that
> > it's spelled pg_temp instead.
>
> Hm. I'm not immediately following why that's happening for deparsing but
> not for the normal get_viewdef stuff?

Haven't researched this closely yet, but I think it's a minor bug.

> > @@ -1060,6 +1066,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
> > case OBJECT_CAST:
> > case OBJECT_COLUMN:
> > case OBJECT_COLLATION:
> > + case OBJECT_COMPOSITE:
> > case OBJECT_CONVERSION:
> > case OBJECT_DEFAULT:
> > case OBJECT_DOMAIN:
> > @@ -1088,6 +1095,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
> > case OBJECT_TSPARSER:
> > case OBJECT_TSTEMPLATE:
> > case OBJECT_TYPE:
> > + case OBJECT_USER_MAPPING:
> > case OBJECT_VIEW:
> > return true;
> > }
>
> Should those two be broken out and just committed?

No, those two symbols are created by the infrastructure patch because
they are not required by the current code.

> > @@ -1193,14 +1201,6 @@ EventTriggerBeginCompleteQuery(void)
> > EventTriggerQueryState *state;
> > MemoryContext cxt;
> >
> > - /*
> > - * Currently, sql_drop and table_rewrite events are the only reason to
> > - * have event trigger state at all; so if there are none, don't install
> > - * one.
> > - */
> > - if (!trackDroppedObjectsNeeded())
> > - return false;
> > -
> > cxt = AllocSetContextCreate(TopMemoryContext,
> > "event trigger state",
> > ALLOCSET_DEFAULT_MINSIZE,
> > @@ -1211,7 +1211,7 @@ EventTriggerBeginCompleteQuery(void)
> > slist_init(&(state->SQLDropList));
> > state->in_sql_drop = false;
> > state->table_rewrite_oid = InvalidOid;
> > -
> > + state->stash = NIL;
> > state->previous = currentEventTriggerState;
> > currentEventTriggerState = state;
>
> Hm. I'm not clear why we don't check for other types of event triggers
> in EventTriggerBeginCompleteQuery and skip the work otherwise. If we
> just enable them all the time - which imo is ok given how they're split
> of in the normal process utility - we should remove the boolean return
> value from Begin/CompleteQuery and drop
> * Note: it's an error to call this routine if EventTriggerBeginCompleteQuery
> * returned false previously.
> from EndCompleteQuery.

I think this merits some more checking, to avoid event trigger overhead
when there are no triggers that are going to use this mechanism.

> > + currentEventTriggerState->stash = lappend(currentEventTriggerState->stash,
> > + stashed);

> It's a bit wierd that the drop list is managed using a slist, but the
> stash isn't...

Yeah, I think I'll change the stash to an slist too. I used List for

> > + /*
> > + * Obtain schema name, if any ("pg_temp" if a temp object)
> > + */
> > + if (is_objectclass_supported(addr.classId))
> > + {
> > + AttrNumber nspAttnum;
> > +
> > + nspAttnum = get_object_attnum_namespace(addr.classId);
> > + if (nspAttnum != InvalidAttrNumber)
> > + {
> > + Relation catalog;
> > + HeapTuple objtup;
> > + Oid schema_oid;
> > + bool isnull;
> > +
> > + catalog = heap_open(addr.classId, AccessShareLock);
> > + objtup = get_catalog_object_by_oid(catalog,
> > + addr.objectId);
> > + if (!HeapTupleIsValid(objtup))
> > + elog(ERROR, "cache lookup failed for object %u/%u",
> > + addr.classId, addr.objectId);
> > + schema_oid = heap_getattr(objtup, nspAttnum,
> > + RelationGetDescr(catalog), &isnull);
> > + if (isnull)
> > + elog(ERROR, "invalid null namespace in object %u/%u/%d",
> > + addr.classId, addr.objectId, addr.objectSubId);
> > + if (isAnyTempNamespace(schema_oid))
> > + schema = pstrdup("pg_temp");
> > + else
> > + schema = get_namespace_name(schema_oid);
> > +
> > + heap_close(catalog, AccessShareLock);
> > + }
> > + }
>
> Hm. How do we get here for !is_objectclass_supported()?

The whole idea here is that object classes that are not "supported" (by
the objectaddress.c generic stuff) then it's not an object type that
lives within a schema. The main point of this block is to obtain schema
name, so objects that don't live in schemas obviously continue to have
schema = NULL.

> > +static ObjTree *
> > +new_objtree_VA(char *fmt, int numobjs,...)
> > +{

> Would look nicer if boolval et al weren't declared - I'd just pass the
> va_arg() return value directly to new_*_object().
>
> Attached as 0001.
>
> Additionally I find the separate handling of ObjTypeNull not so
> nice. 0002.

Both nice, thanks. Folded into infrastructure commit.

> > +/*
> > + * A helper routine to setup %{}T elements.
> > + */
> > +static ObjTree *
> > +new_objtree_for_type(Oid typeId, int32 typmod)
> > +{
>
> > + append_bool_object(typeParam, "is_array", is_array);
>
> Maybe name that one typarray?

Done. I also fixed an issue with timestamp/timestamptz/intervals when
there were arrays of those, which I noticed a few days ago while fooling
with the test framework.

> > + * Handle deparsing of simple commands.
> > + *
> > + * This function contains a large switch that mirrors that in
> > + * ProcessUtilitySlow. All cases covered there should also be covered here.
> > + */
> > +static ObjTree *
> > +deparse_simple_command(StashedCommand *cmd)
> > +{

> It is not clear to me why some commands error out and other don't. It
> makes sense to me to error out for things like GrantStmt that shouldn't
> end up here, but... I guess that's just an artifact of the patch series
> because you add the handling for the non elog()ing commands later?

Yeah, exactly. In this one I added elog(ERROR) to all cases in the
infrastructure patch. Note that the intention is that all command types
are supported by the time we push, and therefore this:

> I think this should use ereport() in some cases if we're not going to
> support some commands for now.

is unnecessary.

> > + /*
> > + * Many routines underlying this one will invoke ruleutils.c functionality
> > + * in order to obtain deparsed versions of expressions. In such results,
> > + * we want all object names to be qualified, so that results are "portable"
> > + * to environments with different search_path settings. Rather than inject
> > + * what would be repetitive calls to override search path all over the
> > + * place, we do it centrally here.
> > + */
> > + overridePath = GetOverrideSearchPath(CurrentMemoryContext);
> > + overridePath->schemas = NIL;
> > + overridePath->addCatalog = false;
> > + overridePath->addTemp = false;
> > + PushOverrideSearchPath(overridePath);
>
> Ah, the 'addTemp = false' probably is the answer to my question above
> about view deparsing adding a fully qualified pg_temp...

Not real sure about that. Might be. But I vaguely recall I needed
isTemp=false for something else.

>
> > +/*------
> > + * Returns a formatted string from a JSON object.
> > + *
> > + * The starting point is the element named "fmt" (which must be a string).
> > + * This format string may contain zero or more %-escapes, which consist of an
> > + * element name enclosed in { }, possibly followed by a conversion modifier,
> > + * followed by a conversion specifier. Possible conversion specifiers are:
> > + *
> > + * % expand to a literal %.
> > + * I expand as a single, non-qualified identifier
> > + * D expand as a possibly-qualified identifier
> > + * T expand as a type name
> > + * O expand as an operator name
> > + * L expand as a string literal (quote using single quotes)
> > + * s expand as a simple string (no quoting)
> > + * n expand as a simple number (no quoting)
> > + *
> > + * The element name may have an optional separator specification preceded
> > + * by a colon. Its presence indicates that the element is expected to be
> > + * an array; the specified separator is used to join the array elements.
> > + *------
>
> I think this documentation should at least be referred to from
> comments in deparse_utility.c. I was wondering where it is.

I'll add a reference.

> > diff --git a/src/backend/utils/adt/jsonb.c b/src/backend/utils/adt/jsonb.c
> > index 6fbe283..a842ef2 100644
> > --- a/src/backend/utils/adt/jsonb.c
> > +++ b/src/backend/utils/adt/jsonb.c
> > @@ -416,7 +416,7 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
> > {
> > bool first = true;
> > JsonbIterator *it;
> > - int type = 0;
> > + JsonbIteratorToken type = WJB_DONE;
> > JsonbValue v;
> > int level = 0;
> > bool redo_switch = false;
> > @@ -498,7 +498,7 @@ JsonbToCString(StringInfo out, JsonbContainer *in, int estimated_len)
> > first = false;
> > break;
> > default:
> > - elog(ERROR, "unknown flag of jsonb iterator");
> > + elog(ERROR, "unknown jsonb iterator token type");
> > }
> > }
>
> Huh?

Apologies -- should be somewhere separate, I guess.

> I think this infrastructure pretty desperately requires some higher
> level overview. Like how are we going from the node tree, to the object
> tree, to jsonb, to actual DDL. I think I understand, but it took me a
> while. Doesn't have to be long and super detailed...

Will add.

> > From dcb353c8c9bd93778a62719ff8bf32b9a419e16d Mon Sep 17 00:00:00 2001
> > From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> Date: Thu, 25 Sep 2014
> > 15:54:00 -0300 Subject: [PATCH 09/42] deparse: Support CREATE TYPE AS
>
> > +static ObjTree *
> > +deparse_ColumnDef(Relation relation, List *dpcontext, bool composite,
> > + ColumnDef *coldef)
> > +{
>
> > + if (!composite)
> > + {
> > + /*
> > + * Emit a NOT NULL declaration if necessary. Note that we cannot trust
> > + * pg_attribute.attnotnull here, because that bit is also set when
> > + * primary keys are specified; and we must not emit a NOT NULL
> > + * constraint in that case, unless explicitely specified. Therefore,
> > + * we scan the list of constraints attached to this column to determine
> > + * whether we need to emit anything.
> > + * (Fortunately, NOT NULL constraints cannot be table constraints.)
> > + */
>
> Is the primary key bit really a problem? To me it sounds like it's
> actually good to retain a NOT NULL in that case. Note that pg_dump
> actually *does* emit a NOT NULL here, even if you drop the primary
> key. Since the column behaves as NOT NULL and is dumped as such it seems
> like a good idea to fully treat it as such.

I think pg_dump is wrong -- as far as I recall, the standard wants you
to drop the NOT NULL bit if you drop the primary key, which would
probably not happen if you just add NOT NULL blindly. My patch to
catalog not null constraints tries to handle this .. but of course,
that's still dormant.

> > + list = NIL;
> > + if (node->deferrable)
> > + list = lappend(list,
> > + new_string_object("DEFERRABLE"));
> > + if (node->initdeferred)
> > + list = lappend(list,
> > + new_string_object("INITIALLY DEFERRED"));
>
> I mildly wonder if DEFERRABLE/INITIALLY DEFERRED shouldn't instead have
> an 'is_present' style representation.

Yeah, good point. Many other clauses in various commands probably need
the same thing.

> > +/*
> > + * deparse_CreateStmt
> > + * Deparse a CreateStmt (CREATE TABLE)
>
> I really wish CreateStmt were named CreateTableStmt. I don't know how
> often that tripped me over, let alone everyone. Yes, that's an unrelated
> rant ;)

Agreed.

> > + * Given a table OID and the parsetree that created it, return an ObjTree
> > + * representing the creation command.
> > + */
> > +static ObjTree *
> > +deparse_CreateStmt(Oid objectId, Node *parsetree)
> > +{
> ...
> > + tmp = new_objtree_VA("TABLESPACE %{tablespace}I", 0);
> > + if (node->tablespacename)
> > + append_string_object(tmp, "tablespace", node->tablespacename);
> > + else
> > + {
> > + append_null_object(tmp, "tablespace");
> > + append_bool_object(tmp, "present", false);
> > + }
> > + append_object_object(createStmt, "tablespace", tmp);
>
> Hm. I had not thought about that before, but given that
> default_tablespace exists, we should maybe somehow indicate which one
> was chosen?

Hmm, yeah, good point. We do likewise for WITH/WITHOUT OIDS, for
instance, so that's fair. Will add.

> > + if (!ownedby)
> > + /* XXX this shouldn't happen ... */
> > + ownedby = new_objtree_VA("OWNED BY %{owner}D",
> > + 3,
> > + "clause", ObjTypeString, "owned",
> > + "owner", ObjTypeNull,
> > + "present", ObjTypeBool, false);
>
> Why shouldn't this happen? Free standing sequences are perfectly normal?
> And OWNED BY NONE will need to be deparseable.

Mumble.

> > +static ObjTree *
> > +deparse_CreateSeqStmt(Oid objectId, Node *parsetree)
> > +{

> > + /* we purposefully do not emit OWNED BY here */
>
> Needs explanation about the reason. Hm. I don't think it's actually
> correct. Right now the following won't be deparsed correctly:
>
> CREATE TABLE seqowner(id int);
> CREATE SEQUENCE seqowned OWNED BY seqowner.id;
> that generates
> CREATE TABLE public.seqowner (id pg_catalog.int4 ) WITH (oids=OFF)
> CREATE SEQUENCE public.seqowned CACHE 1 NO CYCLE INCREMENT BY 1 MINVALUE 1 MAXVALUE 9223372036854775807 START WITH 1 RESTART 1

I think I was blindfolded into supporting SERIAL. Will look into this.

> > + * deparse_CreateExtensionStmt
> > + * deparse a CreateExtensionStmt
> > + *
> > + * Given an extension OID and the parsetree that created it, return the JSON
> > + * blob representing the creation command.
> > + *
> > + * XXX the current representation makes the output command dependant on the
> > + * installed versions of the extension. Is this a problem?
>
> I tend to think it's ok. Might be worthwhile to add the final extension
> version in some unused attribute?

Yes, agreed.

> > It supports everything but functions, aggregates, operator classes and
> > operator families.
>
> AFAICS those are implemented, and you updated the description in git
> since.

Yeah, sorry. I just changed the way we handle functions/aggregates as
you suggest, by adding format_procedure_args(). However, in looking
closer, the representation is not very nice, because you end up with
veyr ugly quoting:

alvherre=# alter function "test schema"."foo bar" ("test schema"."test type") rename to "ack ick";
NOTICE: JSON blob: {
"fmt": "ALTER FUNCTION %{identity}s RENAME TO %{newname}I",
"identity": "\"test schema\".\"ack ick\"(\"test schema\".\"test type\")",
"newname": "ack ick"
}
NOTICE: expanded: ALTER FUNCTION "test schema"."ack ick"("test schema"."test type") RENAME TO "ack ick"
ALTER FUNCTION

Note the identity bit. I think I should use a signature object like
CREATE FUNCTION, which ends up a lot more verbose but seems more usable
programmatically:

"signature": {
"arguments": [
{
"default": {
"fmt": "DEFAULT %{value}s",
"present": false
},
"fmt": "%{mode}s %{name}s %{type}T %{default}s",
"mode": "IN",
"name": {
"fmt": "%{name}I",
"name": "a",
"present": true
},
"type": {
"schemaname": "test schema",
"typarray": false,
"typename": "test type",
"typmod": ""
}
}
],
"fmt": "%{identity}D(%{arguments:, }s)",
"identity": {
"objname": "test function",
"schemaname": "test schema"
}

(ALTER FUNCTION RENAME has no need for things such as DEFAULT and INOUT,
though, I think, so it might end up being simpler than this)

> > +static ObjTree *
> > +deparse_RenameStmt(Oid objectId, Node *parsetree)
> > +{
> > + RenameStmt *node = (RenameStmt *) parsetree;
> > + ObjTree *renameStmt;
> > + char *fmtstr;
> > + Relation relation;
> > + Oid schemaId;
> > +
> > + /*
> > + * FIXME --- this code is missing support for inheritance behavioral flags,
> > + * i.e. the "*" and ONLY elements.
> > + */
>
> Hm. I think we probably need that?

Sure.

> > + * XXX what if there's another event trigger running concurrently that
> > + * renames the schema or moves the object to another schema? Seems
> > + * pretty far-fetched, but possible nonetheless.
> > + */
>
> We should probably prohibit DDL from within event triggers?

Not real sure about adding that restriction. Would make things simpler,
sure.

> > + switch (node->renameType)
> > + {
> > + case OBJECT_COLLATION:
> > + case OBJECT_CONVERSION:
> > + case OBJECT_DOMAIN:
> > + case OBJECT_TSDICTIONARY:
> > + case OBJECT_TSPARSER:
> > + case OBJECT_TSTEMPLATE:
> > + case OBJECT_TSCONFIGURATION:
> > + case OBJECT_TYPE:
> > + {
> > + char *identity;
> > + HeapTuple objTup;
> > + Relation catalog;
> > + Datum objnsp;
> > + bool isnull;
> > + Oid classId = get_objtype_catalog_oid(node->renameType);
> > + AttrNumber Anum_namespace = get_object_attnum_namespace(classId);
> > +
> > + catalog = relation_open(classId, AccessShareLock);
> > + objTup = get_catalog_object_by_oid(catalog, objectId);
> > + objnsp = heap_getattr(objTup, Anum_namespace,
> > + RelationGetDescr(catalog), &isnull);
> > + if (isnull)
> > + elog(ERROR, "invalid NULL namespace");
> > +
> > + identity = psprintf("%s.%s", get_namespace_name(DatumGetObjectId(objnsp)),
> > + strVal(llast(node->object)));
> > + fmtstr = psprintf("ALTER %s %%{identity}s RENAME TO %%{newname}I",
> > + stringify_objtype(node->renameType));
>
> Is that correct? I.e. will it work for a namespace that needs to be
> quoted? There's a couple of these. Afaics they all should use
> new_objtree_for_qualname and %{}D? I guess in some cases, like the type,
> that could be slightly more complicated, but I guess it has to be done
> nonetheless?

Well, the quoting could be solved by adding a call to
quote_qualified_identifier(), but you're right that it would be simpler
to use %{}D here. It would apply similarly to the function example I
gave above.

> > + case OBJECT_AGGREGATE:
> > + case OBJECT_FUNCTION:

> > + /*
> > + * Generating a function/aggregate identity is altogether too
> > + * messy, so instead of doing it ourselves, we generate one for
> > + * the renamed object, then strip out the name and replace it
> > + * with the original name from the parse node. This is so ugly
> > + * that we don't dare do it for any other object kind.
> > + */
>
> Yuck.
>
> I think it'd be just as easy to split the argument output from the
> function name output in format_procedure_internal, and make that
> separately callable.

Fixed, at least partially.

> > Subject: [PATCH 18/42] deparse: Support CREATE FUNCTION
>
> > /*
> > + * deparse_CreateFunctionStmt
> > + * Deparse a CreateFunctionStmt (CREATE FUNCTION)
> > + *
> > + * Given a function OID and the parsetree that created it, return the JSON
> > + * blob representing the creation command.
> > + *
> > + * XXX this is missing the per-function custom-GUC thing.
> > + */
>
> Support attached as 0003.

Thanks, folded.

> > +void
> > +EventTriggerComplexCmdStart(Node *parsetree, ObjectType objtype)
> > +{
> > + MemoryContext oldcxt;
> > + StashedCommand *stashed;
> > +
> > + oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
> > +
> > + stashed = palloc(sizeof(StashedCommand));
> > +
> > + stashed->type = SCT_AlterTable;

> Hm. So +EventTriggerComplexCmdStart is hardcoded to use SCT_AlterTable? That's a bit odd.

Yeah, that's broken, isn't it. My initial thought was that we would
have Simple commands and Complex commands only. Only later I grew the
SCT_Grant case, and it's becoming obvious that I will need
SCT_AlterDefaultPrivileges and SCT_AlterOpFamily, at least. I will
probably rename these functions from StuffComplexCommandSomething into
StuffAlterTableSomething, to make it clear that they only serve
AlterTable.

> > +/*
> > + * EventTriggerEndRecordingSubcmds
> > + * Finish up saving a complex DDL command
> > + *
> > + * 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.
> > + */
>
> I think that'd generally be better than requiring PG_CATCH.

Haven't done anything about this yet.

> > +static ObjTree *
> > +deparse_AlterTableStmt(StashedCommand *cmd)
> > +{
>
> > + foreach(cell, cmd->d.alterTable.subcmds)
> > + {
> > + StashedATSubcmd *substashed = (StashedATSubcmd *) lfirst(cell);
> > + AlterTableCmd *subcmd = (AlterTableCmd *) substashed->parsetree;
> > + ObjTree *tree;
> > +
> > + Assert(IsA(subcmd, AlterTableCmd));
> > +
> > + switch (subcmd->subtype)
> > + {
> > + case AT_AddColumn:
> > + case AT_AddColumnRecurse:
> > + /* XXX need to set the "recurse" bit somewhere? */
> > + Assert(IsA(subcmd->def, ColumnDef));
> > + tree = deparse_ColumnDef(rel, dpcontext, false,
> > + (ColumnDef *) subcmd->def);
> > + tmp = new_objtree_VA("ADD COLUMN %{definition}s",
> > + 2, "type", ObjTypeString, "add column",
> > + "definition", ObjTypeObject, tree);
> > + subcmds = lappend(subcmds, new_object_object(tmp));
> > + break;
>
> Misses ONLY handling?

Yeah -- the "Recurse" bit in the subtype tells us whether to add ONLY or
not, I think.

> > + case AT_AddIndex:
> > + {
> > + Oid idxOid = substashed->oid;
> > + IndexStmt *istmt;
> > + Relation idx;
> > + const char *idxname;
> > + Oid constrOid;
> > +
> > + Assert(IsA(subcmd->def, IndexStmt));
> > + istmt = (IndexStmt *) subcmd->def;
> > +
> > + if (!istmt->isconstraint)
> > + break;
> > +
> > + idx = relation_open(idxOid, AccessShareLock);
> > + idxname = RelationGetRelationName(idx);
> > +
> > + constrOid = get_relation_constraint_oid(
> > + cmd->d.alterTable.objectId, idxname, false);
> > +
> > + tmp = new_objtree_VA("ADD CONSTRAINT %{name}I %{definition}s",
> > + 3, "type", ObjTypeString, "add constraint",
> > + "name", ObjTypeString, idxname,
> > + "definition", ObjTypeString,
> > + pg_get_constraintdef_string(constrOid, false));
> > + subcmds = lappend(subcmds, new_object_object(tmp));
> > +
> > + relation_close(idx, AccessShareLock);
> > + }
> > + break;
>
> Hm, didn't you have a out of line version of this somewhere?

It's similar, but as far as I remember the other one was different.
Didn't look closely, though.

> > + case AT_AlterColumnType:
> > + {
>
> > + /*
> > + * Error out if the USING clause was used. We cannot use
> > + * it directly here, because it needs to run through
> > + * transformExpr() before being usable for ruleutils.c, and
> > + * we're not in a position to transform it ourselves. To
> > + * fix this problem, tablecmds.c needs to be modified to store
> > + * the transformed expression somewhere in the StashedATSubcmd.
> > + */
> > + tmp2 = new_objtree_VA("USING %{expression}s", 0);
> > + if (def->raw_default)
> > + elog(ERROR, "unimplemented deparse of ALTER TABLE TYPE USING");
> > + else
> > + append_bool_object(tmp2, "present", false);
> > + append_object_object(tmp, "using", tmp2);
>
> Hm. This probably needs to be fixed :(.

Sure.

> Shouldn't be too hard...
>
> Could you perhaps attach some string that can be searched for to all
> commands that you think need to be implemented before being committable?

Well, I think all the ones that say "unimplemented" should be
implemented before commit. Most of the remaining ones should be
relatively simple to add, with the two exceptions I mentioned above that
require their own symbol in the SCT enum.

> > else
> > {
> > - /* Recurse for anything else */
> > + /*
> > + * Recurse for anything else. If we need to do
> > + * so, "close" the current complex-command set,
> > + * and start a new one at the bottom; this is
> > + * needed to ensure the ordering of queued
> > + * commands is consistent with the way they are
> > + * executed here.
> > + */
> > + EventTriggerComplexCmdEnd();
> > ProcessUtility(stmt,
> > queryString,
> > PROCESS_UTILITY_SUBCOMMAND,
> > params,
> > None_Receiver,
> > NULL);
> > + EventTriggerComplexCmdStart(parsetree, atstmt->relkind);
> > + EventTriggerComplexCmdSetOid(relid);
> > }
>
> Hm. Commands are always ordered in a way this is ok?

Well, as far as I can tell this works well. If we had a concrete
example of a problematic sequence of commands, we could look into how to
fix it.

> > + copfStmt = new_objtree_VA("CREATE OPERATOR FAMILY %{identity}D USING %{amname}s",
> > + 0);
> > +
> > + tmp = new_objtree_for_qualname(opfForm->opfnamespace,
> > + NameStr(opfForm->opfname));
> > + append_object_object(copfStmt, "identity", tmp);
> > + append_string_object(copfStmt, "amname", NameStr(amForm->amname));
>
> Hm. Amname is unquoted? I can't believe this will ever really be a problem, but still.

That's a bug -- fixed.

> > Subject: [PATCH 24/42] deparse: support ALTER THING OWNER TO
> > static ObjTree *
> > +deparse_AlterOwnerStmt(Oid objectId, Node *parsetree)
> > +{
> > + AlterOwnerStmt *node = (AlterOwnerStmt *) parsetree;
> > + ObjTree *ownerStmt;
> > + ObjectAddress addr;
> > + char *fmt;
> > +
> > + fmt = psprintf("ALTER %s %%{identity}s OWNER TO %%{newname}I",
> > + stringify_objtype(node->objectType));
>
> newname sounds like copied from rename.

Oops, true. Renamed to "newowner".

> > + else
> > + {
> > + Assert(cmd->type == SCT_Grant);
> > +
> > + /* classid */
> > + nulls[i++] = true;
> > + /* objid */
> > + nulls[i++] = true;
> > + /* objsubid */
> > + nulls[i++] = true;
>
> Might actually be nice to fill those out to the object that's being
> changed. But it might end up being more work than justified, and it can
> be easily added later. I guess it'd also mean we would have to generate
> multiple GRANT/REVOKE commands.

Nah -- a single GRANT command can modify several objects of the same
kind. That's why I used a separate representation.

> > diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
> > index c09915d..92bccf5 100644
> > --- a/src/backend/tcop/utility.c
> > +++ b/src/backend/tcop/utility.c
> > @@ -781,6 +781,7 @@ standard_ProcessUtility(Node *parsetree,
> > else
> > ExecuteGrantStmt((GrantStmt *) parsetree);
> > }
> > + break;
>
> Uh. Was that missing from an earlier commit?

Yep, sorry, fixed. (In fact I already pushed it.)

> > diff --git a/src/include/utils/aclchk.h b/src/include/utils/aclchk.h
> > new file mode 100644
> > index 0000000..1ca7095
> > --- /dev/null
> > +++ b/src/include/utils/aclchk.h
> > @@ -0,0 +1,45 @@
> > +/*-------------------------------------------------------------------------
> > + *
> > + * aclchk.h
> > + *
> > + * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
> > + * Portions Copyright (c) 1994, Regents of the University of California
> > + *
> > + * src/include/utils/aclchk.h
>
> Maybe we should name it aclchk_internal.h?

Might as well ...

> > +static ObjTree *
> > +deparse_CommentOnConstraintSmt(Oid objectId, Node *parsetree)
> > +{
> > + CommentStmt *node = (CommentStmt *) parsetree;
> > + ObjTree *comment;
> > + HeapTuple constrTup;
> > + Form_pg_constraint constrForm;
> > + char *fmt;
> > + ObjectAddress addr;
> > +
> > + Assert(node->objtype == OBJECT_TABCONSTRAINT || node->objtype == OBJECT_DOMCONSTRAINT);
> > +
> > + if (node->comment)
> > + fmt = psprintf("COMMENT ON CONSTRAINT %%{identity}s ON %s%%{parentobj}s IS %%{comment}L",
> > + node->objtype == OBJECT_TABCONSTRAINT ? "" : "DOMAIN ");
> > + else
> > + fmt = psprintf("COMMENT ON CONSTRAINT %%{identity}s ON %s%%{parentobj}s IS NULL",
> > + node->objtype == OBJECT_TABCONSTRAINT ? "" : "DOMAIN ");
>
> psprintf(...' IS %s', .... node->comment ? "%{comment}L" : "NULL") maybe?

I fixed COMMENT and SECURITY LABEL to use a different, more complex
representation, that can be used to turn the comment literal and the
NULL part on/off. That seems better for programmatic access.

> > + if (node->label)
> > + {
> > + fmt = psprintf("SECURITY LABEL FOR %%{provider}s ON %s %%{identity}s IS %%{label}L",
> > + stringify_objtype(node->objtype));
> > + label = new_objtree_VA(fmt, 0);
> > +
> > + append_string_object(label, "label", node->label);
> > + }

> "deparse: Handle default security provider." should be squashed into this.

No, actually I think that one is wrong. The security provider should be
returned as an Oid into secondaryOid to ProcessUtilitySlow, to avoid
messing with the parse node.

> > + if (node->relkind == OBJECT_TABLE)
> > + {
> > + tmp = new_objtree_VA("ON COMMIT %{on_commit_value}s", 0);
> > + switch (node->into->onCommit)
> > + {
> > + case ONCOMMIT_DROP:
> > + append_string_object(tmp, "on_commit_value", "DROP");
> > + break;
> > +
> > + case ONCOMMIT_DELETE_ROWS:
> > + append_string_object(tmp, "on_commit_value", "DELETE ROWS");
> > + break;
> > +
> > + case ONCOMMIT_PRESERVE_ROWS:
> > + append_string_object(tmp, "on_commit_value", "PRESERVE ROWS");
> > + break;
> > +
> > + case ONCOMMIT_NOOP:
> > + append_null_object(tmp, "on_commit_value");
> > + append_bool_object(tmp, "present", false);
> > + break;
> > + }
> > + append_object_object(createStmt, "on_commit", tmp);
> > + }
>
> That switch already exists somewhere else? Maybe add a function that
> converts ONCOMMIT_* into the relevant string?

Some refactoring is probably in order here.

> Ran out of energy here...

You had a lot of it -- this stuff isn't small. Many, many thanks for
the review.

> Subject: [PATCH 2/3] fixup! deparse: infrastructure needed for command
> deparsing
>
> Slight cleanup in new_objtree_VA() by treating ObjTypeNull just like the
> other types.

BTW these two patches created a compiler warning for me; it says "elem
can be used uninitialized". I had to add a default clause.

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