Re: Portal->commandTag as an enum

Andres,

The previous patch set seemed to cause confusion, having separated changes into multiple files. The latest patch, heavily influenced by your review, is all in one file, attached.

> On Feb 4, 2020, at 7:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
>
>> 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?

The v3 patch does something like you suggest.

The only gotcha I came across while reorganizing the code this way is that exec_replication_command(…) outputs “SELECT” rather than “SELECT <ROWCOUNT>” as is done everywhere else. Strangely, exec_replication_command(…) does output the rowcount for “COPY <ROWCOUNT>”, which matches how COPY is handled elsewhere. I can’t see any logic in this. I’m concerned that outputting “SELECT 0” from exec_replication_command rather than “SELECT” as is currently done will break some client somewhere, though none that I can find.

Putting the display information into the CommandTag behavior table forces the behavior per tag to be the same everywhere, which forces this change on exec_replication_command.

To get around this, I’ve added an extremely bogus extra boolean argument to EndCommand, force_undecorated_output, that is false from all callers except exec_replication_command(…) in the one spot I described.

I don’t know whether the code should be committed this way, but I need something as a placeholder until I get a better understanding of why exec_replication_command(…) behaves as it does and what I should do about it in the patch.

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

This is changed back in v3 to be more like how it was before.

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

Yeah, and it’s gone in v3. This sort of logic now lives in the behavior table in src/backend/utils/misc/commandtag.c.

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

EventTriggerCommonSetup() gets the command tag enum, not the string, at least in v3.

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

I was having some trouble figuring out what to name the wrapper. “CreateCommandTagAndGetName" is nearly as long as the two function names it replaces. “CreateCommandTagName” sounds like you’re creating a name rather than a CommandTag, which is misleading. But then I realized this function was poorly named to begin with. “Create” is an entirely inappropriate verb for what this function does. Even before this patch, it wasn’t creating anything. I was looking up a constant string name. Now it is looking up an enum.

I went with CreateCommandName(…), but I think this leaves a lot to be desired. Thoughts?

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

This is fixed in v3.

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

I’ve decided not to touch this issue. There are no changes here from how it was done in v2.

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

Rewriting this as you suggest does not prevent tag names from being out of sorted order. Version 3 of the patch adds a perl script that reads commandtag.h and commandtag.c during the build process and stops the build with a brief error message if they don’t match, are malformed, or if the sorting order is wrong. The script does not modify the code. It just reviews it for correctness. As such, it probably doesn’t matter whether it runs on all platforms. I did not look into whether this runs on Windows, but if there is any difficulty there, it could simply be disabled on that platform.

It also doesn’t matter if this perl script gets committed. There is a trade-off here between maintaining the script vs. manually maintaining the enum ordering.

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

I kept a form of GetCommandTagEnum.

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

This is all gone now, either to the perl script or to a StaticAssert, or to a bit of both.

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

This is now handled by the perl script, also.

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

Well, it does get automatically checked for you.

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

There is not enough overlap between NodeTag and CommandTag for any obvious consolidation. Feel free to recommend something specific.

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

I still don’t know what this comment means.

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

This has all been removed from version 3 of the patch set.