Portal->commandTag as an enum

Hackers,

I have implemented $subject, attached.

While reviewing the "New SQL counter statistics view (pg_stat_sql)” thread [1], I came across Andres’ comment

> That's not really something in this patch, but a lot of this would be
> better if we didn't internally have command tags as strings, but as an
> enum. We should really only convert to a string with needed. That
> we're doing string comparisons on Portal->commandTag is just plain bad.
>
>
>
> If so, we could also make this whole patch a lot cheaper - have a fixed
> size array that has an entry for every possible tag (possibly even in
> shared memory, and then use atomics there).

I put the CommandTag enum in src/common because there wasn’t any reason not to do so. It seems plausible that frontend test frameworks might want access to this enum. I don’t have any frontend code using it yet, nor any concrete plans for that. I’m indifferent about this, and will move it into src/backend if folks think that’s better.

In commands/event_trigger.c, I changed the separation between EVENT_TRIGGER_COMMAND_TAG_NOT_SUPPORTED and EVENT_TRIGGER_COMMAND_TAG_NOT_RECOGNIZED. It used to claim not to recognize command tags that are indeed recognized elsewhere in the system, but simply not expected here. It now returns “not supported” for them, and only returns “not recognized” for special enum values COMMANDTAG_NULL and COMMANDTAG_UNKNOWN, as well as values outside the recognized range of the enum. I’m happy to change my implementation to preserve the old behavior if necessary. Is there a backward compatibility issue here? It does not impact regression test output for me to change this, but that’s not definitive….

I have extended the event_trigger.sql regression test, with new expected output, and when applying that change to master, the test fails due to the “not supported” vs. “not recognized” distinction. I have kept this regression test change in its own patch file, 0002. The differences when applied to master look like:

> create event trigger regress_event_trigger_ALTER_SYSTEM on ddl_command_start
> when tag in ('ALTER SYSTEM')
> execute procedure test_event_trigger2();
> -ERROR: event triggers are not supported for ALTER SYSTEM
> +ERROR: filter value "ALTER SYSTEM" not recognized for filter variable "tag"

PreventCommandIfReadOnly and PreventCommandIfParallelMode sometimes take a commandTag, but in commands/sequence.c they take strings “nextval()” and “setval()”. Likewise, PreventCommandDuringRecovery takes "txid_current()” in adt/txid.c. I had to work around this a little, which was not hard to do, but it made me wonder if command tags and these sorts of functions shouldn’t be unified a bit more. They don’t really look consistent with all the other values in the CommandTag enum, so I left them out. I’m open to opinions about this.

There was some confusion in the code between a commandTag and a completionTag, with the commandTag getting overwritten with the completionTag over the course of execution. I’ve split that out into two distinctly separate concepts, which I think makes the code easier to grok. I’ve added a portal->completionTag field that is a fixed size buffer rather than a palloc’d string, to match how completionTag works elsewhere. But the old code that was overwriting the commandTag (a palloc’d string) with a completionTag (a char[] buffer) was using pstrdup for that purpose. I’m now using strlcpy. I don’t care much which way to go here (buffer vs. palloc’d string). Let me know if using a fixed sized buffer as I’ve done bothers anybody.

There were some instances of things like:

strcpy(completionTag, portal->commandTag);

which should have more properly been

strlcpy(completionTag, portal->commandTag, COMPLETION_TAG_BUFSIZE);

I don’t know if any of these were live bugs, but they seemed like traps for the future, should any new commandTag length overflow the buffer size. I think this patch fixes all of those cases.

Generating CommandTag enum values from user queries and then converting those back to string for printing or use in set_ps_display results in normalization of the commandTag, by which I mean that it becomes all uppercase. I don’t know of any situations where this would matter, but I can’t say for sure that it doesn’t. Anybody have thoughts on that?

[1] https://www.postgresql.org/message-id/flat/CAJrrPGeY4xujjoR=z=KoyRMHEK_pSjjp=7VBhOAHq9rfgpV7QQ(at)mail(dot)gmail(dot)com