Re: Add CREATE support to event triggers

Actually here's a different split of these patches, which I hope makes
more sense. My intention here is that patches 0001 to 0004 are simple
changes that can be pushed right away; they are not directly related to
the return-creation-command feature. Patches 0005 to 0027 implement
that feature incrementally. You can see in patch 0005 the DDL commands
that are still not implemented in deparse (they are the ones that have
an elog(ERROR) rather than a "command = NULL"). Patch 0006 adds calls
in ProcessUtilitySlow() to each command, so that the object(s) being
touched are added to the event trigger command stash.

Patches from 0007 to 0027 (excepting patch 0017) implement one or a
small number of commands in deparse. Patch 0017 is necessary
infrastructure in ALTER TABLE to support deparsing that one.

My intention with the later patches is that they would all be pushed as
a single commit, i.e. the deparse support would be implemented for all
commands in a fell swoop rather than piecemeal -- except possibly patch
0017 (the ALTER TABLE infrastructure). I split them up only for ease of
review. Of course, before pushing we (I) need to implement deparsing
for all the remaining commands.

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

On 2014-09-25 18:59:31 -0300, Alvaro Herrera wrote:

/* tid.c */
diff --git a/src/include/utils/ruleutils.h b/src/include/utils/ruleutils.h
new file mode 100644
index 0000000..520b066
--- /dev/null
+++ b/src/include/utils/ruleutils.h
@@ -0,0 +1,34 @@
+/*-------------------------------------------------------------------------
+ *
+ * ruleutils.h
+ *		Declarations for ruleutils.c
+ *
+ * Portions Copyright (c) 1996-2014, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/include/ruleutils.h
+ *
+ *-------------------------------------------------------------------------
+ */
+#ifndef RULEUTILS_H
+#define RULEUTILS_H
+
+#include "nodes/nodes.h"
+#include "nodes/parsenodes.h"
+#include "nodes/pg_list.h"
+
+
+extern char *pg_get_indexdef_string(Oid indexrelid);
+extern char *pg_get_indexdef_columns(Oid indexrelid, bool pretty);
+
+extern char *pg_get_constraintdef_string(Oid constraintId);
+extern char *deparse_expression(Node *expr, List *dpcontext,
+				   bool forceprefix, bool showimplicit);
+extern List *deparse_context_for(const char *aliasname, Oid relid);
+extern List *deparse_context_for_planstate(Node *planstate, List *ancestors,
+							  List *rtable, List *rtable_names);
+extern List *select_rtable_names_for_explain(List *rtable,
+								Bitmapset *rels_used);
+extern char *generate_collation_name(Oid collid);
+
+#endif	/* RULEUTILS_H */
-- 
1.9.1

I wondered for a minute whether any of these are likely to cause
problems for code just including builtins.h - but I think there will be
sufficiently few callers for them to make that not much of a concern.

From 5e3555058a1bbb93e25a3a104c26e6b96dce994d Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 25 Sep 2014 16:34:50 -0300
Subject: [PATCH 02/27] deparse/core: have RENAME return attribute number

Looks "harmless". I.e. +1.

From 72c4d0ef875f9e9b0f3b424499738fd1bd946c32 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Fri, 9 May 2014 18:32:23 -0400
Subject: [PATCH 03/27] deparse/core: event triggers support GRANT/REVOKE

Hm. The docs don't mention whether these only work for database local
objects or also shared objects. Generally event trigger are only
triggered for the former... But the code doesn't seem to make a
difference?

From 5bb35d2e51fe6c6e219fe5cf7ddbab5423e6be1b Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 25 Sep 2014 15:44:44 -0300
Subject: [PATCH 04/27] deparse/core: event triggers support COMMENT

---
doc/src/sgml/event-trigger.sgml | 8 +++++++-
src/backend/commands/comment.c | 5 ++++-
src/backend/commands/event_trigger.c | 1 +
src/backend/tcop/utility.c | 21 +++++++++++++++++----
src/include/commands/comment.h | 2 +-
5 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/doc/src/sgml/event-trigger.sgml b/doc/src/sgml/event-trigger.sgml
index 49b8384..39ecd94 100644
--- a/doc/src/sgml/event-trigger.sgml
+++ b/doc/src/sgml/event-trigger.sgml
@@ -37,7 +37,7 @@
<para>
The <literal>ddl_command_start</> event occurs just before the
execution of a <literal>CREATE</>, <literal>ALTER</>, <literal>DROP</>,
-     <literal>GRANT</> or <literal>REVOKE</>
+     <literal>COMMENT</>, <literal>GRANT</> or <literal>REVOKE</>
command.  No check whether the affected object exists or doesn't exist is
performed before the event trigger fires.
As an exception, however, this event does not occur for
@@ -281,6 +281,12 @@
<entry align="center"><literal>-</literal></entry>
</row>
<row>
+        <entry align="left"><literal>COMMENT</literal></entry>
+        <entry align="center"><literal>X</literal></entry>
+        <entry align="center"><literal>X</literal></entry>
+        <entry align="center"><literal>-</literal></entry>
+       </row>
+       <row>

Hm. No drop support because COMMENTs are odd and use odd NULL
semantics. Fair enough.

From 098f5acabd774004dc5d9c750d55e7c9afa60238 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Wed, 24 Sep 2014 15:53:04 -0300
Subject: [PATCH 05/27] deparse: infrastructure needed for command deparsing

---
src/backend/catalog/objectaddress.c | 115 +++++
src/backend/commands/event_trigger.c | 941 ++++++++++++++++++++++++++++++++++-
src/backend/tcop/Makefile | 2 +-
src/backend/tcop/deparse_utility.c | 877 ++++++++++++++++++++++++++++++++
src/backend/tcop/utility.c | 2 +
src/backend/utils/adt/format_type.c | 113 ++++-
src/include/catalog/objectaddress.h | 2 +
src/include/catalog/pg_proc.h | 4 +
src/include/commands/event_trigger.h | 3 +
src/include/commands/extension.h | 2 +-
src/include/nodes/parsenodes.h | 2 +
src/include/tcop/deparse_utility.h | 60 +++
src/include/utils/builtins.h | 5 +
13 files changed, 2117 insertions(+), 11 deletions(-)
create mode 100644 src/backend/tcop/deparse_utility.c
create mode 100644 src/include/tcop/deparse_utility.h

diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index b69b75b..a2445f1 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -723,6 +723,121 @@ get_object_address(ObjectType objtype, List *objname, List *objargs,
}

