Re: Portal->commandTag as an enum

> On Feb 4, 2020, at 7:34 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
>
> Hi,

Thanks for reviewing! I am pretty much in agreement with your comments, below.

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

Agreed, but I don’t know how.

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

Yes, I was thinking about something like this, only I had in mind a Bitmapset for these. It just so happens that there are 192 enum values, 0..191, which happens to fit in 3 64bit words plus a varlena header. Mind you, that nice property would be immediately blown if we added another entry to the enum. Has anybody made a compile-time static version of Bitmapset? We could store this information in either 24 bytes or 32 bytes, depending on whether we add another enum value.

Getting a little off topic, I was also thinking about having a counting Bitmapset that would store one bit per enum that is included, and then a sparse array of counts, perhaps with one byte counts for [0..127] and 8 byte counts for [128..huge] that we could use in shared memory for the pg_stat_tag work. Is there anything like that?

Anyway, I don’t think we should invent lots of different structures for CommandTag tracking, so something that serves double duty might keep the code tighter. I’m already using ordinary Bitmapset over CommandTags in event_trigger, so naturally that comes to mind for this, too.

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

I still need to get a benchmark more targeted at this codepath.

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

I don’t think I care too much either way. I had some vague ideas about consolidating all of these strings in the backend into one place.

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

I think it is painful in a different way. The existing code on master is a mess of parsing logic that is harder to reason through, but fewer lines. There are other places in the backend that have long switch statements, so I didn’t feel I was breaking with project style to do this. I also made the switch longer than I had too, by including all enumerated values rather than just the ones that are supported. We could remove the extra cases, but I think that’s only a half measure. Something more like a consolidated table or bitmap seems better.

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

It is not a string after applying v2-0003…. The main issue I see in the code you are quoting is that CreateCommandTag(cmd->parsetree) is called more than once, and that’s not the consequence of this patch. That’s pre-existing. I didn’t look into it, though I can if you think it is relevant to this patch set. The name of the function, CreateCommandTag, sounds like something I invented as part of this patch, but it pre-exists this patch. I only changed it’s return value from char * to CommandTag.

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

Actually, I looked at that. The number of them seemed right on the line between making a wrapper and not. I thought the counter-argument was that by making a wrapper that only got used in a few places, I was creating more lines of code and obfuscating what happens. I’m happy to do it your way, if consensus emerges around that.

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

Ok.

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

No objection from me, though I’d have to see the alternative and what it does to performance.

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

I had something closer to what you’re asking for as part of the v1 patch and ripped it out to get the code size down. Avoiding code bloat was one of Tom's concerns. What you are suggesting is admittedly better than what I ripped out, though.

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

I never quite came up with a one-size-fits-all enumeration. There are lots of places where these enumerations seem to almost map onto each other, but with special cases that don’t line up. I’m open to suggestions.

>> 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’m not sure what you mean. That bsearch is pre-existing, not mine.

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

Yeah, I already conceded to Tom in his review that I’m not wedded to committing this test in any form, let alone in this form. That’s part of why I kept it as a separate patch file. But if you like what it is doing, and just don’t like the verbosity, I can try harder to compress it.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company