Re: Portal->commandTag as an enum

Hi,

On 2020-02-04 18:18:52 -0800, Mark Dilger wrote:
> In master, a number of functions pass a char *completionTag argument (really a char completionTag[COMPLETION_TAG_BUFSIZE]) which gets filled in with the string to return to the client from EndCommand. I have removed that kind of logic:
>
> - /* save the rowcount if we're given a completionTag to fill */
> - if (completionTag)
> - snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> - "SELECT " UINT64_FORMAT,
> - queryDesc->estate->es_processed);
>
> In the patch, this is replaced with a new struct, QueryCompletionData. That bit of code above is replaced with:
>
> + /* save the rowcount if we're given a qc to fill */
> + if (qc)
> + SetQC(qc, COMMANDTAG_SELECT, queryDesc->estate->es_processed, DISPLAYFORMAT_NPROCESSED);
>
> For wire protocol compatibility, we have to track the display format.
> When this gets to EndCommand, the display format allows the string to
> be written exactly as the client will expect. If we ever get to the
> point where we can break with that compatibility, the third member of
> this struct, display_format, can be removed.

Hm. While I like not having this as strings a lot, I wish we could get
rid of this displayformat stuff.

> These are replaced by switch() case statements over the possible commandTags:
>
> + switch (commandTag)
> + {
> + /*
> + * Supported idiosyncratic special cases.
> + */
> + case COMMANDTAG_ALTER_DEFAULT_PRIVILEGES:
> + case COMMANDTAG_ALTER_LARGE_OBJECT:
> + case COMMANDTAG_COMMENT:
> + case COMMANDTAG_CREATE_TABLE_AS:
> + case COMMANDTAG_DROP_OWNED:
> + case COMMANDTAG_GRANT:
> + case COMMANDTAG_IMPORT_FOREIGN_SCHEMA:
> + case COMMANDTAG_REFRESH_MATERIALIZED_VIEW:
> + case COMMANDTAG_REVOKE:
> + case COMMANDTAG_SECURITY_LABEL:
> + case COMMANDTAG_SELECT_INTO:

The number of these makes me wonder if we should just have a metadata
table in one place, instead of needing to edit multiple
locations. Something like

const ... CommandTagBehaviour[] = {
[COMMANDTAG_INSERT] = {
.display_processed = true, .display_oid = true, ...},
[COMMANDTAG_CREATE_TABLE_AS] = {
.event_trigger = true, ...},

with the zero initialized defaults being the common cases.

Not sure if it's worth going there. But it's maybe worth thinking about
for a minute?

> Averages for test set 1 by scale:
> set scale tps avg_latency 90%< max_latency
> 1 1 3741 1.734 3.162 133.718
> 1 9 6124 0.904 1.05 230.547
> 1 81 5921 0.931 1.015 67.023
>
> Averages for test set 1 by clients:
> set clients tps avg_latency 90%< max_latency
> 1 1 2163 0.461 0.514 24.414
> 1 4 5968 0.675 0.791 40.354
> 1 16 7655 2.433 3.922 366.519
>
>
> For command tag patch (branched from 1fd687a035):
>
> postgresql % find src -type f -name "*.c" -or -name "*.h" | xargs cat | wc
> 1482969 5691908 45281399
>
> postgresql % find src -type f -name "*.o" | xargs cat | wc
> 38209 476243 18999752
>
>
> Averages for test set 1 by scale:
> set scale tps avg_latency 90%< max_latency
> 1 1 3877 1.645 3.066 24.973
> 1 9 6383 0.859 1.032 64.566
> 1 81 5945 0.925 1.023 162.9
>
> Averages for test set 1 by clients:
> set clients tps avg_latency 90%< max_latency
> 1 1 2141 0.466 0.522 11.531
> 1 4 5967 0.673 0.783 136.882
> 1 16 8096 2.292 3.817 104.026

Not bad.

> diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
> index 9aa2b61600..5322c14ce4 100644
> --- a/src/backend/commands/async.c
> +++ b/src/backend/commands/async.c
> @@ -594,7 +594,7 @@ pg_notify(PG_FUNCTION_ARGS)
> payload = text_to_cstring(PG_GETARG_TEXT_PP(1));
>
> /* For NOTIFY as a statement, this is checked in ProcessUtility */
> - PreventCommandDuringRecovery("NOTIFY");
> + PreventCommandDuringRecovery(COMMANDTAG_NOTIFY);
>
> Async_Notify(channel, payload);
>
> diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
> index 40a8ec1abd..4828e75bd5 100644
> --- a/src/backend/commands/copy.c
> +++ b/src/backend/commands/copy.c
> @@ -1063,7 +1063,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
>
> /* check read-only transaction and parallel mode */
> if (XactReadOnly && !rel->rd_islocaltemp)
> - PreventCommandIfReadOnly("COPY FROM");
> + PreventCommandIfReadOnly(COMMANDTAG_COPY_FROM);
>
> cstate = BeginCopyFrom(pstate, rel, stmt->filename, stmt->is_program,
> NULL, stmt->attlist, stmt->options);

I'm not sure this really ought to be part of this change - seems like a
somewhat independent change to me. With less obvious benefits.

> static event_trigger_command_tag_check_result
> -check_ddl_tag(const char *tag)
> +check_ddl_tag(CommandTag commandTag)
> {
> - const char *obtypename;
> - const event_trigger_support_data *etsd;
> + switch (commandTag)
> + {
> + /*
> + * Supported idiosyncratic special cases.
> + */
> + case COMMANDTAG_ALTER_DEFAULT_PRIVILEGES:
> + case COMMANDTAG_ALTER_LARGE_OBJECT:
> + case COMMANDTAG_COMMENT:
> + case COMMANDTAG_CREATE_TABLE_AS:
> + case COMMANDTAG_DROP_OWNED:
> + case COMMANDTAG_GRANT:
> + case COMMANDTAG_IMPORT_FOREIGN_SCHEMA:
> + case COMMANDTAG_REFRESH_MATERIALIZED_VIEW:
> + case COMMANDTAG_REVOKE:
> + case COMMANDTAG_SECURITY_LABEL:
> + case COMMANDTAG_SELECT_INTO:
>
> - /*
> - * Handle some idiosyncratic special cases.
> - */
> - if (pg_strcasecmp(tag, "CREATE TABLE AS") == 0 ||
> - pg_strcasecmp(tag, "SELECT INTO") == 0 ||
> - pg_strcasecmp(tag, "REFRESH MATERIALIZED VIEW") == 0 ||
> - pg_strcasecmp(tag, "ALTER DEFAULT PRIVILEGES") == 0 ||
> - pg_strcasecmp(tag, "ALTER LARGE OBJECT") == 0 ||
> - pg_strcasecmp(tag, "COMMENT") == 0 ||
> - pg_strcasecmp(tag, "GRANT") == 0 ||
> - pg_strcasecmp(tag, "REVOKE") == 0 ||
> - pg_strcasecmp(tag, "DROP OWNED") == 0 ||
> - pg_strcasecmp(tag, "IMPORT FOREIGN SCHEMA") == 0 ||
> - pg_strcasecmp(tag, "SECURITY LABEL") == 0)
> - return EVENT_TRIGGER_COMMAND_TAG_OK;
> + /*
> + * Supported CREATE commands
> + */
> + case COMMANDTAG_CREATE_ACCESS_METHOD:
> + case COMMANDTAG_CREATE_AGGREGATE:
> + case COMMANDTAG_CREATE_CAST:
> + case COMMANDTAG_CREATE_COLLATION:
> + case COMMANDTAG_CREATE_CONSTRAINT:
> + case COMMANDTAG_CREATE_CONVERSION:
> + case COMMANDTAG_CREATE_DOMAIN:
> + case COMMANDTAG_CREATE_EXTENSION:
> + case COMMANDTAG_CREATE_FOREIGN_DATA_WRAPPER:
> + case COMMANDTAG_CREATE_FOREIGN_TABLE:
> + case COMMANDTAG_CREATE_FUNCTION:
> + case COMMANDTAG_CREATE_INDEX:
> + case COMMANDTAG_CREATE_LANGUAGE:
> + case COMMANDTAG_CREATE_MATERIALIZED_VIEW:
> + case COMMANDTAG_CREATE_OPERATOR:
> + case COMMANDTAG_CREATE_OPERATOR_CLASS:
> + case COMMANDTAG_CREATE_OPERATOR_FAMILY:
> + case COMMANDTAG_CREATE_POLICY:
> + case COMMANDTAG_CREATE_PROCEDURE:
> + case COMMANDTAG_CREATE_PUBLICATION:
> + case COMMANDTAG_CREATE_ROUTINE:
> + case COMMANDTAG_CREATE_RULE:
> + case COMMANDTAG_CREATE_SCHEMA:
> + case COMMANDTAG_CREATE_SEQUENCE:
> + case COMMANDTAG_CREATE_SERVER:
> + case COMMANDTAG_CREATE_STATISTICS:
> + case COMMANDTAG_CREATE_SUBSCRIPTION:
> + case COMMANDTAG_CREATE_TABLE:
> + case COMMANDTAG_CREATE_TEXT_SEARCH_CONFIGURATION:
> + case COMMANDTAG_CREATE_TEXT_SEARCH_DICTIONARY:
> + case COMMANDTAG_CREATE_TEXT_SEARCH_PARSER:
> + case COMMANDTAG_CREATE_TEXT_SEARCH_TEMPLATE:
> + case COMMANDTAG_CREATE_TRANSFORM:
> + case COMMANDTAG_CREATE_TRIGGER:
> + case COMMANDTAG_CREATE_TYPE:
> + case COMMANDTAG_CREATE_USER_MAPPING:
> + case COMMANDTAG_CREATE_VIEW:
>
> - /*
> - * Otherwise, command should be CREATE, ALTER, or DROP.
> - */
> - if (pg_strncasecmp(tag, "CREATE ", 7) == 0)
> - obtypename = tag + 7;
> - else if (pg_strncasecmp(tag, "ALTER ", 6) == 0)
> - obtypename = tag + 6;
> - else if (pg_strncasecmp(tag, "DROP ", 5) == 0)
> - obtypename = tag + 5;
> - else
> - return EVENT_TRIGGER_COMMAND_TAG_NOT_RECOGNIZED;
> + /*
> + * Supported ALTER commands
> + */
> + case COMMANDTAG_ALTER_ACCESS_METHOD:
> + case COMMANDTAG_ALTER_AGGREGATE:
> + case COMMANDTAG_ALTER_CAST:
> + case COMMANDTAG_ALTER_COLLATION:
> + case COMMANDTAG_ALTER_CONSTRAINT:
> + case COMMANDTAG_ALTER_CONVERSION:
> + case COMMANDTAG_ALTER_DOMAIN:
> + case COMMANDTAG_ALTER_EXTENSION:
> + case COMMANDTAG_ALTER_FOREIGN_DATA_WRAPPER:
> + case COMMANDTAG_ALTER_FOREIGN_TABLE:
> + case COMMANDTAG_ALTER_FUNCTION:
> + case COMMANDTAG_ALTER_INDEX:
> + case COMMANDTAG_ALTER_LANGUAGE:
> + case COMMANDTAG_ALTER_MATERIALIZED_VIEW:
> + case COMMANDTAG_ALTER_OPERATOR:
> + case COMMANDTAG_ALTER_OPERATOR_CLASS:
> + case COMMANDTAG_ALTER_OPERATOR_FAMILY:
> + case COMMANDTAG_ALTER_POLICY:
> + case COMMANDTAG_ALTER_PROCEDURE:
> + case COMMANDTAG_ALTER_PUBLICATION:
> + case COMMANDTAG_ALTER_ROUTINE:
> + case COMMANDTAG_ALTER_RULE:
> + case COMMANDTAG_ALTER_SCHEMA:
> + case COMMANDTAG_ALTER_SEQUENCE:
> + case COMMANDTAG_ALTER_SERVER:
> + case COMMANDTAG_ALTER_STATISTICS:
> + case COMMANDTAG_ALTER_SUBSCRIPTION:
> + case COMMANDTAG_ALTER_TABLE:
> + case COMMANDTAG_ALTER_TEXT_SEARCH_CONFIGURATION:
> + case COMMANDTAG_ALTER_TEXT_SEARCH_DICTIONARY:
> + case COMMANDTAG_ALTER_TEXT_SEARCH_PARSER:
> + case COMMANDTAG_ALTER_TEXT_SEARCH_TEMPLATE:
> + case COMMANDTAG_ALTER_TRANSFORM:
> + case COMMANDTAG_ALTER_TRIGGER:
> + case COMMANDTAG_ALTER_TYPE:
> + case COMMANDTAG_ALTER_USER_MAPPING:
> + case COMMANDTAG_ALTER_VIEW:
>
> - /*
> - * ...and the object type should be something recognizable.
> - */
> - for (etsd = event_trigger_support; etsd->obtypename != NULL; etsd++)
> - if (pg_strcasecmp(etsd->obtypename, obtypename) == 0)
> + /*
> + * Supported DROP commands
> + */
> + case COMMANDTAG_DROP_ACCESS_METHOD:
> + case COMMANDTAG_DROP_AGGREGATE:
> + case COMMANDTAG_DROP_CAST:
> + case COMMANDTAG_DROP_COLLATION:
> + case COMMANDTAG_DROP_CONSTRAINT:
> + case COMMANDTAG_DROP_CONVERSION:
> + case COMMANDTAG_DROP_DOMAIN:
> + case COMMANDTAG_DROP_EXTENSION:
> + case COMMANDTAG_DROP_FOREIGN_DATA_WRAPPER:
> + case COMMANDTAG_DROP_FOREIGN_TABLE:
> + case COMMANDTAG_DROP_FUNCTION:
> + case COMMANDTAG_DROP_INDEX:
> + case COMMANDTAG_DROP_LANGUAGE:
> + case COMMANDTAG_DROP_MATERIALIZED_VIEW:
> + case COMMANDTAG_DROP_OPERATOR:
> + case COMMANDTAG_DROP_OPERATOR_CLASS:
> + case COMMANDTAG_DROP_OPERATOR_FAMILY:
> + case COMMANDTAG_DROP_POLICY:
> + case COMMANDTAG_DROP_PROCEDURE:
> + case COMMANDTAG_DROP_PUBLICATION:
> + case COMMANDTAG_DROP_ROUTINE:
> + case COMMANDTAG_DROP_RULE:
> + case COMMANDTAG_DROP_SCHEMA:
> + case COMMANDTAG_DROP_SEQUENCE:
> + case COMMANDTAG_DROP_SERVER:
> + case COMMANDTAG_DROP_STATISTICS:
> + case COMMANDTAG_DROP_SUBSCRIPTION:
> + case COMMANDTAG_DROP_TABLE:
> + case COMMANDTAG_DROP_TEXT_SEARCH_CONFIGURATION:
> + case COMMANDTAG_DROP_TEXT_SEARCH_DICTIONARY:
> + case COMMANDTAG_DROP_TEXT_SEARCH_PARSER:
> + case COMMANDTAG_DROP_TEXT_SEARCH_TEMPLATE:
> + case COMMANDTAG_DROP_TRANSFORM:
> + case COMMANDTAG_DROP_TRIGGER:
> + case COMMANDTAG_DROP_TYPE:
> + case COMMANDTAG_DROP_USER_MAPPING:
> + case COMMANDTAG_DROP_VIEW:
> + return EVENT_TRIGGER_COMMAND_TAG_OK;
> +
> + /*
> + * Unsupported CREATE commands
> + */
> + case COMMANDTAG_CREATE_DATABASE:
> + case COMMANDTAG_CREATE_EVENT_TRIGGER:
> + case COMMANDTAG_CREATE_ROLE:
> + case COMMANDTAG_CREATE_TABLESPACE:
> +
> + /*
> + * Unsupported ALTER commands
> + */
> + case COMMANDTAG_ALTER_DATABASE:
> + case COMMANDTAG_ALTER_EVENT_TRIGGER:
> + case COMMANDTAG_ALTER_ROLE:
> + case COMMANDTAG_ALTER_TABLESPACE:
> +
> + /*
> + * Unsupported DROP commands
> + */
> + case COMMANDTAG_DROP_DATABASE:
> + case COMMANDTAG_DROP_EVENT_TRIGGER:
> + case COMMANDTAG_DROP_ROLE:
> + case COMMANDTAG_DROP_TABLESPACE:
> +
> + /*
> + * Other unsupported commands. These used to return
> + * EVENT_TRIGGER_COMMAND_TAG_NOT_RECOGNIZED prior to the
> + * conversion of commandTag from string to enum.
> + */
> + case COMMANDTAG_ALTER_SYSTEM:
> + case COMMANDTAG_ANALYZE:
> + case COMMANDTAG_BEGIN:
> + case COMMANDTAG_CALL:
> + case COMMANDTAG_CHECKPOINT:
> + case COMMANDTAG_CLOSE:
> + case COMMANDTAG_CLOSE_CURSOR:
> + case COMMANDTAG_CLOSE_CURSOR_ALL:
> + case COMMANDTAG_CLUSTER:
> + case COMMANDTAG_COMMIT:
> + case COMMANDTAG_COMMIT_PREPARED:
> + case COMMANDTAG_COPY:
> + case COMMANDTAG_COPY_FROM:
> + case COMMANDTAG_DEALLOCATE:
> + case COMMANDTAG_DEALLOCATE_ALL:
> + case COMMANDTAG_DECLARE_CURSOR:
> + case COMMANDTAG_DELETE:
> + case COMMANDTAG_DISCARD:
> + case COMMANDTAG_DISCARD_ALL:
> + case COMMANDTAG_DISCARD_PLANS:
> + case COMMANDTAG_DISCARD_SEQUENCES:
> + case COMMANDTAG_DISCARD_TEMP:
> + case COMMANDTAG_DO:
> + case COMMANDTAG_DROP_REPLICATION_SLOT:
> + case COMMANDTAG_EXECUTE:
> + case COMMANDTAG_EXPLAIN:
> + case COMMANDTAG_FETCH:
> + case COMMANDTAG_GRANT_ROLE:
> + case COMMANDTAG_INSERT:
> + case COMMANDTAG_LISTEN:
> + case COMMANDTAG_LOAD:
> + case COMMANDTAG_LOCK_TABLE:
> + case COMMANDTAG_MOVE:
> + case COMMANDTAG_NOTIFY:
> + case COMMANDTAG_PREPARE:
> + case COMMANDTAG_PREPARE_TRANSACTION:
> + case COMMANDTAG_REASSIGN_OWNED:
> + case COMMANDTAG_REINDEX:
> + case COMMANDTAG_RELEASE:
> + case COMMANDTAG_RESET:
> + case COMMANDTAG_REVOKE_ROLE:
> + case COMMANDTAG_ROLLBACK:
> + case COMMANDTAG_ROLLBACK_PREPARED:
> + case COMMANDTAG_SAVEPOINT:
> + case COMMANDTAG_SELECT:
> + case COMMANDTAG_SELECT_FOR_KEY_SHARE:
> + case COMMANDTAG_SELECT_FOR_NO_KEY_UPDATE:
> + case COMMANDTAG_SELECT_FOR_SHARE:
> + case COMMANDTAG_SELECT_FOR_UPDATE:
> + case COMMANDTAG_SET:
> + case COMMANDTAG_SET_CONSTRAINTS:
> + case COMMANDTAG_SHOW:
> + case COMMANDTAG_START_TRANSACTION:
> + case COMMANDTAG_TRUNCATE_TABLE:
> + case COMMANDTAG_UNLISTEN:
> + case COMMANDTAG_UPDATE:
> + case COMMANDTAG_VACUUM:
> + return EVENT_TRIGGER_COMMAND_TAG_NOT_SUPPORTED;
> + case COMMANDTAG_UNKNOWN:
> break;
> - if (etsd->obtypename == NULL)
> - return EVENT_TRIGGER_COMMAND_TAG_NOT_RECOGNIZED;
> - if (!etsd->supported)
> - return EVENT_TRIGGER_COMMAND_TAG_NOT_SUPPORTED;
> - return EVENT_TRIGGER_COMMAND_TAG_OK;
> + }
> + return EVENT_TRIGGER_COMMAND_TAG_NOT_RECOGNIZED;
> }

This is pretty painful.

> @@ -745,7 +902,7 @@ EventTriggerCommonSetup(Node *parsetree,
> return NIL;
>
> /* Get the command tag. */
> - tag = CreateCommandTag(parsetree);
> + tag = GetCommandTagName(CreateCommandTag(parsetree));
>
> /*
> * Filter list of event triggers by command tag, and copy them into our
> @@ -2136,7 +2293,7 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
> /* objsubid */
> values[i++] = Int32GetDatum(addr.objectSubId);
> /* command tag */
> - values[i++] = CStringGetTextDatum(CreateCommandTag(cmd->parsetree));
> + values[i++] = CStringGetTextDatum(GetCommandTagName(CreateCommandTag(cmd->parsetree)));
> /* object_type */
> values[i++] = CStringGetTextDatum(type);
> /* schema */
> @@ -2161,7 +2318,7 @@ pg_event_trigger_ddl_commands(PG_FUNCTION_ARGS)
> /* objsubid */
> nulls[i++] = true;
> /* command tag */
> - values[i++] = CStringGetTextDatum(CreateCommandTag(cmd->parsetree));
> + values[i++] = CStringGetTextDatum(GetCommandTagName(CreateCommandTag(cmd->parsetree)));
> /* object_type */
> values[i++] = CStringGetTextDatum(stringify_adefprivs_objtype(cmd->d.defprivs.objtype));
> /* schema */

So GetCommandTagName we commonly do twice for some reason? Once in
EventTriggerCommonSetup() and then again in
pg_event_trigger_ddl_commands()? Why is EventTriggerData.tag still the
string?

> Assert(list_length(plan->plancache_list) == 1);
> @@ -1469,7 +1469,7 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> /* translator: %s is a SQL statement name */
> errmsg("%s is not allowed in a non-volatile function",
> - CreateCommandTag((Node *) pstmt))));
> + GetCommandTagName(CreateCommandTag((Node *) pstmt)))));

