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
haveremoved 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.dilger@enterprisedb.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