/*
+ * Return the OID of the catalog corresponding to the given object type
+ */
+Oid
+get_objtype_catalog_oid(ObjectType objtype)
+{

+		case OBJECT_ATTRIBUTE:
+			catalog_id = TypeRelationId;	/* XXX? */
+			break;

I'm indeed not clear why thats a meaningful choice?

+/* XXX merge this with ObjectTypeMap? */

hm. I think that's obsolete by now?

@@ -924,6 +929,7 @@ EventTriggerSupportsObjectType(ObjectType obtype)
case OBJECT_ATTRIBUTE:
case OBJECT_CAST:
case OBJECT_COLUMN:
+ case OBJECT_COMPOSITE:

Is it actually correct that we return false here up to now? I.e. isn't
this a bug?

+/*
+ * EventTriggerStashCommand
+ * 		Save data about a simple DDL command that was just executed
+ */
+void
+EventTriggerStashCommand(Oid objectId, uint32 objectSubId, ObjectType objtype,
+						 Node *parsetree)
+{
+	MemoryContext oldcxt;
+	StashedCommand *stashed;
+
+	oldcxt = MemoryContextSwitchTo(currentEventTriggerState->cxt);
+
+	stashed = palloc(sizeof(StashedCommand));
+
+	stashed->type = SCT_Simple;

I'm not really sure why that subtype is called 'simple'?

+Datum
+pg_event_trigger_get_creation_commands(PG_FUNCTION_ARGS)
+{
+	ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
+	TupleDesc	tupdesc;
+	Tuplestorestate *tupstore;
+	MemoryContext per_query_ctx;
+	MemoryContext oldcontext;
+	ListCell   *lc;
+
+	/*
+	 * Protect this function from being called out of context
+	 */
+	if (!currentEventTriggerState)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("%s can only be called in an event trigger function",
+						"pg_event_trigger_get_creation_commands()")));

I wonder wether we shouldn't introduce ERRCODE_WRONG_CONTEXT or
such. Seems useful here and in a fair number of other situations. But
not really required, just something that annoyed me before.

+	foreach(lc, currentEventTriggerState->stash)
+	{
+		StashedCommand *cmd = lfirst(lc);
+		char	   *command;
+
+		/*
+		 * For IF NOT EXISTS commands that attempt to create an existing
+		 * object, the returned OID is Invalid; in those cases, return an empty
+		 * command instead of trying to soldier on.
+		 *
+		 * XXX an alternative would be to look up the Oid of the existing
+		 * object and run the deparse with that.  But since the parse tree
+		 * might be different from the one that created the object in the first
+		 * place, we might not end up in a consistent state anyway.
+		 */
+		if (cmd->type == SCT_Simple &&
+			!OidIsValid(cmd->d.simple.objectId))
+			continue;

I'd replace the XXX by 'One might think that'. You've perfectly
explained why that's not a viable alternative.

+		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);
+					}
+				}
+
+				/* classid */
+				values[i++] = ObjectIdGetDatum(addr.classId);
+				/* objid */
+				values[i++] = ObjectIdGetDatum(addr.objectId);
+				/* objsubid */
+				values[i++] = Int32GetDatum(addr.objectSubId);
+				/* command tag */
+				values[i++] = CStringGetTextDatum(tag);
+				/* object_type */
+				values[i++] = CStringGetTextDatum(type);
+				/* schema */
+				if (schema == NULL)
+					nulls[i++] = true;
+				else
+					values[i++] = CStringGetTextDatum(schema);
+				/* identity */
+				values[i++] = CStringGetTextDatum(identity);
+				/* in_extension */
+				values[i++] = BoolGetDatum(cmd->in_extension);
+				/* command */
+				values[i++] = CStringGetTextDatum(command);
+			}
+
+			tuplestore_putvalues(tupstore, tupdesc, values, nulls);

So: a) we only support SCT_Simple here. b) we add commands whether
they're supported or not? And afaics the only way to discern that is by
looking at 'schema'? If we're not skipping unsupported stuff, shouldn't
we at least set a column indicating that?

+/* ************************* JSON STUFF FROM HERE ************************* *
+ *	Code below is used to decode blobs returned by deparse_utility_command	*
+ *																			*/
+
+/*
+ * Note we only support types that are valid in command representation from
+ * deparse_utility_command.
+ */
+typedef enum
+{
+	JsonTypeArray,
+	JsonTypeObject,
+	JsonTypeString
+} JsonType;

There indeed seems to be no existing enum/define for this. Odd.

+typedef enum
+{
+	SpecTypename,
+	SpecOperatorname,
+	SpecDottedName,
+	SpecString,
+	SpecStringLiteral,
+	SpecIdentifier
+} convSpecifier;

Ok, these are all pretty specific to the usecase.

