Re: Portal->commandTag as an enum

> On Feb 3, 2020, at 9:41 AM, Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
> In v1, I stayed closer to the existing code structure than you are requesting. I like the direction you’re suggesting that 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 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.

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->commandTag remains COMMANDTAG_EXECUTE while the qcdata.commandTag becomes COMMANDTAG_UPDATE. This could be helpful to 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_strcasecmp calls are likely more expensive at runtime.

In master, EventTriggerCacheItem tracks a sorted array of palloc’d cstrings. In the patch, that becomes a Bitmapset over the 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 bsearch through 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 code that 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