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