+/*
+ * Extract the named json field, which must be of type string, from the given
+ * JSON datum, which must be of type object.  If the field doesn't exist,
+ * NULL is returned.  Otherwise the string value is returned.
+ */
+static char *
+expand_get_strval(Datum json, char *field_name)
+{
+	FunctionCallInfoData fcinfo;
+	Datum		result;
+	char	   *value_str;
+
+	InitFunctionCallInfoData(fcinfo, NULL, 2, InvalidOid, NULL, NULL);
+
+	fcinfo.arg[0] = json;
+	fcinfo.argnull[0] = false;
+	fcinfo.arg[1] = CStringGetTextDatum(field_name);
+	fcinfo.argnull[1] = false;
+
+	result = (*json_object_field_text) (&fcinfo);

I've not looked it up, but is it actually legal to call functions
without flinfo setup? It seems nicer to revamp this to use
FunctionCallInfoInvoke() rather than doing it ourselves.

+/*
+ * Given a JSON value, return its type.
+ *
+ * We return both a JsonType (for easy control flow), and a string name (for
+ * error reporting).
+ */
+static JsonType
+jsonval_get_type(Datum jsonval, char **typename)
+{
+	JsonType	json_elt_type;
+	Datum		paramtype_datum;
+	char	   *paramtype;
+
+	paramtype_datum = DirectFunctionCall1(json_typeof, jsonval);
+	paramtype = TextDatumGetCString(paramtype_datum);

So, here you suddenly use a different method of invoking functions?

+	if (strcmp(paramtype, "array") == 0)
+		json_elt_type = JsonTypeArray;
+	else if (strcmp(paramtype, "object") == 0)
+		json_elt_type = JsonTypeObject;
+	else if (strcmp(paramtype, "string") == 0)
+		json_elt_type = JsonTypeString;
+	else
+		/* XXX improve this; need to specify array index or param name */
+		elog(ERROR, "unexpected JSON element type %s",
+			 paramtype);

What about that XXX? Seems like the current usage allows that to be
something for the future?

+/*
+ * dequote_jsonval
+ *		Take a string value extracted from a JSON object, and return a copy of it
+ *		with the quoting removed.
+ *
+ * Another alternative to this would be to run the extraction routine again,
+ * using the "_text" variant which returns the value without quotes; but this
+ * is expensive, and moreover it complicates the logic a lot because not all
+ * values are extracted in the same way (some are extracted using
+ * json_object_field, others using json_array_element).  Dequoting the object
+ * already at hand is a lot easier.
+ */
+static char *
+dequote_jsonval(char *jsonval)
+{
+	char	   *result;
+	int			inputlen = strlen(jsonval);
+	int			i;
+	int			j = 0;
+
+	result = palloc(strlen(jsonval) + 1);
+
+	/* skip the start and end quotes right away */
+	for (i = 1; i < inputlen - 1; i++)
+	{
+		if (jsonval[i] == '\\')
+		{
+			i++;
+
+			/* This uses same logic as json.c */
+			switch (jsonval[i])
+			{
+				case 'b':
+					result[j++] = '\b';
+					continue;
+				case 'f':
+					result[j++] = '\f';
+					continue;
+				case 'n':
+					result[j++] = '\n';
+					continue;
+				case 'r':
+					result[j++] = '\r';
+					continue;
+				case 't':
+					result[j++] = '\t';
+					continue;
+				case '"':
+				case '\\':
+				case '/':
+					break;
+				default:
+					/* XXX: ERROR? */
+					break;
+			}
+		}
+
+		result[j++] = jsonval[i];
+	}
+	result[j] = '\0';
+
+	return result;
+}

This looks like something that really should live at least partially in json.c?

+/*
+ * Expand a json value as a dotted-name.  The value must be of type object
+ * and must contain elements "schemaname" (optional), "objname" (mandatory),
+ * "attrname" (optional).
+ *
+ * XXX do we need a "catalogname" as well?
+ */

I'd replace the XXX by 'One day we might need "catalogname" as well, but
no current ..."

+static void
+expand_jsonval_dottedname(StringInfo buf, Datum jsonval)
+{
+	char	   *schema;
+	char	   *objname;
+	char	   *attrname;
+	const char *qschema;
+	const char *qname;
+
+	schema = expand_get_strval(jsonval, "schemaname");
+	objname = expand_get_strval(jsonval, "objname");
+	if (objname == NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid NULL object name in %%D element")));
+	qname = quote_identifier(objname);
+	if (schema == NULL)
+	{
+		appendStringInfo(buf, "%s", qname);

*appendStringInfoString().

+	}
+	else
+	{
+		qschema = quote_identifier(schema);
+		appendStringInfo(buf, "%s.%s",
+						 qschema, qname);
+		if (qschema != schema)
+			pfree((char *) qschema);
+		pfree(schema);
+	}
+
+	attrname = expand_get_strval(jsonval, "attrname");
+	if (attrname)
+	{
+		const char *qattr;
+
+		qattr = quote_identifier(attrname);
+		appendStringInfo(buf, ".%s", qattr);
+		if (qattr != attrname)
+			pfree((char *) qattr);
+		pfree(attrname);
+	}

Hm. Does specifying schema, object and attrname result ins oemthing
valid with this? Might not ever be required, but in that case it should
probably error out

+/*
+ * expand a json value as a type name.
+ */
+static void
+expand_jsonval_typename(StringInfo buf, Datum jsonval)
+{
+	char	   *schema = NULL;
+	char	   *typename;
+	char	   *typmodstr;
+	bool		array_isnull;
+	bool		is_array;
+
+	typename = expand_get_strval(jsonval, "typename");
+	if (typename == NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid NULL type name in %%T element")));
+	typmodstr = expand_get_strval(jsonval, "typmod");	/* OK if null */
+	is_array = expand_get_boolval(jsonval, "is_array", &array_isnull);
+	schema = expand_get_strval(jsonval, "schemaname");
+
+	/*
+	 * If schema is NULL, then don't schema qualify, but do quote the type
+	 * name; if the schema is empty instead, then we don't quote the type name.
+	 * This is our (admittedly quite ugly) way of dealing with type names that
+	 * might require special treatment.
+	 */

Indeed, really quite ugly. What's it needed for? Maybe reference the
function that needs it to explain.

+
+/*
+ * Expand a json value as an operator name
+ */
+static void
+expand_jsonval_operator(StringInfo buf, Datum jsonval)
+{
+	char	   *schema = NULL;
+	char	   *operator;
+
+	operator = expand_get_strval(jsonval, "objname");
+	if (operator == NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid NULL operator name in %%O element")));
+	schema = expand_get_strval(jsonval, "schemaname");
+
+	/* schema might be NULL or empty */

Why can it be empty here?

+	if (schema == NULL || schema[0] == '\0')
+		appendStringInfo(buf, "%s", operator);
+	else
+		appendStringInfo(buf, "%s.%s",
+						 quote_identifier(schema),
+						 operator);
+}

+/*
+ * Expand a json value as a string.  The value must be of type string or of
+ * type object, in which case it must contain a "fmt" element which will be
+ * recursively expanded; also, if the object contains an element "present"
+ * and it is set to false, the expansion is the empty string.
+ */
+static void
+expand_jsonval_string(StringInfo buf, Datum jsonval, JsonType json_elt_type)
+{
+	if (json_elt_type == JsonTypeString)
+	{
+		char	   *str;
+		char	   *unquoted;
+
+		str = TextDatumGetCString(jsonval);
+		unquoted = dequote_jsonval(str);
+		appendStringInfo(buf, "%s", unquoted);
+		pfree(str);
+		pfree(unquoted);
+	}
+	else if (json_elt_type == JsonTypeObject)
+	{
+		bool		present;
+		bool		isnull;
+		present = expand_get_boolval(jsonval, "present", &isnull);

Nitpick, but I find isnull slightly confusing as a name here.

+		if (isnull || present)
+		{
+			Datum		inner;
+			char	   *str;
+
+			inner = DirectFunctionCall1(pg_event_trigger_expand_command,
+										jsonval);
+			str = TextDatumGetCString(inner);
+
+			appendStringInfoString(buf, str);
+			pfree(DatumGetPointer(inner));
+			pfree(str);
+		}
+	}
+}

+/*
+ * Expand a json value as a string literal
+ */
+static void
+expand_jsonval_strlit(StringInfo buf, Datum jsonval)
+{
+	char   *str;
+	char   *unquoted;
+	StringInfoData dqdelim;
+	static const char dqsuffixes[] = "_XYZZYX_";
+	int         dqnextchar = 0;
+
+	/* obtain the string, and remove the JSON quotes and stuff */
+	str = TextDatumGetCString(jsonval);
+	unquoted = dequote_jsonval(str);
+
+	/* easy case: if there are no ' and no \, just use a single quote */
+	if (strchr(unquoted, '\'') == NULL &&
+		strchr(unquoted, '\\') == NULL)
+	{
+		appendStringInfo(buf, "'%s'", unquoted);
+		return;
+	}
+
+	/* Find a useful dollar-quote delimiter */
+	initStringInfo(&dqdelim);
+	appendStringInfoString(&dqdelim, "$");
+	while (strstr(unquoted, dqdelim.data) != NULL)
+	{
+		appendStringInfoChar(&dqdelim, dqsuffixes[dqnextchar++]);
+		dqnextchar %= sizeof(dqsuffixes) - 1;
+	}
+	/* add trailing $ */
+	appendStringInfoChar(&dqdelim, '$');
+
+	/* And finally produce the quoted literal into the output StringInfo */
+	appendStringInfo(buf, "%s%s%s", dqdelim.data, unquoted, dqdelim.data);
+}

Ugh. Do we really need this logic here? Doesn't that already exist
somewhere?

+/*
+ * Expand one json element according to rules.
+ */

What are rules?

+static void
+expand_one_element(StringInfo buf, char *param,
+				   Datum jsonval, char *valtype, JsonType json_elt_type,
+				   convSpecifier specifier)
+{
+	/*
+	 * Validate the parameter type.  If dotted-name was specified, then a JSON
+	 * object element is expected; if an identifier was specified, then a JSON
+	 * string is expected.	If a string was specified, then either a JSON
+	 * object or a string is expected.
+	 */
+	if (specifier == SpecDottedName && json_elt_type != JsonTypeObject)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON object for %%D element \"%s\", got %s",
+						param, valtype)));
+	if (specifier == SpecTypename && json_elt_type != JsonTypeObject)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON object for %%T element \"%s\", got %s",
+						param, valtype)));
+	if (specifier == SpecOperatorname && json_elt_type != JsonTypeObject)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON object for %%O element \"%s\", got %s",
+						param, valtype)));
+	if (specifier == SpecIdentifier && json_elt_type != JsonTypeString)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON string for %%I element \"%s\", got %s",
+						param, valtype)));
+	if (specifier == SpecStringLiteral && json_elt_type != JsonTypeString)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON string for %%L element \"%s\", got %s",
+						param, valtype)));
+	if (specifier == SpecString &&
+		json_elt_type != JsonTypeString && json_elt_type != JsonTypeObject)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("expected JSON string or object for %%s element \"%s\", got %s",
+						param, valtype)));

Isn't this essentially a duplication of the switch below?

+	switch (specifier)
+	{
+		case SpecIdentifier:
+			expand_jsonval_identifier(buf, jsonval);
+			break;
+
+		case SpecDottedName:
+			expand_jsonval_dottedname(buf, jsonval);
+			break;
+
+		case SpecString:
+			expand_jsonval_string(buf, jsonval, json_elt_type);
+			break;
+
+		case SpecStringLiteral:
+			expand_jsonval_strlit(buf, jsonval);
+			break;
+
+		case SpecTypename:
+			expand_jsonval_typename(buf, jsonval);
+			break;
+
+		case SpecOperatorname:
+			expand_jsonval_operator(buf, jsonval);
+			break;
+	}
+}
+
+/*
+ * Expand one JSON array element according to rules.
+ */

The generic "rules" again...