Probably worth having a wrapper for this - these lines are pretty long,
and there quite a number of cases like it in the patch.

> @@ -172,11 +175,38 @@ EndCommand(const char *commandTag, CommandDest dest)
> case DestRemoteSimple:
>
> /*
> - * We assume the commandTag is plain ASCII and therefore requires
> - * no encoding conversion.
> + * We assume the tagname is plain ASCII and therefore
> + * requires no encoding conversion.
> */
> - pq_putmessage('C', commandTag, strlen(commandTag) + 1);
> - break;
> + tagname = GetCommandTagName(qc->commandTag);
> + switch (qc->display_format)
> + {
> + case DISPLAYFORMAT_PLAIN:
> + pq_putmessage('C', tagname, strlen(tagname) + 1);
> + break;
> + case DISPLAYFORMAT_LAST_OID:
> + /*
> + * We no longer display LastOid, but to preserve the wire protocol,
> + * we write InvalidOid where the LastOid used to be written. For
> + * efficiency in the snprintf(), hard-code InvalidOid as zero.
> + */
> + Assert(InvalidOid == 0);
> + snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> + "%s 0 " UINT64_FORMAT,
> + tagname,
> + qc->nprocessed);
> + pq_putmessage('C', completionTag, strlen(completionTag) + 1);
> + break;
> + case DISPLAYFORMAT_NPROCESSED:
> + snprintf(completionTag, COMPLETION_TAG_BUFSIZE,
> + "%s " UINT64_FORMAT,
> + tagname,
> + qc->nprocessed);
> + pq_putmessage('C', completionTag, strlen(completionTag) + 1);
> + break;
> + default:
> + elog(ERROR, "Invalid display_format in EndCommand");
> + }

