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
testframeworks 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
elsewherein 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
rangeof the enum.  I’m happy to change my implementation to preserve the old behavior if necessary.  Is there a
backwardcompatibility 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
changein 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
takestrings “nextval()” and “setval()”.  Likewise, PreventCommandDuringRecovery takes "txid_current()” in adt/txid.c.
Ihad to work around this a little, which was not hard to do, but it made me wonder if command tags and these sorts of
functionsshouldn’t be unified a bit more.  They don’t really look consistent with all the other values in the
CommandTagenum, 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
withthe completionTag over the course of execution.  I’ve split that out into two distinctly separate concepts, which I
thinkmakes the code easier to grok.  I’ve added a portal->completionTag field that is a fixed size buffer rather than a
palloc’dstring, to match how completionTag works elsewhere.  But the old code that was overwriting the commandTag (a
palloc’dstring) with a completionTag (a char[] buffer) was using pstrdup for that purpose.  I’m now using strlcpy.  I
don’tcare much which way to go here (buffer vs. palloc’d string).  Let me know if using a fixed sized buffer as I’ve
donebothers 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
lengthoverflow 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_displayresults in normalization of the commandTag, by which I mean that it becomes all uppercase.  I don’t know
ofany 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@mail.gmail.com



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