Currently connection options are set using $this->init('SET NAMES...'), $this->init('SET sql_mode=...') to set NAMES/COLLATE and SQL_MODE. This patch moves the connection options into a configurable array. The basis for this patch being that we are unable to modify the sql_mode used when connecting to MySQL - which has been associated with a number of bugs and unexpected behavior (#1136854: MySQL 5.5 breaks speedy testing).
Sampling from MySQL changelogs:
The mysql_odbc_escape_string() C API function has been removed. It has multi-byte character escaping issues, doesn't honor the NO_BACKSLASH_ESCAPES SQL mode and is not needed anymore by Connector/ODBC as of 3.51.17. (Bug #29592, #41728)
Changing the SQL mode to cause dates with “zero” parts to be considered invalid (such as '1000-00-00') could result in indexed and nonindexed searches returning different results for a column that contained such dates. (Bug #31928)
Incompatible Change: With ONLY_FULL_GROUP_BY SQL mode enabled, queries such as SELECT a FROM t1 HAVING COUNT(*)>2 were not being rejected as they should have been. (Bug #31794)
mysql_install_db failed if the server was running with an SQL mode of TRADITIONAL. This program now resets the SQL mode internally to avoid this problem. (Bug #34159)
The NO_BACKSLASH_ESCAPES SQL mode was ignored for LOAD DATA INFILE and SELECT INTO ... OUTFILE. The setting is taken into account now. (Bug #37114)
Prepared statements permitted invalid dates to be inserted when the ALLOW_INVALID_DATES SQL mode was not enabled. (Bug #40365)
I hit an issue with ANSI_QUOTES and tungsten-replicator (unsupported) that breaks replication, and was surprised to find the sql_mode's were hardcoded.
The goal for this issue is to make sql_mode configurable. I see this having two paths:
- a one-off for sql_mode
- a rewrite of the handling of mysql connection options/init commands
Update: we are unable to execute more than one query in MYSQL_ATTR_INIT_COMMAND (https://bugs.php.net/bug.php?id=44081)
Comments
Comment #1
basic CreditAttribution: basic commentedThis also adds functionality of setting SQL Modes via settings.php.
Comment #4
basic CreditAttribution: basic commentedThis patch cleans up how the default expr's are set for names, collation and sql_mode, and adds an extra array that accepts custom connection/init variables. I like the idea of keeping defaults set in database.inc but allowing an easy override as well as ability to add additional connection options.
Comment #5
basic CreditAttribution: basic commentedI just thought of a case where one would want to set sql_mode=''. Added additional logic for this.
Comment #6
bdragon CreditAttribution: bdragon commentedSetting to CNW. The patch removes some important documentation regarding WHY our default mode is what it is.
Also, I think the default.settings.php documentation needs to have a better warning that normal users should not monkey with the defaults.
Comment #7
basic CreditAttribution: basic commentedI've updated documentation as requested. I've also added the ability to override postgres init options and documented them.
Comment #8
basic CreditAttribution: basic commentedAfter a discussion with chx about how to best implement this for D8, we decided this was clean, simple and allows more flexibility for the 1% of users that need it.
Comment #10
bdragon CreditAttribution: bdragon commented#8: configurable_sql_mode-1309278-8.patch queued for re-testing.
Comment #12
basic CreditAttribution: basic commentedA bug in the PDO driver appears to prevent us from executing more than one query in MYSQL_ATTR_INIT_COMMAND (https://bugs.php.net/bug.php?id=44081).
I've moved these back to $this->query() and they pass local testing.
Comment #13
basic CreditAttribution: basic commentedOops, I realized that schema.inc also needed to be updated, and we were using exec() instead of query() here.
Comment #14
basic CreditAttribution: basic commentedClean up of schema.inc implementation
Comment #15
sunTouching the SQL MODE is fine.
But before touching charsets or collations, you need to read all nitty-gritty details in #772678: Database default collation is not respected
Comment #15.0
sunUpdate description to reflect bug in PDO
Comment #15.1
basic CreditAttribution: basic commentedAdd more detail to Issue description
Comment #16
basic CreditAttribution: basic commentedComment #17
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedClosed #1308874: Unable to override sql_mode as a duplicate. First get this done, then backport (maybe).
Comment #18
basic CreditAttribution: basic commentedWe need to make a decision on whether we want to:
Each approach has some pros and cons. We can be more strict about a one-off for 'sql_mode', but as soon as we add another default connection option for mysql, we have to add another one-off. Making all init_commands configurable gives us all the flexibility in the world, thus making it easier to accidentally break installations.
Before this issue can progress, I believe we need to decide on what changes we are comfortable with (a one-off or a new init_commands array).
Thoughts? Questions?
NOTE: this is a MySQL specific issue - sql_mode was one of the first things ripped out of Drizzle.
Comment #19
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAs discussed in IRC I am in favor of having init_commands instead of extra properties for every connection setting I'll ever need. SQL_BIG_SELECTS just to name one other.
However I don't think we should include charsets, collations and what not there. Drupal uses UTF-8.
Comment #20
basic CreditAttribution: basic commentedI am leaning toward a solution that leaves alone the current handling of SET NAMES, and COLLATE - where we always SET NAMES utf8, and only ['database']['collation'] allows for an override. We can then move sql_mode to ['database']['init_commands']['sql_mode'], which allows defaults to be overridden and arbitrary init_commands to be defined (such as ['database']['init_commands']['big_selects'] => 'SET SQL_BIG_SELECTS=1').
Comment #21
Crell CreditAttribution: Crell commentedI would much rather go minimalist here. We spent probably 80 hours or more between a half dozen people trying to figure out the right magic incantation of configuration options to make MySQL behave like a standard SQL database rather than a 1997-era card catalog that tries to be smarter than you (aka MySQL 3). I am extremely reluctant to let people break that who don't know what they're doing (which is pretty much everyone, including me).
Changing the character encoding is asking for trouble, as you can't change the encoding of the HTTP traffic. Try to write to the database in a different character set than the data coming from the browser and you're in for a world of pain.
There is an "out" now for really edge cases: Subclass the MySQL driver to your own custom driver and change the settings there. Works great. :-)
So I'm not entirely convinced the benefit here is worth the risk of toppling the house of cards that is MySQL.
Comment #22
basic CreditAttribution: basic commentedI think the ability to change character encoding is agreed upon - this patch won't be doing that. This patch includes the init_commands array to enable users to set other non-default session variables, and moves SET sql_mode to the init_commands array.
Comment #23
basic CreditAttribution: basic commentedComment #24
Crell CreditAttribution: Crell commentedI am more comfortable with #22, although still skeptical. I'd like to get David Strauss to weigh in here.
Comment #25
basic CreditAttribution: basic commentedI sent David an email with details on the issue (as requested by him). I'm hoping he has some time to reply here soon.
Comment #26
chx CreditAttribution: chx commented@CrellI I think there are N ways to configure Drupal from settings.php in a way that it breaks rather subtly. Also in the light of #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols) allowing overriding SET NAMES is a good idea, isn't it?
Comment #27
David StraussI have a couple questions about the use cases supporting the inclusion of this patch:
* Is it possible to achieve the same changes by updating my.cnf? Because this patch is to support a <1% use case, a solution that involves re-configuring MySQL is acceptable.
* Is it possible to just do this by sub-classing the MySQL driver? Again, for <1% use cases, heavy solutions are okay.
Most of the changelog excerpts and examples are not terribly convincing for why Drupal should offer yet another configuration option, especially in the form of advertising it in default.settings.php. Pretty much all of the excepts and examples either don't substantially affect Drupal projects of any size (like issues around stored procedures), are about accommodating bugs in fairly obscure systems (like tungsten-replicator), or are only encountered outside Drupal's DB interface (like running INFILE from outside Drupal).
Comment #28
basic CreditAttribution: basic commentedIs it possible to achieve the same changes by updating my.cnf? Because this patch is to support a <1% use case, a solution that involves re-configuring MySQL is acceptable.
No, Drupal sets names, collation and sql_mode per session. This overrides any defaults set in my.cnf (which is good, default is still latin1).
Is it possible to just do this by sub-classing the MySQL driver? Again, for <1% use cases, heavy solutions are okay.
Yes, but sub-classing the driver to work around mysql session variables that we hardcode in the driver doesn't seem like the best way forward. Most people will just hack core's mysql driver to work around issues. Why not make this easier to achieve without hacking core?
#1149372: Don't hack core: sql_mode
#1136854: MySQL 5.5 breaks speedy testing
#1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols)
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedI'm with basic here: it is really useful in the special cases to be able to modify connection options and initialization commands. Of course, you can *always* subclass the driver, but doing so will break a lot of things that actually rely on the driver name (especially: Drush).
I think we should extend this concept a little bit:
PDO::ATTR_TIMEOUT
which controls the amount of time the driver will wait for a connection to be established, and is awfully long by default - 30s)Comment #30
sun@Damien Tournoud's suggestions make sense to me.
However, let's keep the charset issue separated in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols), please.
Comment #31
basic CreditAttribution: basic commented@DamZ
PDO options, init commands, or both? Other database engines handle collation/encoding a little differently, and sql_mode is a mysql specific setting - I'm sure you already know that drizzle removed sql_mode's (and forces utf8).
This is something that I think makes sense to allow modifying across all database drivers. It looks to be pretty straight forward.
Comment #32
Damien Tournoud CreditAttribution: Damien Tournoud commentedI was thinking both init commands and PDO driver options. Of course, both of those have different meanings on different database engines, but both are useful.
Comment #33
David StraussI'm more okay with this change if equivalent functionality is available for other engines.
On the issue of not being able to use a subclassed driver, that sounds like a bug. Code should be checking whether something is an instance of the type they need, not whether the class names matches a string.
Comment #34
basic CreditAttribution: basic commentedThis adds support for setting pdo driver options and init commands via settings.php for all database drivers.
Comment #36
basic CreditAttribution: basic commentedRewrite for the new file structure.
Comment #37
basic CreditAttribution: basic commentedUsing git format-patch. Why? Because everyone else is doing it.
Comment #38
basic CreditAttribution: basic commentedFixing comments and documentation.
Comment #39
Damien Tournoud CreditAttribution: Damien Tournoud commentedApproved here. This gives some much needed flexibility in tuning those parameters, which is especially important in large-scale deployments.
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd I would like to see this backported to 7.x as well.
Comment #41
basic CreditAttribution: basic commentedBackport to 7.x
Comment #42
basic CreditAttribution: basic commentedAssuming tests pass for 7.x, moving back to 8.x
Comment #43
sunLooks good to me as well, and safe for backporting.
Comment #44
catchAssigning this to Crell for a final glance over if he's got time. I'm happy committing this myself though.
Comment #45
webchickFixing tag.
Also confirmed with Crell on IRC that he'd like one last look at this before committing. Bumping back down to needs review.
Comment #46
Crell CreditAttribution: Crell commentedUgh. Sorry for taking so long to respond here, everyone. I'm more comfortable with the patch in #41 (for both D7 and D8), with the following caveat:
While this is true, I believe it should have a big warning on it that the default settings are designed for database portability and changing them may cause unexpected behavior, including potentially data loss. This is NOT something that anyone should be playing with without really knowing what they're doing.
Let's improve the documentation there to warn people away. After that, I'm comfortable committing this.
Comment #47
basic CreditAttribution: basic commentedI worked a bit with webchick to better document this functionality.
How does this look/sound?
Comment #48
Crell CreditAttribution: Crell commented#47 looks fine to me. Thanks!
Comment #49
moshe weitzman CreditAttribution: moshe weitzman commentedThe last sentence in the warning is pure nanny talk IMO. Otherwise, nice patch.
Comment #50
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedHere's a reroll removing the last sentence and moving to the new directory structure.
Comment #51
Crell CreditAttribution: Crell commentedOK, let's do. I'm OK with backporting this as well as there is no API change.
Comment #52
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedAnd the D7 port. Only the paths differ.
Comment #53
catchLooks good to me. Committed/pushed to 8.x.
Moving to 7.x - it'll need a patch with no suffix for the testbot to have a crack at it.
Comment #54
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedYay! The 7.x patch again.
Comment #55
basic CreditAttribution: basic commentedThanks Niklas, I was gone for the Thanksgiving holiday and I'm glad to see this get pushed through. FWIW http://drupal.org/files/configurable_sql_mode-1309278-38.patch was already updated to the new directory structure for 8.x, I hope that wasn't too much trouble :)
Comment #56
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedLol, no, it was pretty straight forward - looks like I must have accidantely started from your D7 backport :)
Comment #57
basic CreditAttribution: basic commentedThis
should beis ready for D7Comment #58
webchickEXCELLENT. This type of backwards-compatible capability enhancement is super exciting to see, and I know Rudy's been working on it for quite awhile. Seems to have buy-in from the DBTNG maintainers, and shouldn't break anything in existing installations. In case it does, we have a good month or so to find out.
Committed and pushed to 7.x! Thanks!!
Comment #59
phayes CreditAttribution: phayes commentedHi Folks,
Forgive me if I'm wrong, but one of the goals of this change is to allow the charset to be configurable in order to allow the following bugs to be fixed:
#1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols)
#1199736: Error on pasting in different language
We need to able able to make the "SET NAMES" command configurable between "utf8" and "utf8mb4"
Comment #60
Niklas Fiekas CreditAttribution: Niklas Fiekas commentedI think we decided to leave the charset out of this, see #15, #19, #30. Not sure, though ...
Comment #61
basic CreditAttribution: basic commentedSET NAMES is a separate issue, lets keep that moving in #1314214: MySQL driver does not support full UTF-8 (emojis, asian symbols, mathematical symbols).
Comment #62.0
(not verified) CreditAttribution: commentedUpdated issue summary.