Imo there should only be one pq_putmessage(). Also think this type of
default: is a bad idea, just prevents the compiler from warning if we
were to ever introduce a new variant of DISPLAYFORMAT_*, without
providing any meaningful additional security.

> @@ -855,7 +889,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
>
> case T_DiscardStmt:
> /* should we allow DISCARD PLANS? */
> - CheckRestrictedOperation("DISCARD");
> + CheckRestrictedOperation(COMMANDTAG_DISCARD);
> DiscardCommand((DiscardStmt *) parsetree, isTopLevel);
> break;
>
> @@ -974,7 +1008,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objtype))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecuteGrantStmt(stmt);
> }
> @@ -987,7 +1021,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->removeType))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecDropStmt(stmt, isTopLevel);
> }
> @@ -1000,7 +1034,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->renameType))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecRenameStmt(stmt);
> }
> @@ -1013,7 +1047,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objectType))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecAlterObjectDependsStmt(stmt, NULL);
> }
> @@ -1026,7 +1060,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objectType))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecAlterObjectSchemaStmt(stmt, NULL);
> }
> @@ -1039,7 +1073,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objectType))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> ExecAlterOwnerStmt(stmt);
> }
> @@ -1052,7 +1086,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objtype))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);
> else
> CommentObject(stmt);
> break;
> @@ -1065,7 +1099,7 @@ standard_ProcessUtility(PlannedStmt *pstmt,
> if (EventTriggerSupportsObjectType(stmt->objtype))
> ProcessUtilitySlow(pstate, pstmt, queryString,
> context, params, queryEnv,
> - dest, completionTag);
> + dest, qc);