+#define ADVANCE_PARSE_POINTER(ptr,end_ptr) \
+	do { \
+		if (++(ptr) >= (end_ptr)) \
+			ereport(ERROR, \
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE), \
+					 errmsg("unterminated format specifier"))); \
+	} while (0)
+
+/*------
+ * 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)
+ *
+ * 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.
+ *
+ * XXX the current implementation works fine, but is likely to be slow: for
+ * each element found in the fmt string we parse the JSON object once.	It
+ * might be better to use jsonapi.h directly so that we build a hash or tree of
+ * elements and their values once before parsing the fmt string, and later scan
+ * fmt using the tree.
+ *------
+ */
+Datum
+pg_event_trigger_expand_command(PG_FUNCTION_ARGS)
+{
+	text	   *json = PG_GETARG_TEXT_P(0);
+	char	   *fmt_str;
+	int			fmt_len;
+	const char *cp;
+	const char *start_ptr;
+	const char *end_ptr;
+	StringInfoData str;
+
+	fmt_str = expand_get_strval(PointerGetDatum(json), "fmt");
+	if (fmt_str == NULL)
+		ereport(ERROR,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("invalid NULL format string")));
+	fmt_len = strlen(fmt_str);
+
+	start_ptr = fmt_str;
+	end_ptr = start_ptr + fmt_len;
+	initStringInfo(&str);
+
+	for (cp = start_ptr; cp < end_ptr; cp++)
+	{

I haven't checked, but I'm not sure the code here handles multibyte
stuff correctly. IIRC it's legal for multibyte characters to appear
directly in json?

+		/*
+		 * Obtain the element to be expanded.  Note we cannot use
+		 * DirectFunctionCall here, because the element might not exist.
+		 */

"and the return value thus could be NULL."

+/*
+ * Append a string parameter to a tree.
+ *
+ * Note: we don't pstrdup the source string.  Caller must ensure the
+ * source string lives long enough.
+ */
+static void

Why this difference to most other functions?

+/* Allocate a new object parameter */
+static ObjElem *
+new_object_object(char *name, ObjTree *value)
+{
+	ObjElem    *param;
+
+	param = palloc0(sizeof(ObjElem));
+
+	param->name = name ? pstrdup(name) : NULL;
+	param->objtype = ObjTypeObject;
+	param->obj_value = value;	/* XXX not duped */

Aha? Why? Because it's freshly allocated anyhow? In which case the XXX
doesn't seem warranted?

+/*
+ * Create a JSON blob from our ad-hoc representation.
+ *
+ * Note this leaks some memory; caller is responsible for later clean up.
+ *
+ * XXX this implementation will fail if there are more JSON objects in the tree
+ * than the maximum number of columns in a heap tuple.  To fix we would first call
+ * construct_md_array and then json_object.
+ */

I have no idea what that XXX is supposed to say to me. Note that there's
no construct_md_array in the patchset.

+static char *
+jsonize_objtree(ObjTree *tree)
+{

+	slist_foreach(iter, &tree->params)
+	{
+
+		nulls[i - 1] = false;
+		switch (object->objtype)
+		{
+			case ObjTypeNull:
+				nulls[i - 1] = true;
+				break;
+			case ObjTypeBool:
+				values[i - 1] = BoolGetDatum(object->bool_value);
+				break;
+			case ObjTypeString:
+				values[i - 1] = CStringGetTextDatum(object->str_value);
+				break;
+			case ObjTypeArray:
+				{
+					ArrayType  *arrayt;
+					Datum	   *arrvals;
+					Datum		jsonary;
+					ListCell   *cell;
+					int			length = list_length(object->array_value);
+					int			j;
+
+					/*
+					 * Arrays are stored as Lists up to this point, with each
+					 * element being a ObjElem; we need to construct an
+					 * ArrayType with them to turn the whole thing into a JSON
+					 * array.
+					 */
+					j = 0;
+					arrvals = palloc(sizeof(Datum) * length);
+					foreach(cell, object->array_value)
+					{
+						ObjElem    *elem = lfirst(cell);
+
+						switch (elem->objtype)
+						{
+							case ObjTypeString:
+								arrvals[j] =
+									/*
+									 * XXX need quotes around the value.  This
+									 * needs to be handled differently because
+									 * it will fail for values of anything but
+									 * trivial complexity.
+									 */
+									CStringGetTextDatum(psprintf("\"%s\"",
+																 elem->str_value));
+								break;

Didn't you earlier add a function doing most of this? That XXX looks
like something that definitely needs to be addressed before the commit.

From ea8997de828931e954e1a52821ffc834370859d2 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Thu, 25 Sep 2014 15:50:37 -0300
Subject: [PATCH 06/27] deparse: sprinkle EventTriggerStashCommand() calls

Seems like a odd separate commit ;). What's the point of not squashing
it?

XXX: I'm now too tired to continue honestly. Man, that's a fair amount
of code... Will try to continue it. Once I've made it through once I
hopefully also give some sensible higher level comments.

Greetings,

Andres Freund

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Sep 26, 2014 at 6:59 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Actually here's a different split of these patches, which I hope makes
more sense. My intention here is that patches 0001 to 0004 are simple
changes that can be pushed right away; they are not directly related to
the return-creation-command feature. Patches 0005 to 0027 implement
that feature incrementally. You can see in patch 0005 the DDL commands
that are still not implemented in deparse (they are the ones that have
an elog(ERROR) rather than a "command = NULL"). Patch 0006 adds calls
in ProcessUtilitySlow() to each command, so that the object(s) being
touched are added to the event trigger command stash.

Patches from 0007 to 0027 (excepting patch 0017) implement one or a
small number of commands in deparse. Patch 0017 is necessary
infrastructure in ALTER TABLE to support deparsing that one.

My intention with the later patches is that they would all be pushed as
a single commit, i.e. the deparse support would be implemented for all
commands in a fell swoop rather than piecemeal -- except possibly patch
0017 (the ALTER TABLE infrastructure). I split them up only for ease of
review. Of course, before pushing we (I) need to implement deparsing
for all the remaining commands.

Thanks for the update. This stuff is still on my TODO list and I was
planning to look at it at some extent today btw :) Andres has already
sent some comments (20140925235724.GH16581@awork2.anarazel.de) though,
I'll try to not overlap with what has already been mentioned.

Patch 1: I still like this patch as it gives a clear separation of the
built-in functions and the sub-functions of ruleutils.c that are
completely independent. Have you considered adding the external
declaration of quote_all_identifiers as well? It is true that this
impacts extensions (some of my stuff as well), but my point is to bite
the bullet and make the separation cleaner between builtins.h and
ruleutils.h. Attached is a patch that can be applied on top of patch 1
doing so... Feel free to discard for the potential breakage this would
create though.

Patch 2:
1) Declarations of renameatt are inconsistent:
@tablecmds.c:
-renameatt(RenameStmt *stmt)
+renameatt(RenameStmt *stmt, int32 *objsubid)
@tablecmds.h:
-extern Oid     renameatt(RenameStmt *stmt);
+extern Oid     renameatt(RenameStmt *stmt, int *attnum);
Looking at this code, for correctness renameatt should use everywhere
"int *attnum" and ExecRenameStmt should use "int32 *subobjid".
2) Just a smallish picky formatting remark, I would have done that on
a single line:
+       attnum =
+               renameatt_internal(relid,
3) in ExecRenameStmt, I think that you should set subobjid for
aggregate, collations, fdw, etc. There is an access to ObjectAddress
so that's easy to set using address.objectSubId.
4) This comment is misleading, as for an attribute what is returned is
actually an attribute number:
+ * Return value is the OID of the renamed object.  The objectSubId, if any,
+ * is returned in objsubid.
  */
5) Perhaps it is worth mentioning at the top of renameatt that the
attribute number is returned as well?

Patch 3: Looks fine, a bit of testing is showing up that this works as expected:
=# CREATE ROLE foo;
CREATE ROLE
=# CREATE TABLE aa (a int);
CREATE TABLE
=# CREATE OR REPLACE FUNCTION abort_grant()
RETURNS event_trigger AS $$
BEGIN
RAISE NOTICE 'Event: %', tg_event;
RAISE EXCEPTION 'Execution of command % forbidden', tg_tag;
END;
$$ LANGUAGE plpgsql;
CREATE FUNCTION
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_start
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# GRANT SELECT ON aa TO foo;
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command GRANT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
=# DROP EVENT TRIGGER abort_grant_trigger;
DROP EVENT TRIGGER
=# CREATE EVENT TRIGGER abort_grant_trigger ON ddl_command_end
WHEN TAG IN ('GRANT', 'REVOKE') EXECUTE PROCEDURE abort_grant();
CREATE EVENT TRIGGER
=# REVOKE SELECT ON aa FROM foo;
NOTICE: 00000: Event: ddl_command_end
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command REVOKE forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068

Patch 4: Shouldn't int32 be used instead of uint32 in the declaration
of CommentObject? And yes, adding support for only ddl_command_start
and ddl_command_end is enough. Let's not play with dropped objects in
this area... Similarly to the test above:
=# comment on table aa is 'test';
NOTICE: 00000: Event: ddl_command_start
LOCATION: exec_stmt_raise, pl_exec.c:3068
ERROR: P0001: Execution of command COMMENT forbidden
LOCATION: exec_stmt_raise, pl_exec.c:3068
Regards,
--
Michael

Alvaro Herrera wrote:

Actually here's a different split of these patches, which I hope makes
more sense. My intention here is that patches 0001 to 0004 are simple
changes that can be pushed right away; they are not directly related to
the return-creation-command feature. Patches 0005 to 0027 implement
that feature incrementally. You can see in patch 0005 the DDL commands
that are still not implemented in deparse (they are the ones that have
an elog(ERROR) rather than a "command = NULL"). Patch 0006 adds calls
in ProcessUtilitySlow() to each command, so that the object(s) being
touched are added to the event trigger command stash.

Patches from 0007 to 0027 (excepting patch 0017) implement one or a
small number of commands in deparse. Patch 0017 is necessary
infrastructure in ALTER TABLE to support deparsing that one.

My intention with the later patches is that they would all be pushed as
a single commit, i.e. the deparse support would be implemented for all
commands in a fell swoop rather than piecemeal -- except possibly patch
0017 (the ALTER TABLE infrastructure). I split them up only for ease of
review. Of course, before pushing we (I) need to implement deparsing
for all the remaining commands.

Here's a new version of this series. The main change is that I've
changed deparse_utility.c to generate JSON, and the code that was in
commands/event_trigger.c to decode that JSON, so that it uses the new
Jsonb API instead. In addition, I've moved the new code that was in
commands/event_trigger.c to utils/adt/ddl_json.c. (The only entry point
of the new file is the SQL-callable pg_event_trigger_expand_command()
function, and its purpose is to expand a JSON object emitted by the
deparse_utility.c code back into a plain text SQL command.)

I have also cleaned up the code per comments from Michael Paquier and
Andres Freund:

* the GRANT support for event triggers now correctly ignores global
objects.

* COMMENT ON .. IS NULL no longer causes a crash

* renameatt() and ExecRenameStmt are consistent in returning the
objSubId as an "int" (not int32). This is what is used as objectSubId
in ObjectAddress, which is what we're using this value for.

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

On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Here's a new version of this series. The main change is that I've
changed deparse_utility.c to generate JSON, and the code that was in
commands/event_trigger.c to decode that JSON, so that it uses the new
Jsonb API instead. In addition, I've moved the new code that was in
commands/event_trigger.c to utils/adt/ddl_json.c. (The only entry point
of the new file is the SQL-callable pg_event_trigger_expand_command()
function, and its purpose is to expand a JSON object emitted by the
deparse_utility.c code back into a plain text SQL command.)
I have also cleaned up the code per comments from Michael Paquier and
Andres Freund:

* the GRANT support for event triggers now correctly ignores global
objects.

* COMMENT ON .. IS NULL no longer causes a crash

Why would this not fail? Patch 3 in this set is identical to the last
one. I tested that and it worked well...

* renameatt() and ExecRenameStmt are consistent in returning the
objSubId as an "int" (not int32). This is what is used as objectSubId
in ObjectAddress, which is what we're using this value for.

In patch 1, ExecRenameStmt returns objsubid for an attribute name when
rename is done on an attribute. Now could you clarify why we skip this
list of objects even if their sub-object ID is available with
address.objectSubId?
case OBJECT_AGGREGATE:
case OBJECT_COLLATION:
case OBJECT_CONVERSION:
case OBJECT_EVENT_TRIGGER:
case OBJECT_FDW:
case OBJECT_FOREIGN_SERVER:
case OBJECT_FUNCTION:
case OBJECT_OPCLASS:
case OBJECT_OPFAMILY:
case OBJECT_LANGUAGE:
case OBJECT_TSCONFIGURATION:
case OBJECT_TSDICTIONARY:
case OBJECT_TSPARSER:
case OBJECT_TSTEMPLATE:

Patch 2 fails on make check for the tests privileges and foreign_data
by showing up double amount warnings for some object types:
*** /Users/ioltas/git/postgres/src/test/regress/expected/privileges.out
Fri Oct 10 14:34:10 2014
--- /Users/ioltas/git/postgres/src/test/regress/results/privileges.out
 Thu Oct 16 15:47:42 2014
***************
*** 118,123 ****
--- 118,124 ----
  ERROR:  permission denied for relation atest2
  GRANT ALL ON atest1 TO PUBLIC; -- fail
  WARNING:  no privileges were granted for "atest1"
+ WARNING:  no privileges were granted for "atest1"
  -- checks in subquery, both ok
  SELECT * FROM atest1 WHERE ( b IN ( SELECT col1 FROM atest2 ) );
   a | b
EventTriggerSupportsGrantObjectType is fine to remove
ACL_OBJECT_DATABASE and ACL_OBJECT_TABLESPACE from the list of
supported objects. That's as well in line with the current firing
matrix. I think that it would be appropriate to add a comment on top
of this function.

Patch 3 looks good.
Regards,
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 16, 2014 at 4:01 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Here's a new version of this series.

Here is some input on patch 4:

1) Use of OBJECT_ATTRIBUTE:
+               case OBJECT_ATTRIBUTE:
+                       catalog_id = TypeRelationId;    /* XXX? */
+                       break;
I think that the XXX mark could be removed, using TypeRelationId is correct
IMO as OBJECT ATTRIBUTE is used when managing an attribute object, which is
a custom type used for CREATE/ALTER TYPE.
2) This patch is showing up many warnings, among them:
event_trigger.c:1460:20: note: uninitialized use occurs here
                                addr.classId = classId;
                                               ^~~~~~~
