Re: deparsing utility commands

Hi,

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?

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

> From d03a8edcefd8f0ea1c46b315c446f96c61146a85 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Date: Wed, 24 Sep 2014 15:53:04 -0300
> Subject: [PATCH 07/42] deparse: infrastructure needed for command deparsing
>
> This patch introduces unused infrastructure for command deparsing.
> There are three main parts:

+ "as of yet"?

> 1. A list (stash) of executed commands in Node format, stored in the
> event trigger state. At ddl_command_end, the stored items can be
> extracted. For now this only support "basic" commands (in particular
> not ALTER TABLE or GRANT). It's useful enough to cover all CREATE
> command as well as many simple ALTER forms.

seems outdated.

(yea, I know you want to fold the patch anyway).

> +/* XXX merge this with ObjectTypeMap? */
> static event_trigger_support_data event_trigger_support[] = {
> {"AGGREGATE", true},
> {"CAST", true},

Hm, I'd just drop the XXX for now.

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

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

> +/*
> + * EventTriggerStashCommand
> + * Save data about a simple DDL command that was just executed
> + */
> +void
> +EventTriggerStashCommand(Oid objectId, uint32 objectSubId, ObjectType objtype,
> + Oid secondaryOid, Node *parsetree)
> +{
> + MemoryContext oldcxt;
> + StashedCommand *stashed;
> +
> + oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
> +
> + stashed = palloc(sizeof(StashedCommand));
> +
> + stashed->type = SCT_Simple;
> + stashed->in_extension = creating_extension;
> +
> + stashed->d.simple.objectId = objectId;
> + stashed->d.simple.objtype = objtype;
> + stashed->d.simple.objectSubId = objectSubId;
> + stashed->d.simple.secondaryOid = secondaryOid;
> + stashed->parsetree = copyObject(parsetree);
> +
> + currentEventTriggerState->stash = lappend(currentEventTriggerState->stash,
> + stashed);
> +
> + MemoryContextSwitchTo(oldcxt);
> +}
>
It's a bit wierd that the drop list is managed using a slist, but the
stash isn't...

> +Datum
> +pg_event_trigger_get_creation_commands(PG_FUNCTION_ARGS)
> +{

> + foreach(lc, currentEventTriggerState->stash)
> + {

> + command = deparse_utility_command(cmd);
> +
> + /*
> + * Some parse trees return NULL when deparse is attempted; we don't
> + * emit anything for them.
> + */
> + if (command != NULL)
> + {
> + Datum values[9];
> + bool nulls[9];
> + ObjectAddress addr;
> + int i = 0;
> +
> + MemSet(nulls, 0, sizeof(nulls));
> +
> + if (cmd->type == SCT_Simple)
> + {
> + Oid classId;
> + Oid objId;
> + uint32 objSubId;
> + const char *tag;
> + char *identity;
> + char *type;
> + char *schema = NULL;
> +
> + if (cmd->type == SCT_Simple)
> + {
> + classId = get_objtype_catalog_oid(cmd->d.simple.objtype);
> + objId = cmd->d.simple.objectId;
> + objSubId = cmd->d.simple.objectSubId;
> + }
> +
> + tag = CreateCommandTag(cmd->parsetree);
> + addr.classId = classId;
> + addr.objectId = objId;
> + addr.objectSubId = objSubId;
> +
> + type = getObjectTypeDescription(&addr);
> + identity = getObjectIdentity(&addr);
> +
> + /*
> + * 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()?

> +/*
> + * Allocate a new object tree to store parameter values -- varargs version.
> + *
> + * The "fmt" argument is used to append as a "fmt" element in the output blob.
> + * numobjs indicates the number of extra elements to append; for each one, a
> + * name (string), type (from the ObjType enum) and value must be supplied. The
> + * value must match the type given; for instance, ObjTypeInteger requires an
> + * int64, ObjTypeString requires a char *, ObjTypeArray requires a list (of
> + * ObjElem), ObjTypeObject requires an ObjTree, and so on. Each element type *
> + * must match the conversion specifier given in the format string, as described
> + * in pg_event_trigger_expand_command, q.v.
> + *
> + * Note we don't have the luxury of sprintf-like compiler warnings for
> + * malformed argument lists.
> + */
> +static ObjTree *
> +new_objtree_VA(char *fmt, int numobjs,...)
> +{
> + ObjTree *tree;
> + va_list args;
> + int i;
> +
> + /* Set up the toplevel object and its "fmt" */
> + tree = new_objtree();
> + append_string_object(tree, "fmt", fmt);
> +
> + /* And process the given varargs */
> + va_start(args, numobjs);
> + for (i = 0; i < numobjs; i++)
> + {
> + ObjTree *value;
> + ObjType type;
> + ObjElem *elem;
> + char *name;
> + char *strval;
> + bool boolval;
> + List *list;
> + int64 number;
> + float8 fnumber;
> +
> + name = va_arg(args, char *);
> + type = va_arg(args, ObjType);
> +
> + /* Null params don't have a value (obviously) */
> + if (type == ObjTypeNull)
> + {
> + append_null_object(tree, name);
> + continue;
> + }
> +
> + /*
> + * For all other param types there must be a value in the varargs.
> + * Fetch it and add the fully formed subobject into the main object.
> + */
> + switch (type)
> + {
> + case ObjTypeBool:
> + boolval = va_arg(args, int);
> + elem = new_bool_object(boolval);
> + break;
> + case ObjTypeString:
> + strval = va_arg(args, char *);
> + elem = new_string_object(strval);
> + break;
> + case ObjTypeObject:
> + value = va_arg(args, ObjTree *);
> + elem = new_object_object(value);
> + break;
> + case ObjTypeArray:
> + list = va_arg(args, List *);
> + elem = new_array_object(list);
> + break;
> + case ObjTypeInteger:
> + number = va_arg(args, int64);
> + elem = new_integer_object(number);
> + break;
> + case ObjTypeFloat:
> + fnumber = va_arg(args, double);
> + elem = new_float_object(fnumber);
> + break;
> + default:
> + elog(ERROR, "invalid parameter type %d", type);
> + }
> +
> + elem->name = name;
> + append_premade_object(tree, elem);
> + }
> +
> + va_end(args);
> + return tree;
> +}

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.

> +/*
> + * 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?

> +/*
> + * 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)
> +{
> +
> + case T_CommentStmt:
> + command = NULL;
> + break;
> +
> + case T_GrantStmt:
> + /* handled elsewhere */
> + elog(ERROR, "unexpected command type %s", CreateCommandTag(parsetree));
> + break;
...
> + case T_SecLabelStmt:
> + elog(ERROR, "unimplemented deparse of %s", CreateCommandTag(parsetree));
> + break;
> + }
> +
> + return command;
> +}

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?

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

> +/*
> + * Given a utility command parsetree and the OID of the corresponding object,
> + * return a JSON representation of the command.
> + *
> + * The command is expanded fully, so that there are no ambiguities even in the
> + * face of search_path changes.
> + */
> +char *
> +deparse_utility_command(StashedCommand *cmd)
> +{
> + OverrideSearchPath *overridePath;
> + MemoryContext oldcxt;
> + MemoryContext tmpcxt;
> + ObjTree *tree;
> + char *command;
> + StringInfoData str;
> +
> + /*
> + * Allocate everything done by the deparsing routines into a temp context,
> + * to avoid having to sprinkle them with memory handling code; but allocate
> + * the output StringInfo before switching.
> + */
> + initStringInfo(&str);
> + tmpcxt = AllocSetContextCreate(CurrentMemoryContext,
> + "deparse ctx",
> + ALLOCSET_DEFAULT_MINSIZE,
> + ALLOCSET_DEFAULT_INITSIZE,
> + ALLOCSET_DEFAULT_MAXSIZE);
> + oldcxt = MemoryContextSwitchTo(tmpcxt);
> +
> + /*
> + * 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...

> +/*------
> + * 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.

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

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

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

> deparse: Support CREATE SCHEMA/TABLE/SEQUENCE/INDEX/TRIGGER

> +/*
> + * deparse_CreateTrigStmt
> + * Deparse a CreateTrigStmt (CREATE TRIGGER)
> + *
> + * Given a trigger OID and the parsetree that created it, return an ObjTree
> + * representing the creation command.
> + */
> +static ObjTree *
> +deparse_CreateTrigStmt(Oid objectId, Node *parsetree)
> +{
> + CreateTrigStmt *node = (CreateTrigStmt *) parsetree;
> + Relation pg_trigger;
> + HeapTuple trigTup;
> + Form_pg_trigger trigForm;
> + ObjTree *trigger;
> + ObjTree *tmp;
> + int tgnargs;
> + List *list;
> + List *events;
> +
> + pg_trigger = heap_open(TriggerRelationId, AccessShareLock);
> +
> + trigTup = get_catalog_object_by_oid(pg_trigger, objectId);
> + trigForm = (Form_pg_trigger) GETSTRUCT(trigTup);
> +
> + /*
> + * Some of the elements only make sense for CONSTRAINT TRIGGERs, but it
> + * seems simpler to use a single fmt string for both kinds of triggers.
> + */
> + trigger =
> + new_objtree_VA("CREATE %{constraint}s TRIGGER %{name}I %{time}s %{events: OR }s "
> + "ON %{relation}D %{from_table}s %{constraint_attrs: }s "
> + "FOR EACH %{for_each}s %{when}s EXECUTE PROCEDURE %{function}s",
> + 2,
> + "name", ObjTypeString, node->trigname,
> + "constraint", ObjTypeString,
> + node->isconstraint ? "CONSTRAINT" : "");
> +
> + if (node->timing == TRIGGER_TYPE_BEFORE)
> + append_string_object(trigger, "time", "BEFORE");
> + else if (node->timing == TRIGGER_TYPE_AFTER)
> + append_string_object(trigger, "time", "AFTER");
> + else if (node->timing == TRIGGER_TYPE_INSTEAD)
> + append_string_object(trigger, "time", "INSTEAD OF");
> + else
> + elog(ERROR, "unrecognized trigger timing value %d", node->timing);
> +
> + /*
> + * Decode the events that the trigger fires for. The output is a list;
> + * in most cases it will just be a string with the even name, but when
> + * there's an UPDATE with a list of columns, we return a JSON object.
> + */
> + events = NIL;
> + if (node->events & TRIGGER_TYPE_INSERT)
> + events = lappend(events, new_string_object("INSERT"));
> + if (node->events & TRIGGER_TYPE_DELETE)
> + events = lappend(events, new_string_object("DELETE"));
> + if (node->events & TRIGGER_TYPE_TRUNCATE)
> + events = lappend(events, new_string_object("TRUNCATE"));
> + if (node->events & TRIGGER_TYPE_UPDATE)
> + {
> + if (node->columns == NIL)
> + {
> + events = lappend(events, new_string_object("UPDATE"));
> + }
> + else
> + {
> + ObjTree *update;
> + ListCell *cell;
> + List *cols = NIL;
> +
> + /*
> + * Currently only UPDATE OF can be objects in the output JSON, but
> + * we add a "kind" element so that user code can distinguish
> + * possible future new event types.
> + */
> + update = new_objtree_VA("UPDATE OF %{columns:, }I",
> + 1, "kind", ObjTypeString, "update_of");
> +
> + foreach(cell, node->columns)
> + {
> + char *colname = strVal(lfirst(cell));
> +
> + cols = lappend(cols,
> + new_string_object(colname));
> + }
> +
> + append_array_object(update, "columns", cols);
> +
> + events = lappend(events,
> + new_object_object(update));
> + }
> + }
> + append_array_object(trigger, "events", events);
> +
> + tmp = new_objtree_for_qualname_id(RelationRelationId,
> + trigForm->tgrelid);
> + append_object_object(trigger, "relation", tmp);
> +
> + tmp = new_objtree_VA("FROM %{relation}D", 0);
> + if (trigForm->tgconstrrelid)
> + {
> + ObjTree *rel;
> +
> + rel = new_objtree_for_qualname_id(RelationRelationId,
> + trigForm->tgconstrrelid);
> + append_object_object(tmp, "relation", rel);
> + }
> + else
> + append_bool_object(tmp, "present", false);
> + append_object_object(trigger, "from_table", tmp);
> +
> + 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.

> +/*
> + * 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 ;)

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

> +static ObjElem *
> +deparse_Seq_OwnedBy(ObjTree *parent, Oid sequenceId)
> +{
> + ObjTree *ownedby = NULL;
> + Relation depRel;
> + SysScanDesc scan;
> + ScanKeyData keys[3];
> + HeapTuple tuple;
> +
> + depRel = heap_open(DependRelationId, AccessShareLock);
> + ScanKeyInit(&keys[0],
> + Anum_pg_depend_classid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(RelationRelationId));
> + ScanKeyInit(&keys[1],
> + Anum_pg_depend_objid,
> + BTEqualStrategyNumber, F_OIDEQ,
> + ObjectIdGetDatum(sequenceId));
> + ScanKeyInit(&keys[2],
> + Anum_pg_depend_objsubid,
> + BTEqualStrategyNumber, F_INT4EQ,
> + Int32GetDatum(0));
> +
> + scan = systable_beginscan(depRel, DependDependerIndexId, true,
> + NULL, 3, keys);
> + while (HeapTupleIsValid(tuple = systable_getnext(scan)))
> + {
> + Oid ownerId;
> + Form_pg_depend depform;
> + ObjTree *tmp;
> + char *colname;
> +
> + depform = (Form_pg_depend) GETSTRUCT(tuple);
> +
> + /* only consider AUTO dependencies on pg_class */
> + if (depform->deptype != DEPENDENCY_AUTO)
> + continue;
> + if (depform->refclassid != RelationRelationId)
> + continue;
> + if (depform->refobjsubid <= 0)
> + continue;
> +
> + ownerId = depform->refobjid;
> + colname = get_attname(ownerId, depform->refobjsubid);
> + if (colname == NULL)
> + continue;
> +
> + tmp = new_objtree_for_qualname_id(RelationRelationId, ownerId);
> + append_string_object(tmp, "attrname", colname);
> + ownedby = new_objtree_VA("OWNED BY %{owner}D",
> + 2,
> + "clause", ObjTypeString, "owned",
> + "owner", ObjTypeObject, tmp);
> + }
> +
> + systable_endscan(scan);
> + relation_close(depRel, AccessShareLock);
> +
> + /*
> + * If there's no owner column, emit an empty OWNED BY element, set up so
> + * that it won't print anything.
> + */

"column"? Should have been row?

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

> +static ObjTree *
> +deparse_CreateSeqStmt(Oid objectId, Node *parsetree)
> +{
> + ObjTree *createSeq;
> + ObjTree *tmp;
> + Relation relation = relation_open(objectId, AccessShareLock);
> + Form_pg_sequence seqdata;
> + List *elems = NIL;
> +
> + seqdata = get_sequence_values(objectId);
> +
> + createSeq =
> + new_objtree_VA("CREATE %{persistence}s SEQUENCE %{identity}D "
> + "%{definition: }s",
> + 1,
> + "persistence", ObjTypeString,
> + get_persistence_str(relation->rd_rel->relpersistence));
> +
> + tmp = new_objtree_for_qualname(relation->rd_rel->relnamespace,
> + RelationGetRelationName(relation));
> + append_object_object(createSeq, "identity", tmp);
> +
> + /* definition elements */
> + elems = lappend(elems, deparse_Seq_Cache(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_Cycle(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_IncrementBy(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_Minvalue(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_Maxvalue(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_Startwith(createSeq, seqdata));
> + elems = lappend(elems, deparse_Seq_Restart(createSeq, seqdata));
> + /* 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

> +/*
> + * deparse_IndexStmt
> + * deparse an IndexStmt
> + *
> + * Given an index OID and the parsetree that created it, return an ObjTree
> + * representing the creation command.
> + *
> + * If the index corresponds to a constraint, NULL is returned.
> + */
> +static ObjTree *
> +deparse_IndexStmt(Oid objectId, Node *parsetree)
> +{

> + /* tablespace */
> + tmp = new_objtree_VA("TABLESPACE %{tablespace}s", 0);
> + if (tablespace)
> + append_string_object(tmp, "tablespace", tablespace);
> + else
> + append_bool_object(tmp, "present", false);
> + append_object_object(indexStmt, "tablespace", tmp);

Same default tablespace question as with create table...

> From 4e35ba6557a6d04539d69d75821c568f9efb09b5 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Date: Fri, 14 Feb 2014 19:04:08 -0300
> Subject: [PATCH 12/42] deparse: Support CREATE TYPE AS RANGE
>

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

> + /* SUBTYPE_OPCLASS */
> + if (OidIsValid(rangeForm->rngsubopc))
> + {
> + tmp = new_objtree_for_qualname_id(OperatorClassRelationId,
> + rangeForm->rngsubopc);
> + tmp = new_objtree_VA("SUBTYPE_OPCLASS = %{opclass}D",
> + 2,
> + "clause", ObjTypeString, "opclass",
> + "opclass", ObjTypeObject, tmp);
> + definition = lappend(definition, new_object_object(tmp));
> + }
> +
> + /* COLLATION */
> + if (OidIsValid(rangeForm->rngcollation))
> + {
> + tmp = new_objtree_for_qualname_id(CollationRelationId,
> + rangeForm->rngcollation);
> + tmp = new_objtree_VA("COLLATION = %{collation}D",
> + 2,
> + "clause", ObjTypeString, "collation",
> + "collation", ObjTypeObject, tmp);
> + definition = lappend(definition, new_object_object(tmp));
> + }
> +
> + /* CANONICAL */
> + if (OidIsValid(rangeForm->rngcanonical))
> + {
> + tmp = new_objtree_for_qualname_id(ProcedureRelationId,
> + rangeForm->rngcanonical);
> + tmp = new_objtree_VA("CANONICAL = %{canonical}D",
> + 2,
> + "clause", ObjTypeString, "canonical",
> + "canonical", ObjTypeObject, tmp);
> + definition = lappend(definition, new_object_object(tmp));
> + }
> +
> + /* SUBTYPE_DIFF */
> + if (OidIsValid(rangeForm->rngsubdiff))
> + {
> + tmp = new_objtree_for_qualname_id(ProcedureRelationId,
> + rangeForm->rngsubdiff);
> + tmp = new_objtree_VA("SUBTYPE_DIFF = %{subtype_diff}D",
> + 2,
> + "clause", ObjTypeString, "subtype_diff",
> + "subtype_diff", ObjTypeObject, tmp);
> + definition = lappend(definition, new_object_object(tmp));
> + }

Maybe use the present = false stuff here as well?

> From 04dc40db971dc51c7e8c646cb5d822488da306f7 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Date: Fri, 21 Feb 2014 18:11:35 -0300
> Subject: [PATCH 13/42] deparse: Support CREATE EXTENSION

> /*
> + * 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?

> From f45a9141905e93b83d28809e357f95c7fa713fe7 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Date: Wed, 26 Feb 2014 17:26:55 -0300
> Subject: [PATCH 14/42] deparse: Support CREATE RULE

/me scheds some tears

> From 67ff742875bfadd182ae28e40e54476c9e4d220e Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> Date: Fri, 25 Apr 2014 16:43:53 -0300
> Subject: [PATCH 16/42] deparse: Support for ALTER <OBJECT> RENAME
>
> It supports everything but functions, aggregates, operator classes and
> operator families.

AFAICS those are implemented, and you updated the description in git
since.
> +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?

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

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

> + case OBJECT_AGGREGATE:
> + case OBJECT_FUNCTION:
> + {
> + char *newident;
> + ObjectAddress objaddr;
> + const char *quoted_newname;
> + StringInfoData old_ident;
> + char *start;
> +
> + /*
> + * 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.
> + */
> +
> + objaddr.classId = get_objtype_catalog_oid(node->renameType);
> + objaddr.objectId = objectId;
> + objaddr.objectSubId = 0;
> + newident = getObjectIdentity(&objaddr);
> +
> + quoted_newname = quote_identifier(node->newname);
> + start = strstr(newident, quoted_newname);
> + if (!start)
> + elog(ERROR, "could not find %s in %s", start, newident);
> + initStringInfo(&old_ident);
> + appendBinaryStringInfo(&old_ident, newident, start - newident);
> + appendStringInfoString(&old_ident,
> + quote_identifier(strVal(llast(node->object))));
> + appendStringInfoString(&old_ident, start + strlen(quoted_newname));
> +
> + fmtstr = psprintf("ALTER %s %%{identity}s RENAME TO %%{newname}I",
> + stringify_objtype(node->renameType));
> + renameStmt = new_objtree_VA(fmtstr, 1,
> + "identity", ObjTypeString, old_ident.data);

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.

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

> Subject: [PATCH 19/42] deparse: Support ALTER TABLE

> +/*
> + * EventTriggerStartRecordingSubcmds
> + * Prepare to receive data on a complex DDL command about to be executed
> + *
> + * Note we don't actually stash the object we create here into the "stashed"
> + * list; instead we keep it in curcmd, and only when we're done processing the
> + * subcommands we will add it to the actual stash.
> + *
> + * FIXME -- this API isn't considering the possibility of an ALTER TABLE command
> + * being called reentrantly by an event trigger function. Do we need stackable
> + * commands at this level?
> + */
> +void
> +EventTriggerComplexCmdStart(Node *parsetree, ObjectType objtype)
> +{
> + MemoryContext oldcxt;
> + StashedCommand *stashed;
> +
> + oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
> +
> + stashed = palloc(sizeof(StashedCommand));
> +
> + stashed->type = SCT_AlterTable;
> + stashed->in_extension = creating_extension;
> +
> + stashed->d.alterTable.objectId = InvalidOid;
> + stashed->d.alterTable.objtype = objtype;
> + stashed->d.alterTable.subcmds = NIL;
> + stashed->parsetree = copyObject(parsetree);
> +
> + currentEventTriggerState->curcmd = stashed;
> +
> + MemoryContextSwitchTo(oldcxt);
> +}

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

> +/*
> + * 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.

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

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

> + 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 :(.

> + }
> + break;
> +
> + case AT_AlterColumnGenericOptions:
> + elog(ERROR, "unimplemented deparse of ALTER TABLE ALTER COLUMN OPTIONS");
> + break;

> + case AT_SetRelOptions:
> + elog(ERROR, "unimplemented deparse of ALTER TABLE SET");
> + break;
> +
> + case AT_ResetRelOptions:
> + elog(ERROR, "unimplemented deparse of ALTER TABLE RESET");
> + break;

> + case AT_GenericOptions:
> + elog(ERROR, "unimplemented deparse of ALTER TABLE OPTIONS (...)");
> + break;
> +

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?

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

> Subject: [PATCH 21/42] deparse: Support CREATE OPERATOR FAMILY

> static ObjTree *
> +deparse_CreateOpFamily(Oid objectId, Node *parsetree)
> +{
> + HeapTuple opfTup;
> + HeapTuple amTup;
> + Form_pg_opfamily opfForm;
> + Form_pg_am amForm;
> + ObjTree *copfStmt;
> + ObjTree *tmp;
> +
> + opfTup = SearchSysCache1(OPFAMILYOID, ObjectIdGetDatum(objectId));
> + if (!HeapTupleIsValid(opfTup))
> + elog(ERROR, "cache lookup failed for operator family with OID %u", objectId);
> + opfForm = (Form_pg_opfamily) GETSTRUCT(opfTup);
> +
> + amTup = SearchSysCache1(AMOID, ObjectIdGetDatum(opfForm->opfmethod));
> + if (!HeapTupleIsValid(amTup))
> + elog(ERROR, "cache lookup failed for access method %u",
> + opfForm->opfmethod);
> + amForm = (Form_pg_am) GETSTRUCT(amTup);
> +
> + 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.

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

> Subject: [PATCH 25/42] deparse: Support ALTER EXTENSION / UPDATE TO

> Subject: [PATCH 26/42] deparse: Support GRANT/REVOKE

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

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

> case T_DropStmt:
> {
> diff --git a/src/include/commands/event_trigger.h b/src/include/commands/event_trigger.h
> index cdce9e1..60b590b 100644
> --- a/src/include/commands/event_trigger.h
> +++ b/src/include/commands/event_trigger.h
> @@ -17,6 +17,7 @@
> #include "catalog/objectaddress.h"
> #include "catalog/pg_event_trigger.h"
> #include "nodes/parsenodes.h"
> +#include "utils/aclchk.h"
>
> typedef struct EventTriggerData
> {
> @@ -62,6 +63,7 @@ extern void EventTriggerSQLDropAddObject(const ObjectAddress *object,
>
> extern void EventTriggerStashCommand(Oid objectId, uint32 objectSubId,
> ObjectType objtype, Oid secondaryOid, Node *parsetree);
> +extern void EventTriggerStashGrant(InternalGrant *istmt);
> extern void EventTriggerComplexCmdStart(Node *parsetree, ObjectType objtype);
> extern void EventTriggerComplexCmdSetOid(Oid objectId);
> extern void EventTriggerRecordSubcmd(Node *subcmd, Oid relid,
> diff --git a/src/include/tcop/deparse_utility.h b/src/include/tcop/deparse_utility.h
> index 8ebbf34..c364603 100644
> --- a/src/include/tcop/deparse_utility.h
> +++ b/src/include/tcop/deparse_utility.h
> @@ -14,6 +14,8 @@
>
> #include "access/attnum.h"
> #include "nodes/nodes.h"
> +#include "utils/aclchk.h"
> +
>
> /*
> * Support for keeping track of a command to deparse.
> @@ -26,7 +28,8 @@
> typedef enum StashedCommandType
> {
> SCT_Simple,
> - SCT_AlterTable
> + SCT_AlterTable,
> + SCT_Grant
> } StashedCommandType;
>
> /*
> @@ -61,6 +64,12 @@ typedef struct StashedCommand
> ObjectType objtype;
> List *subcmds;
> } alterTable;
> +
> + struct GrantCommand
> + {
> + InternalGrant *istmt;
> + const char *type;
> + } grant;
> } d;
> } StashedCommand;
>
> 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?

> Subject: [PATCH 27/42] deparse: Support ALTER FUNCTION

> + * deparse_AlterFunctionStmt
> + * Deparse a AlterFunctionStmt (ALTER FUNCTION)
> + *
> + * Given a function OID and the parsetree that created it, return the JSON
> + * blob representing the alter command.
> + *
> + * XXX this is missing the per-function custom-GUC thing.
> + */

Hm, so my earlier ptatch needs to partially be copied here. I'm running out of time now though...

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

> + foreach(cell, node->actions)
> + {
> + DefElem *defel = (DefElem *) lfirst(cell);
> + ObjTree *tmp = NULL;
> +
> + if (strcmp(defel->defname, "volatility") == 0)
> + {
> + tmp = new_objtree_VA(strVal(defel->arg), 0);
> + }
> + else if (strcmp(defel->defname, "strict") == 0)
> + {
> + tmp = new_objtree_VA(intVal(defel->arg) ?
> + "RETURNS NULL ON NULL INPUT" :
> + "CALLED ON NULL INPUT", 0);
> + }

Ick. I wish there was NOT STRICT.

> Subject: [PATCH 28/42] deparse: support COMMENT ON
>

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

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

> + if (node->comment)
> + fmt = psprintf("COMMENT ON %s %%{identity}s IS %%{comment}L",
> + stringify_objtype(node->objtype));
> + else
> + fmt = psprintf("COMMENT ON %s %%{identity}s IS NULL",
> + stringify_objtype(node->objtype));

similarly here?

> + comment = new_objtree_VA(fmt, 0);
> + if (node->comment)
> + append_string_object(comment, "comment", node->comment);

Or at leas tthis should be moved into the above if.

> Subject: [PATCH 29/42] deparse: support SECURITY LABEL

> static ObjTree *
> +deparse_SecLabelStmt(Oid objectId, Oid objectSubId, Node *parsetree)
> +{
> + SecLabelStmt *node = (SecLabelStmt *) parsetree;
> + ObjTree *label;
> + ObjectAddress addr;
> + char *fmt;
> +
> + 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);
> + }
> + else
> + {
> + fmt = psprintf("SECURITY LABEL FOR %%{provider}s ON %s %%{identity}s IS NULL",
> + stringify_objtype(node->objtype));
> + label = new_objtree_VA(fmt, 0);
> + }

Should probably merged similarly.

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

> Subject: [PATCH 34/42] deparse: support CREATE TABLE AS

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

> + /*
> + * Note that INSERT INTO is deparsed as CREATE TABLE AS. They are
> + * functionally equivalent synonyms so there is no harm from this.
> + */
> + if (node->relkind == OBJECT_MATVIEW)
> + fmt = "CREATE %{persistence}s MATERIALIZED VIEW %{if_not_exists}s "
> + "%{identity}D %{columns}s %{with}s %{tablespace}s "
> + "AS %{query}s %{with_no_data}s";
> + else
> + fmt = "CREATE %{persistence}s TABLE %{if_not_exists}s "
> + "%{identity}D %{columns}s %{with}s %{on_commit}s %{tablespace}s "
> + "AS %{query}s %{with_no_data}s";

With my replication hat on, which I admit isn't very general in this
case, this isn't actually what I want... What I really would rather have
there is a normal CREATE TABLE statement that I can replay on the other
side. But I guess that has to be done somehow in a utility hook :(
> + /* Add an ON COMMIT clause. CREATE MATERIALIZED VIEW doesn't have one */
> + 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?

Ran out of energy here...

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services