Re: Portal->commandTag as an enum

> On Feb 3, 2020, at 9:41 AM, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
>
> In v1, I stayed closer to the existing code structure than you are requesting.  I like the direction you’re
suggestingthat I go, and I’ve begun that transition in anticipation of posting a v2 patch set soon. 

Ok, here is v2, attached.

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
haveremoved 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
allowsthe string to be written exactly as the client will expect.  If we ever get to the point where we can break with
thatcompatibility, the third member of this struct, display_format, can be removed. 

Where string parsing is being done in master to get the count back out, it changes to look like this:

-                                       if (strncmp(completionTag, "SELECT ", 7) == 0)
-                                               _SPI_current->processed =
-                                                       pg_strtouint64(completionTag + 7, NULL, 10);
+                                       if (qcdata.commandTag == COMMANDTAG_SELECT)
+                                               _SPI_current->processed = qcdata.nprocessed;

One of the advantages to the patch is that the commandTag for a portal is not overwritten by the commandTag in the
QueryCompletionData,meaning for example that if an EXECUTE command returns the string “UPDATE 0”, the
portal->commandTagremains COMMANDTAG_EXECUTE while the qcdata.commandTag becomes COMMANDTAG_UPDATE.  This could be
helpfulto code trying to track how many operations of a given type have run. 

In event_trigger.c, in master there are ad-hoc comparisons against c-strings:

-       /*
-        * 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)

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:

I think this is easier to read, verify, and maintain.  The compiler can help if you leave a command tag out of the
list,which the compiler cannot help discover in master as it is currently written.  But I also think all those
pg_strcasecmpcalls are likely more expensive at runtime. 

In master, EventTriggerCacheItem tracks a sorted array of palloc’d cstrings.  In the patch, that becomes a Bitmapset
overthe enum: 

typedef struct
 {
        Oid                     fnoid;                  /* function to be called */
        char            enabled;                /* as SESSION_REPLICATION_ROLE_* */
-       int                     ntags;                  /* number of command tags */
-       char      **tag;                        /* command tags in SORTED order */
+       Bitmapset  *tagset;                     /* command tags, or NULL if empty */
 } EventTriggerCacheItem;

The code in evtcache.c is shorter and, in my opinion, easier to read.  In filter_event_trigger, rather than running
bsearchthrough a sorted array of strings, it just runs bms_is_member. 

I’ve kept this change to the event trigger code in its own separate patch file, to make the change easier to review in
isolation.

> I’ll include stats about code-size and speed when I post v2.

The benchmarks are from tpc-b_96.sql.  I think I’ll need to adjust the benchmark to put more emphasis on the particular
codethat I’m changing, but I have run this standard benchmark for this email: 

For master (1fd687a035):

    postgresql % find src -type f -name "*.c" -or -name "*.h" | xargs cat | wc
     1482117 5690660 45256959

    postgresql % find src -type f -name "*.o" | xargs cat | wc
       38283  476264 18999164

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





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