event_trigger.c:1446:21: note: initialize the variable 'objSubId' to
silence this warning
                                uint32          objSubId;
                                                        ^
                                                         = 0
Or:
deparse_utility.c:301:1: warning: unused function 'append_object_object'
[-Wunused-function]
append_object_object(ObjTree *tree, char *name, ObjTree *value)
^
In the 2nd case though I imagine that those functions in deparse_utility.c
are used afterwards... There are a couple of other warning related to
SCT_Simple but that's normal as long as 17 or 24 are not combined with it.
3) What's that actually?
+/* XXX merge this with ObjectTypeMap? */
 static event_trigger_support_data event_trigger_support[] = {
We may be able to do something smarter with event_trigger_support[], but as
this consists in a mapping of subcommands associated with CREATE/DROP/ALTER
and is only a reverse engineering operation of somewhat
AlterObjectTypeCommandTag  or CreateCommandTag, I am not sure how you could
merge that. Some input perhaps?
4)
+/*
+ * EventTriggerStashCommand
+ *             Save data about a simple DDL command that was just executed
+ */
Shouldn't this be "single" instead of "simple"?
5) I think that SCT_Simple should be renamed as SCT_Single
+typedef enum StashedCommandType
+{
+       SCT_Simple,
+} StashedCommandType;
This comment holds as well for deparse_simple_command.
6)
+               command = deparse_utility_command(cmd);
+
+               /*
+                * Some parse trees return NULL when deparse is attempted;
we don't
+                * emit anything for them.
+                */
+               if (command != NULL)
+               {
Small detail, but you may here just use a continue to make the code a bit
more readable after deparsing the command.
7) pg_event_trigger_get_creation_commands is modified as well in patches 17
and 24. You may as well use an enum on cmd->type.
8) Rejoining a comment done earlier by Andres, it would be nice to have
ERRCODE_WRONG_CONTEXT (unrelated to this patch).
ERRCODE_FEATURE_NOT_SUPPORTED seems rather a different error type...
9) Those declarations are not needed in event_trigger.c:
+#include "utils/json.h"
+#include "utils/jsonb.h"
10) Would you mind explaining what means "fmt"?
+ * 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.
11) deparse_utility.c mentions here and there JSON objects, but what is
created are JSONB objects. I'd rather be clear here.
12) Already mentioned before, but this reverse engineering machine for
types would be more welcome as a set of functions in lsyscache (one for
namespace, one for type name and one for is_array). For typemodstr the need
is different as it uses printTypmod.
+void
+format_type_detailed(Oid type_oid, int32 typemod,
+                                        Oid *nspid, char **typname, char
**typemodstr,
+                                        bool *is_array)