Not this patch's fault or task. But I hate this type of code - needing
to touch a dozen places for new type of statement is just
insane. utility.c should long have been rewritten to just have one
metadata table for nearly all of this. Perhaps with a few callbacks for
special cases.

> +static const char * tag_names[] = {
> + "???",
> + "ALTER ACCESS METHOD",
> + "ALTER AGGREGATE",
> + "ALTER CAST",

This seems problematic to maintain, because the order needs to match
between this and something defined in a header - and there's no
guarantee a misordering is immediately noticeable. We should either go
for my metadata table idea, or at least rewrite this, even if more
verbose, to something like

static const char * tag_names[] = {
[COMMAND_TAG_ALTER_ACCESS_METHOD] = "ALTER ACCESS METHOD",
...

I think the fact that this would show up in a grep for
COMMAND_TAG_ALTER_ACCESS_METHOD is good too.

> +/*
> + * Search CommandTag by name
> + *
> + * Returns CommandTag, or COMMANDTAG_UNKNOWN if not recognized
> + */
> +CommandTag
> +GetCommandTagEnum(const char *commandname)
> +{
> + const char **base, **last, **position;
> + int result;
> +
> + OPTIONALLY_CHECK_COMMAND_TAGS();
> + if (commandname == NULL || *commandname == '\0')
> + return COMMANDTAG_UNKNOWN;
> +
> + base = tag_names;
> + last = tag_names + tag_name_length - 1;
> + while (last >= base)
> + {
> + position = base + ((last - base) >> 1);
> + result = pg_strcasecmp(commandname, *position);
> + if (result == 0)
> + return (CommandTag) (position - tag_names);
> + else if (result < 0)
> + last = position - 1;
> + else
> + base = position + 1;
> + }
> + return COMMANDTAG_UNKNOWN;
> +}

This seems pretty grotty - but you get rid of it later. See my comments there.

> +#ifdef COMMANDTAG_CHECKING
> +bool
> +CheckCommandTagEnum()
> +{
> + CommandTag i, j;
> +
> + if (FIRST_COMMAND_TAG < 0 || LAST_COMMAND_TAG < 0 || LAST_COMMAND_TAG < FIRST_COMMAND_TAG)
> + {
> + elog(ERROR, "FIRST_COMMAND_TAG (%u), LAST_COMMAND_TAG (%u) not reasonable",
> + (unsigned int) FIRST_COMMAND_TAG, (unsigned int) LAST_COMMAND_TAG);
> + return false;
> + }
> + if (FIRST_COMMAND_TAG != (CommandTag)0)
> + {
> + elog(ERROR, "FIRST_COMMAND_TAG (%u) != 0", (unsigned int) FIRST_COMMAND_TAG);
> + return false;
> + }
> + if (LAST_COMMAND_TAG != (CommandTag)(tag_name_length - 1))
> + {
> + elog(ERROR, "LAST_COMMAND_TAG (%u) != tag_name_length (%u)",
> + (unsigned int) LAST_COMMAND_TAG, (unsigned int) tag_name_length);
> + return false;
> + }

These all seem to want to be static asserts.

> + for (i = FIRST_COMMAND_TAG; i < LAST_COMMAND_TAG; i++)
> + {
> + for (j = i+1; j < LAST_COMMAND_TAG; j++)
> + {
> + int cmp = strcmp(tag_names[i], tag_names[j]);
> + if (cmp == 0)
> + {
> + elog(ERROR, "Found duplicate tag_name: \"%s\"",
> + tag_names[i]);
> + return false;
> + }
> + if (cmp > 0)
> + {
> + elog(ERROR, "Found commandnames out of order: \"%s\" before \"%s\"",
> + tag_names[i], tag_names[j]);
> + return false;
> + }
> + }
> + }
> + return true;
> +}

And I think we could get rid of this with my earlier suggestions?

> +/*
> + * BEWARE: These are in sorted order, but ordered by their printed
> + * values in the tag_name list (see common/commandtag.c).
> + * In particular it matters because the sort ordering changes
> + * when you replace a space with an underscore. To wit:
> + *
> + * "CREATE TABLE"
> + * "CREATE TABLE AS"
> + * "CREATE TABLESPACE"
> + *
> + * but...
> + *
> + * CREATE_TABLE
> + * CREATE_TABLESPACE
> + * CREATE_TABLE_AS
> + *
> + * It also matters that COMMANDTAG_UNKNOWN is written "???".
> + *
> + * If you add a value here, add it in common/commandtag.c also, and
> + * be careful to get the ordering right. You can build with
> + * COMMANDTAG_CHECKING to have this automatically checked
> + * at runtime, but that adds considerable overhead, so do so sparingly.
> + */
> +typedef enum CommandTag
> +{

This seems pretty darn nightmarish.

> +#define FIRST_COMMAND_TAG COMMANDTAG_UNKNOWN
> + COMMANDTAG_UNKNOWN,
> + COMMANDTAG_ALTER_ACCESS_METHOD,
> + COMMANDTAG_ALTER_AGGREGATE,
> + COMMANDTAG_ALTER_CAST,
> + COMMANDTAG_ALTER_COLLATION,
> + COMMANDTAG_ALTER_CONSTRAINT,
> + COMMANDTAG_ALTER_CONVERSION,
> + COMMANDTAG_ALTER_DATABASE,
> + COMMANDTAG_ALTER_DEFAULT_PRIVILEGES,
> + COMMANDTAG_ALTER_DOMAIN,
> [...]

I'm a bit worried that this basically duplicates a good portion of NodeTag, without having otherwise much of a point?

> From a70b0cadc1142e92b2354a0ca3cd47aaeb0c148e Mon Sep 17 00:00:00 2001
> From: Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com>
> Date: Tue, 4 Feb 2020 12:25:05 -0800
> Subject: [PATCH v2 2/3] Using a Bitmapset of tags rather than a string array.
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> EventTriggerCacheItem no longer holds an array of palloc’d tag strings
> in sorted order, but rather just a Bitmapset over the CommandTags. This
> makes the code a little simpler and easier to read, in my opinion. In
> filter_event_trigger, rather than running bsearch through a sorted array
> of strings, it just runs bms_is_member.
> ---

It seems weird to add the bsearch just to remove it immediately again a
patch later. This probably should just go first?

> diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
> index 346168673d..cad02212ad 100644
> --- a/src/test/regress/sql/event_trigger.sql
> +++ b/src/test/regress/sql/event_trigger.sql
> @@ -10,6 +10,13 @@ BEGIN
> END
> $$ language plpgsql;
>
> +-- OK
> +create function test_event_trigger2() returns event_trigger as $$
> +BEGIN
> + RAISE NOTICE 'test_event_trigger2: % %', tg_event, tg_tag;
> +END
> +$$ LANGUAGE plpgsql;
> +
> -- should fail, event triggers cannot have declared arguments
> create function test_event_trigger_arg(name text)
> returns event_trigger as $$ BEGIN RETURN 1; END $$ language plpgsql;
> @@ -82,6 +89,783 @@ create event trigger regress_event_trigger2 on ddl_command_start
> -- OK
> comment on event trigger regress_event_trigger is 'test comment';
>
> +-- These are all unsupported
> +create event trigger regress_event_triger_NULL on ddl_command_start
> + when tag in ('')
> + execute procedure test_event_trigger2();
> +
> +create event trigger regress_event_triger_UNKNOWN on ddl_command_start
> + when tag in ('???')
> + execute procedure test_event_trigger2();
> +
> +create event trigger regress_event_trigger_ALTER_DATABASE on ddl_command_start
> + when tag in ('ALTER DATABASE')
> + execute procedure test_event_trigger2();
[...]

There got to be a more maintainable way to write this.

Greetings,

Andres Freund