On connect, set all variables in a single SET statement #1099
Conversation
When opening a connection, instead of iterating on all variables and calling "SET <variable>=<value>" for each, we now use a single SET statement with all pairs using a comma as separator: SET <variable1>=<value1>,<variable2>=<value2>,... MySQL doc about the SET statement: https://dev.mysql.com/doc/refman/8.0/en/set-variable.html
There was a problem hiding this comment.
LGTM, thanks!
Travis is driving me a little crazy lately... I have no idea why we have two status checks here now and how to make it pass. Restarting the job it is blocked on just updates the other status check.
Could you try to rebase this on the current master before I disable Travis as a required check?
| if err != nil { | ||
| return | ||
| } | ||
| params = append(params, param+"="+val) |
There was a problem hiding this comment.
nitpick (not a must): This allocates a new string for every param. Maybe using strings.Builder (and adding the , here) is the better approach?
There was a problem hiding this comment.
I know there are improvement in this area. For this important pull request I wanted to apply the minimum change required. The string concatenation problem was in the original code, so I didn't want to change that and divert the reviewers from the main issue.
Now that this structural change is merged (thanks!), I am now able to propose some further implementation improvements.
…r#1099) When opening a connection, instead of iterating on all variables and calling "SET <variable>=<value>" for each, we now use a single SET statement with all pairs using a comma as separator: SET <variable1>=<value1>,<variable2>=<value2>,...
…r#1099) When opening a connection, instead of iterating on all variables and calling "SET <variable>=<value>" for each, we now use a single SET statement with all pairs using a comma as separator: SET <variable1>=<value1>,<variable2>=<value2>,...
Description
For faster connection startup this patch use (almost) a single
SETstatement to set all variables at once instead of aSETstatement for each variable.MySQL doc about the
SETstatement: https://dev.mysql.com/doc/refman/8.0/en/set-variable.htmlBefore:
SET <variable1>=<value1>SET NAMES utf8mb4SET <variable2>=<value2>SET <variable3>=<value3>Now:
SET NAMES utf8mb4SET <variable1>=<value1>,<variable2>=<value2>,<variable3>=<value3>This patch should make establishing new connection must faster as it reduce the number of roundtrips necessary before the first query when multiple variables are defined in the DSN.
As a side effect, the
SET NAMESstatement is always executed first. This will fix the possibility of someSETstatements being executed before the right charset is set (for the case where the database default charset is not the one we want to use for the connection, e.g. Latin1).Here is how to test this patch with your own Go project built with Go modules:
Background: on MySQL 5.7 database hosted on Amazon RDS we noticed that in Performance Insights that some costly queries were hidden by
SETstatements. Sometimes we saw aSET time_zone=..., sometimes aSET group_concat_max_len=..., sometimesSET sql_mode='TRADITIONAL'.Checklist