13) This change seems unrelated to this patch...
-       int                     type = 0;
+       JsonbIteratorToken type = WJB_DONE;
        JsonbValue      v;
        int                     level = 0;
        bool            redo_switch = false;
@@ -454,7 +454,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");
14) This could already be pushed as a separate commit:
-extern bool creating_extension;
+extern PGDLLIMPORT bool creating_extension;

A couple of comments: this patch introduces a basic infrastructure able to
do the following set of operations:
- Obtention of parse tree using StashedCommand
- Reorganization of parse tree to become an ObjTree, with boolean, array
- Reorganization of ObjTree to a JsonB value
I am actually a bit doubtful about why we actually need this intermediate
ObjTree step... What would be wrong in manipulating a JSON(B) parsed tree,
that is first empty, and pushing key/value objects to it when processing
each command? Something moving toward in this direction is that ObjTree has
some logic to manipulate booleans, null values, etc. This results in some
duplication with what jsonb and json can actually do when creating when
manipulating strings, as well as the extra processing like
objtree_to_jsonb_element. ddl_json.c (should be ddl_jsonb.c?) would as well
take more its sense as it directly manipulates JSONB containers.
Regards,
--
Michael

Hi Michael, thanks for the review.

Michael Paquier wrote:

On Thu, Oct 16, 2014 at 4:01 PM, Michael Paquier <michael.paquier@gmail.com>
wrote:

On Mon, Oct 13, 2014 at 12:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Here's a new version of this series.

Here is some input on patch 4:

1) Use of OBJECT_ATTRIBUTE:
+               case OBJECT_ATTRIBUTE:
+                       catalog_id = TypeRelationId;    /* XXX? */
+                       break;
I think that the XXX mark could be removed, using TypeRelationId is correct
IMO as OBJECT ATTRIBUTE is used when managing an attribute object, which is
a custom type used for CREATE/ALTER TYPE.

Agreed.

2) This patch is showing up many warnings, among them:

Will check.

3) What's that actually?
+/* XXX merge this with ObjectTypeMap? */
static event_trigger_support_data event_trigger_support[] = {
We may be able to do something smarter with event_trigger_support[], but as
this consists in a mapping of subcommands associated with CREATE/DROP/ALTER
and is only a reverse engineering operation of somewhat
AlterObjectTypeCommandTag or CreateCommandTag, I am not sure how you could
merge that. Some input perhaps?

ObjectTypeMap is part of the patch that handles DROP for replication;
see my other patch in commitfest. I am also not sure how to merge all
this stuff; with these patches, we would have three "do event triggers
support this object type" functions, so I was thinking in having maybe a
text file from which these functions are generated from a perl script or
something. But for now I think it's okay to keep things like this.
That comment is only there to remind me that some cleanup might be in
order.

4)
+/*
+ * EventTriggerStashCommand
+ *             Save data about a simple DDL command that was just executed
+ */
Shouldn't this be "single" instead of "simple"?

In an older version it was "basic". Not wedded to "simple", but I don't
think "single" is the right thing. A later patch in the series
introduces type Grant, and there's also type AlterTable. The idea
behind Simple is to include command types that do not require special
handling; but all these commands are single commands.

6)
+               command = deparse_utility_command(cmd);
+
+               /*
+                * Some parse trees return NULL when deparse is attempted;
we don't
+                * emit anything for them.
+                */
+               if (command != NULL)
+               {
Small detail, but you may here just use a continue to make the code a bit
more readable after deparsing the command.

Will check.

9) Those declarations are not needed in event_trigger.c:
+#include "utils/json.h"
+#include "utils/jsonb.h"

Will check. I split ddl_json.c at the last minute and I may have
forgotten to remove these.

10) Would you mind explaining what means "fmt"?
+ * 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.

"fmt" is equivalent to sprintf and friends' fmt argument. I guess this
commands needs to be expanded a bit.

11) deparse_utility.c mentions here and there JSON objects, but what is
created are JSONB objects. I'd rather be clear here.

Good point.

12) Already mentioned before, but this reverse engineering machine for
types would be more welcome as a set of functions in lsyscache (one for
namespace, one for type name and one for is_array). For typemodstr the need
is different as it uses printTypmod.
+void
+format_type_detailed(Oid type_oid, int32 typemod,
+                                        Oid *nspid, char **typname, char
**typemodstr,
+                                        bool *is_array)

I am unsure about this. Other things that require many properties of
the same object do a single lookup and return all of them in a single
call, rather than repeated calls.

13) This change seems unrelated to this patch...
-       int                     type = 0;
+       JsonbIteratorToken type = WJB_DONE;
JsonbValue      v;
int                     level = 0;
bool            redo_switch = false;
@@ -454,7 +454,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");

Yes, sorry. I was trying to figure out how to use the jsonb stuff and
I found this error message was quite unclear. In general, jsonb code
seems to have random warts ...

A couple of comments: this patch introduces a basic infrastructure able to
do the following set of operations:
- Obtention of parse tree using StashedCommand
- Reorganization of parse tree to become an ObjTree, with boolean, array
- Reorganization of ObjTree to a JsonB value
I am actually a bit doubtful about why we actually need this intermediate
ObjTree step... What would be wrong in manipulating a JSON(B) parsed tree,
that is first empty, and pushing key/value objects to it when processing
each command? Something moving toward in this direction is that ObjTree has
some logic to manipulate booleans, null values, etc. This results in some
duplication with what jsonb and json can actually do when creating when
manipulating strings, as well as the extra processing like
objtree_to_jsonb_element. ddl_json.c (should be ddl_jsonb.c?) would as well
take more its sense as it directly manipulates JSONB containers.

Uhm. Obviously we didn't have jsonb when I started this and we do have
them now, so I could perhaps see about updating the patch to do things
this way; but I'm not totally sold on that idea, as my ObjTree stuff is
a lot easier to manage and the jsonb API is pretty ugly.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-10-28 05:30:43 -0300, Alvaro Herrera wrote:

A couple of comments: this patch introduces a basic infrastructure able to
do the following set of operations:
- Obtention of parse tree using StashedCommand
- Reorganization of parse tree to become an ObjTree, with boolean, array
- Reorganization of ObjTree to a JsonB value
I am actually a bit doubtful about why we actually need this intermediate
ObjTree step... What would be wrong in manipulating a JSON(B) parsed tree,
that is first empty, and pushing key/value objects to it when processing
each command? Something moving toward in this direction is that ObjTree has
some logic to manipulate booleans, null values, etc. This results in some
duplication with what jsonb and json can actually do when creating when
manipulating strings, as well as the extra processing like
objtree_to_jsonb_element. ddl_json.c (should be ddl_jsonb.c?) would as well
take more its sense as it directly manipulates JSONB containers.

Uhm. Obviously we didn't have jsonb when I started this and we do have
them now, so I could perhaps see about updating the patch to do things
this way; but I'm not totally sold on that idea, as my ObjTree stuff is
a lot easier to manage and the jsonb API is pretty ugly.

I looked at this as well, and I think trying to do so would not result
in readable code.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, Oct 28, 2014 at 6:00 AM, Andres Freund <andres@2ndquadrant.com> wrote:

