Portal->commandTag as an enum - Mailing list pgsql-hackers
From | Mark Dilger |
---|---|
Subject | Portal->commandTag as an enum |
Date | |
Msg-id | 981A9DB4-3F0C-4DA5-88AD-CB9CFF4D6CAD@enterprisedb.com Whole thread Raw |
Responses |
Re: Portal->commandTag as an enum
|
List | pgsql-hackers |
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
Attachment
pgsql-hackers by date: