Re: Add CREATE support to event triggers

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(at)alvh(dot)no-ip(dot)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(at)alvh(dot)no-ip(dot)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(at)alvh(dot)no-ip(dot)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(at)alvh(dot)no-ip(dot)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(at)alvh(dot)no-ip(dot)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