Uhm. Obviously we didn't have jsonb when I started this and we do have
them now, so I could perhaps see about updating the patch to do things
this way; but I'm not totally sold on that idea, as my ObjTree stuff is
a lot easier to manage and the jsonb API is pretty ugly.

I looked at this as well, and I think trying to do so would not result
in readable code.

That doesn't speak very well of jsonb. :-(

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, Oct 30, 2014 at 2:40 AM, Robert Haas <robertmhaas@gmail.com> wrote:

On Tue, Oct 28, 2014 at 6:00 AM, Andres Freund <andres@2ndquadrant.com>
wrote:

Uhm. Obviously we didn't have jsonb when I started this and we do have
them now, so I could perhaps see about updating the patch to do things
this way; but I'm not totally sold on that idea, as my ObjTree stuff is
a lot easier to manage and the jsonb API is pretty ugly.

I looked at this as well, and I think trying to do so would not result
in readable code.

That doesn't speak very well of jsonb. :-(

Just did the same and I played a bit with the APIs. And I am getting the
impression that the jsonb API is currently focused on the fact of deparsing
and parsing Jsonb strings to/from containers but there is no real interface
that allows to easily manipulate the containers where the values are
located. So, what I think is missing is really a friendly interface to
manipulate JsonbContainers directly, and I think that we are not far from
it with something like this set, roughly:
- Initialization of an empty container
- Set of APIs to directly push a value to a container (boolean, array,
null, string, numeric or other jsonb object)
- Initialization of JsonbValue objects
With this basic set of APIs patch 4 could for example use JsonbToCString to
then convert the JSONB bucket back to a string it sends to client. Note as
well that there is already findJsonbValueFromContainer present to get back
a value in a container.

In short, my point is: instead of re-creating the wheel like what this
series of patch is trying to do with ObjTree, I think that it would be more
fruitful to have a more solid in-place JSONB infrastructure that allows to
directly manipulate JSONB objects. This feature as well as future
extensions could benefit from that.
Feel free to comment.
Regards,
--
Michael

On Fri, Oct 31, 2014 at 11:27 AM, Michael Paquier <michael.paquier@gmail.com

wrote:

So, what I think is missing is really a friendly interface to manipulate
JsonbContainers directly, and I think that we are not far from it with
something like this set, roughly:
- Initialization of an empty container
- Set of APIs to directly push a value to a container (boolean, array,
null, string, numeric or other jsonb object)
- Initialization of JsonbValue objects

Here are more thoughts among those lines looking at the current state of
the patch 4 that introduces the infrastructure of the whole feature. This
would make possible in-memory manipulation of jsonb containers without
relying on a 3rd-part set of APIs like what this patch is doing with
ObjTree to deparse the DDL parse trees.
1) Set of constructor functions for JsonbValue: null, bool, string, array,
JSONB object for nested values. Note that keys for can be used as Jsonb
string objects
2) Lookup functions for values in a JsonbContainer. Patch 4 is introducing
that with find_string_in_jsonbcontainer and find_bool_in_jsonbcontainer. We
may as well extend it to be able to look for another Jsonb object for
nested searches for example.
3) Functions to push JsonbValue within a container, using a key and a
value. This is where most of the work would be necessary, for bool, null,
string, Jsonb object and numeric.

This infrastructure would allow in-memory manipulation of jsonb containers.
Containers that can then be easily be manipulated to be changed back to
strings and for value lookups using key strings.
Regards,
--
Michael

Michael Paquier wrote:

Here are more thoughts among those lines looking at the current state of
the patch 4 that introduces the infrastructure of the whole feature. This
would make possible in-memory manipulation of jsonb containers without
relying on a 3rd-part set of APIs like what this patch is doing with
ObjTree to deparse the DDL parse trees.

Thanks for the thoughts. I have to say that I have no intention of
reworking the jsonb code. If somebody else wants to do the legwork and
add that API as you suggest, I'm happy to remove all the ObjTree stuff
from this patch. I don't expect this to happen too soon, though, so I
would instead consider committing this patch based on ObjTree. Later,
when somebody comes around to reworking jsonb, we can rip ObjTree out.

This infrastructure would allow in-memory manipulation of jsonb containers.
Containers that can then be easily be manipulated to be changed back to
strings and for value lookups using key strings.

Honestly, I had hoped that the jsonb code would have already included
this kind of functionality. I wasn't too happy when I discovered that I
needed to keep the ObjTree crap. But I don't want to do that work
myself.

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, Nov 7, 2014 at 10:45 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Here are more thoughts among those lines looking at the current state of
the patch 4 that introduces the infrastructure of the whole feature. This
would make possible in-memory manipulation of jsonb containers without
relying on a 3rd-part set of APIs like what this patch is doing with
ObjTree to deparse the DDL parse trees.

Thanks for the thoughts. I have to say that I have no intention of
reworking the jsonb code. If somebody else wants to do the legwork and
add that API as you suggest, I'm happy to remove all the ObjTree stuff
from this patch. I don't expect this to happen too soon, though, so I
would instead consider committing this patch based on ObjTree. Later,
when somebody comes around to reworking jsonb, we can rip ObjTree out.

This infrastructure would allow in-memory manipulation of jsonb containers.
Containers that can then be easily be manipulated to be changed back to
strings and for value lookups using key strings.

Honestly, I had hoped that the jsonb code would have already included
this kind of functionality. I wasn't too happy when I discovered that I
needed to keep the ObjTree crap. But I don't want to do that work
myself.

If we're going to have infrastructure for this in core, we really
ought to make the effort to make it general instead of not.

I still think this whole idea is a mess. It adds what looks to be a
LOT of code that, at least as I understand it, we have no compelling
way to regression test and which will likely receive very limited
testing from users to support a feature that is not in core, may never
be, and which I strongly suspect may be too clever by half. Once
you've committed it, you're going to move onto other things and leave
it to everyone who comes after to try to maintain it. I bet we'll
still be running into bugs and half-implemented features five years
from now, and maybe in ten. Ramming through special-purpose
infrastructure instead of generalizing it is merely icing on the cake,
but it's still moving in the wrong direction.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-11-07 21:41:17 -0500, Robert Haas wrote:

On Fri, Nov 7, 2014 at 10:45 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Here are more thoughts among those lines looking at the current state of
the patch 4 that introduces the infrastructure of the whole feature. This
would make possible in-memory manipulation of jsonb containers without
relying on a 3rd-part set of APIs like what this patch is doing with
ObjTree to deparse the DDL parse trees.

Thanks for the thoughts. I have to say that I have no intention of
reworking the jsonb code. If somebody else wants to do the legwork and
add that API as you suggest, I'm happy to remove all the ObjTree stuff
from this patch. I don't expect this to happen too soon, though, so I
would instead consider committing this patch based on ObjTree. Later,
when somebody comes around to reworking jsonb, we can rip ObjTree out.

This infrastructure would allow in-memory manipulation of jsonb containers.
Containers that can then be easily be manipulated to be changed back to
strings and for value lookups using key strings.

Honestly, I had hoped that the jsonb code would have already included
this kind of functionality. I wasn't too happy when I discovered that I
needed to keep the ObjTree crap. But I don't want to do that work
myself.

If we're going to have infrastructure for this in core, we really
ought to make the effort to make it general instead of not.

I still think this whole idea is a mess. It adds what looks to be a
LOT of code that, at least as I understand it, we have no compelling
way to regression test

I don't understand why this is particularly difficult to regresssion
test. It actually is comparatively simple?

and which will likely receive very limited
testing from users to support a feature that is not in core,

Just like half of the features you worked on yourself lately? Why is
this an argument?

Being able to replicate DDL is a feature wish that has been around
pretty much since the inception of trigger based replication
solution. It's not some current fancy. And the json stuff only got there
because people wanted some way to manipulate the names in the replicated
- which this abstraction provides them with.

may never be, and which I strongly suspect may be too clever by half.
Once you've committed it, you're going to move onto other things and
leave it to everyone who comes after to try to maintain it. I bet
we'll still be running into bugs and half-implemented features five
years from now, and maybe in ten. Ramming through special-purpose
infrastructure instead of generalizing it is merely icing on the cake,
but it's still moving in the wrong direction.

You're just as much to blame for not writing a general json abstraction
layer for EXPLAIN. I'd say that's not much blame, but still, there's
really not much difference there.

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Nov 8, 2014 at 12:45 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

Michael Paquier wrote:

Here are more thoughts among those lines looking at the current state of
the patch 4 that introduces the infrastructure of the whole feature. This
would make possible in-memory manipulation of jsonb containers without
relying on a 3rd-part set of APIs like what this patch is doing with
ObjTree to deparse the DDL parse trees.

Thanks for the thoughts. I have to say that I have no intention of
reworking the jsonb code. If somebody else wants to do the legwork and
add that API as you suggest, I'm happy to remove all the ObjTree stuff
from this patch. I don't expect this to happen too soon, though, so I
would instead consider committing this patch based on ObjTree. Later,
when somebody comes around to reworking jsonb, we can rip ObjTree out.

The thing freaking me out in this case is when would that really
happen? Maybe years from now, and perhaps at that point we would
regret to not have put in place the infrastructure that we knew we
could have done.

This infrastructure would allow in-memory manipulation of jsonb containers.
Containers that can then be easily be manipulated to be changed back to
strings and for value lookups using key strings.

Honestly, I had hoped that the jsonb code would have already included
this kind of functionality. I wasn't too happy when I discovered that I
needed to keep the ObjTree crap. But I don't want to do that work
myself.

I can't blame you for that.

In any case, if this code goes in as-is (I am against it at this point
but I am just giving my opinion as a reviewer here), I think that at
least the following things could be done with a minimal effort:
- Provide the set of constructor functions for JsonbValue
- Move the jsonb APIs (find_*_in_jsonbcontainer) doing value lookups
using keys from ddl_json.c to jsonb_util.c, rename them to something
more consistent and complete the set for the other object types.
- Add a TODO item in the wiki, and a TODO item in where ObjTree is defined.
Thanks,
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, Nov 8, 2014 at 4:37 AM, Andres Freund <andres@2ndquadrant.com> wrote:

I don't understand why this is particularly difficult to regresssion
test. It actually is comparatively simple?

If it is, great. I previously wrote this email:

/messages/by-id/CA+TgmoZ=vZriJMxLkqi_V0jg4k4LEAPmwUSC6RWXS5MquXUJNA@mail.gmail.com

Alvaro came up with a way of addressing the second point I raised
there, which I'm quite pleased about, but AFAIK there's been no
progress on the first one. Maybe I missed something?

and which will likely receive very limited
testing from users to support a feature that is not in core,

Just like half of the features you worked on yourself lately? Why is
this an argument?

Because the stuff I'm adding doesn't break every time someone adds a
new DDL command, something that we do regularly.

If it were a question of writing this code once and being done with
it, that would be unobjectionable in my view. But it isn't.
Practically every change to gram.y is going to require a corresponding
change to this stuff. As far as I can see, nobody except me has
commented on the burden that places on everyone who may wish to add
syntax support for a new construct in the future, which might mean
that I'm worrying about something that isn't worth worrying about, but
what I think is more likely is that nobody's worrying about it right
now because they haven't had to do it yet.

Just to illustrate the point, consider the CREATE TABLE name OF type
syntax that Peter added a few years ago. That patch
(e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
gram.y:

src/backend/parser/gram.y | 56 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 43 insertions(+), 13 deletions(-)

Now let's have a look at what impact it has on the deparsing code.
Patch 6 has deparse_ColumnDef_Typed from lines 134 to 193. There's 3
or so lines in deparseTableElements that decide whether to call it.
Patch 8 has more handling for this case, lines 439 to 443 and 463 to
490. So, if this feature had been committed before TABLE OF, it would
have needed about 100 lines of net new code to handle this case -
exclusive of refactoring. The actual size of the patch would probably
have been modestly larger than that, because some code would need to
be reindented when it got iffed out, and quite possibly some
rearrangement would have been needed. But even ignoring all that, the
deparse footprint of the patch would have been MORE THAN TWICE the
parser footprint.

I think that's going to be typical; and I think the deparse code is
going to be significantly more labor-intensive to write than bison
productions are. Do you really think that's not going to be a burden?

Being able to replicate DDL is a feature wish that has been around
pretty much since the inception of trigger based replication
solution. It's not some current fancy. And the json stuff only got there
because people wanted some way to manipulate the names in the replicated
- which this abstraction provides them with.

I understand that being able to replicate DDL is an important need,
and there may be no better way to do it than this. But that doesn't
mean that this code is going to be bug-free or easy to maintain.

may never be, and which I strongly suspect may be too clever by half.
Once you've committed it, you're going to move onto other things and
leave it to everyone who comes after to try to maintain it. I bet
we'll still be running into bugs and half-implemented features five
years from now, and maybe in ten. Ramming through special-purpose
infrastructure instead of generalizing it is merely icing on the cake,
but it's still moving in the wrong direction.

You're just as much to blame for not writing a general json abstraction
layer for EXPLAIN. I'd say that's not much blame, but still, there's
really not much difference there.

Well, this patch is series is at least an order of magnitude larger,
and it's apparently doing significantly more complex stuff with JSON,
because the explain.c patch just writes it out into a StringInfo.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:

If it were a question of writing this code once and being done with
it, that would be unobjectionable in my view. But it isn't.
Practically every change to gram.y is going to require a corresponding
change to this stuff. As far as I can see, nobody except me has
commented on the burden that places on everyone who may wish to add
syntax support for a new construct in the future, which might mean
that I'm worrying about something that isn't worth worrying about, but
what I think is more likely is that nobody's worrying about it right
now because they haven't had to do it yet.

I haven't been paying much attention to this thread, but I concur with
Robert that adding yet another set of overhead requirements to any
addition of new SQL is not a good thing.

Just to illustrate the point, consider the CREATE TABLE name OF type
syntax that Peter added a few years ago. That patch
(e7b3349a8ad7afaad565c573fbd65fb46af6abbe) had the following impact on
gram.y:

This analysis is kind of cheating, because adding new syntax hasn't been
only a matter of touching gram.y for a very long time. You invariably
have to touch pg_dump, and you have to touch ruleutils.c unless it's
strictly a DDL-command change. But people are used to those, and the
value of keeping pg_dump working is clear to everybody. Adding a similar
level of burden to support a feature with a narrow use-case seems like
a nonstarter from here. ESPECIALLY if we also have to manually add
regression test cases because there's no easy way to test it directly.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On 2014-11-08 11:52:43 -0500, Tom Lane wrote:

Adding a similar
level of burden to support a feature with a narrow use-case seems like
a nonstarter from here.

I don't understand this statement. In my experience the lack of a usable
replication solution that allows temporary tables and major version
differences is one of the most, if not *the* most, frequent criticisms
of postgres I hear. How is this a narrow use case?

Greetings,

Andres Freund

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Andres Freund <andres@2ndquadrant.com> writes:

On 2014-11-08 11:52:43 -0500, Tom Lane wrote:

Adding a similar
level of burden to support a feature with a narrow use-case seems like
a nonstarter from here.

I don't understand this statement. In my experience the lack of a usable
replication solution that allows temporary tables and major version
differences is one of the most, if not *the* most, frequent criticisms
of postgres I hear. How is this a narrow use case?

[ shrug... ] I don't personally give a damn about logical replication,
especially not logical replication implemented in this fashion. It looks
large and rickety (ie full of opportunities for bugs) and I would never
trust data I cared about to it.

Or in short: AFAICS you're not building the next WAL-shipping replication
solution, you're building the next Slony, and Slony never has and never
will be more than a niche use-case. Putting half of it into core wouldn't
fix that, it would just put a lot more maintenance burden on core
developers. Core developers are entitled to push back on such proposals.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers