In an effort to write module code that works with all of the supported databases, I propose setting up MySQL connections to run in ANSI compatibility mode.

When the connection is first opened, if the following queries are executed, then the connection will be running in ANSI mode.

SET SESSION TRANSACTION ISOLATION LEVEL SERIALIZABLE;
SET SESSION sql_mode = 'ANSI';

This sets the following MySQL options: REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE,ANSI

From mysql.com:

REAL_AS_FLOAT
Treat REAL as a synonym for FLOAT. By default, MySQL treats REAL as a synonym for DOUBLE.
PIPES_AS_CONCAT
Treat || as a string concatenation operator (same as CONCAT()) rather than as a synonym for OR.
ANSI_QUOTES
Treat “"” as an identifier quote character (like the “`” quote character) and not as a string quote character. You can still use “`” to quote identifiers with this mode enabled. With ANSI_QUOTES enabled, you cannot use double quotes to quote literal strings, because it is interpreted as an identifier.
IGNORE_SPACE
Allow spaces between a function name and the “(” character. This causes built-in function names to be treated as reserved words. As a result, identifiers that are the same as function names must be quoted as described in Section 8.2, “Schema Object Names”. For example, because there is a COUNT() function, the use of count as a table name in the following statement causes an error:
mysql> CREATE TABLE count (i INT);
ERROR 1064 (42000): You have an error in your SQL syntax

The table name should be quoted:

mysql> CREATE TABLE `count` (i INT);
Query OK, 0 rows affected (0.00 sec)

The IGNORE_SPACE SQL mode applies to built-in functions, not to user-defined functions or stored functions. It is always allowable to have spaces after a UDF or stored function name, regardless of whether IGNORE_SPACE is enabled.
For further discussion of IGNORE_SPACE, see Section 8.2.3, “Function Name Parsing and Resolution”.

ANSI
This mode changes syntax and behavior to conform more closely to standard SQL.

My main motivation for wanting this is the 'PIPES_AS_CONCAT' option, which allows concatenation to be done the same way as most other databases Drupal uses. (Postgres, SQLite). I'm not sure about Oracle, I think '||' would work there. The only database that would not support this is MS SQL, as it uses '+', although I think it might have a ANSI mode also, but I'm not sure.

The other options only seem to enforce drupal coding standards.

I could easily write a patch to achieve this, but I think it is something that probably warrants a fair bit of discussion.

CommentFileSizeAuthor
#25 mysql-group_by-followup-344575-25.patch1.57 KBcdale
Passed: 7985 passes, 0 fails, 0 exceptions View
#23 mysql-group_by-followup-344575-23.patch862 bytescdale
Passed: 7985 passes, 0 fails, 0 exceptions View
#17 mysql-ansi-344575-17.patch7.29 KBcdale
Failed: Failed to apply patch. View
#15 mysql-ansi-344575-15.patch7.32 KBcdale
Failed: Failed to apply patch. View
#10 mysql-ansi-344575-10.patch7.1 KBcdale
#5 mysql-ansi-344575-5.patch1.96 KBcdale
Failed: Failed to apply patch. View
#4 mysql-ansi-344575-4.patch796 bytescdale
Failed: Failed to apply patch. View
#3 mysql-ansi-344575-3.patch794 bytescdale
Failed: Failed to install HEAD. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Crell’s picture

I am open to it. However, how exactly does this differ from "strict mode", which we have already enabled? I thought that was the "do things the standards-compliant way" flag.

The other caveat is that I suspect the ignore-space flag will be a problem with our current code base. Before we implement this we'd need to roll a patch and try it and see what broke, and fix anything that does. Can you try rolling a small patch and posting it to see what all we'd break by forcing MySQL into ANSI mode?

Also, you don't describe what the transaction isolation setting does. What does it do? :-)

chx’s picture

I am not opposed either...

cdale’s picture

FileSize
794 bytes
Failed: Failed to install HEAD. View

You can read http://dev.mysql.com/doc/refman/5.0/en/server-sql-mode.html for a more detailed explanation, but basically, ANSI changes the syntax and behavior of the SQL, where as the STRICT_ALL_TABLES only modifies inserting data, or invalid data rather. (Error instead of warning).

My setting above did not take into consideration the strict mode settings, so it should really be:

SET SESSION sql_mode = "ANSI,TRADITIONAL";

From mysql.com:

The ANSI options change syntax and behavior to conform more closely to standard SQL.
The TRADITIONAL options make MySQL behave like a “traditional” SQL database system. A simple description of this mode is “give an error instead of a warning” when inserting an incorrect value into a column.

If IGNORE_SPACE is a massive problem, we could use:

REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,TRADITIONAL

This removes the ignore space option.

The transaction isolation setting only affects INNODB tables, and is affected by the value of the autocommit property. From mysql.com:

This level is like REPEATABLE READ, but InnoDB implicitly converts all plain SELECT statements to SELECT ... LOCK IN SHARE MODE if autocommit is disabled. If autocommit is enabled, the SELECT is its own transaction. It therefore is known to be read only and can be serialized if performed as a consistent (non-locking) read and need not block for other transactions. (This means that to force a plain SELECT to block if other transactions have modified the selected rows, you should disable autocommit.).

Probably not really needed for drupal, but it probably would make it more conformant with other DBAs. Only useful for INNODb tables.

I've attached a patch that only adds the sql_mode setting ANSI and TRADITIONAL.

The individual options that ANSI and TRADITIONAL actually include are these:

SET SESSION sql_mode = "STRICT_TRANS_TABLES,STRICT_ALL_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER, REAL_AS_FLOAT,PIPES_AS_CONCAT,ANSI_QUOTES,IGNORE_SPACE";

Sadly, I currently don't have an install with the MySQL PDO extensions, so I can't currently test this myself just yet. Lets see what the bot has to say.

cdale’s picture

Status: Active » Needs review
FileSize
796 bytes
Failed: Failed to apply patch. View

Oops, bad patch. Lets try again.

cdale’s picture

FileSize
1.96 KB
Failed: Failed to apply patch. View

I've added a basic test that should prove MySQL is running in ANSI mode. (Or at least that the PIPES_AS_CONCAT option is set).

I'm not really familiar with the testing framework, or the new database layer, so hopefully this is right. I don't think it is worth adding this test to the suite, though it does show that string concatenation in SQL can now be done across all the main DBA's using the same syntax, which was all I really wanted out of this.

Crell’s picture

Status: Needs review » Needs work

Actually I'd argue that we should have unit tests for the intricacies of the SQL syntax. Instead of a DatabaseANSIConcatTestCase, let's make it a DatabaseAnsiTestCase so we can put a variety of syntax checks in there, each in its own test method. Frankly I think || is a totally moronic syntax to use for concatenation, but then I hate SQL in general. (Which is one reason I pushed so hard for query builders. :-) )

In the basic concat test, It's probably better to use $result->fetchField() and get a single value back rather than fetching a whole column when there's only one thing in it.

The comment in the MySQL driver above the $this->exec() call should also be updated for the new mode.

That the tests all pass from this change is a very good sign, IMO. Unless webchick, Dries, or David Strauss have a major objection I'm +1 on running with this.

cdale’s picture

I agree that the || is awful syntax, though it is what the likes of postgres, sqlite etc.. use. And if we can get all of the different DB drivers to use the same syntax, it should cut back on discrepancies of SQL interpretation between the different DB's, which should also reduce bug reports. And having the ability to concatenate strings in selects, and work the same across all DB's I think is a definite advantage and could allow some of the post processing that is currently done to be done directly in the query itself. At the very least, MySQL should now interpret the syntax exactly the same as postgres.

I'll see if I can put together some tests as Crell suggested.

Tests I can think of from a MySQL perspective/changes this patch makes:

  • PIPES_AS_CONCAT - A few tests concatenating fields from tables, and strings directly.
  • ANSI_QUOTES - Quoting/escaping fields with double quotes.
  • IGNORE_SPACE - I'm not sure this is worth testing.
  • REAL_AS_FLOAT - I wouldn't even know how to test this. Some kind of precision testing I'd say. Not sure the effort is worth it.
  • STRICT_ALL_TABLES - Verify that an insert or update aborts once invalid data is reached.
  • NO_ZERO_IN_DATE - Assert that 2008-00-01, 2008-01-00 are invalid dates, and are not inserted.
  • NO_ZERO_DATE - Assert that 0000-00-00 is not allowed as a date value.
  • ERROR_FOR_DIVISION_BY_ZERO - Assert that when a division by zero occurs in an update or insert, an error is generated instead of NULL being returned/inserted.

Any others? The above only focus on the items this patch changes, though it might be a good time to add in general syntax tests to verify what can and can't be done across different database drivers. i.e. what will port without additional work, and what will not. Does the testbot run tests on different drivers? At the very least, I think the tests should aim for consistency across the drivers. Doing this should also help to cut down on database specific bugs.

Might take me a little while to get my head around the DB layer and all the nice new features/functions, such as $result->fetchField. Any recommended reading? Or is the code the best place to look? :)

Crell’s picture

Right now the test bot only runs MySQL. There's talk of having it run on Postgres and/or SQLite as well, but we first have to get Postgres running all tests successfully. :-)

Let's include only the relevant tests for these changes for now so we can get this in. We can add more syntax checking tests as we go. For some of those it looks like we will need to add more test tables, which is fine.

I agree that much as || sucks as a concatenation operator that if the SQL standard specifies that then we should bite our lip and use it.

As for docs for the DB layer, see here: http://drupal.org/dbtng-documentation

(Hey, I hadn't noticed that it has its own alias now, neat! :-) )

There's still a few things that need to be worked on, but I'm working on it as we speak. The other existing tests should make a good reference, too. You can also track me down in #Drupal. I try to be available for DBTNG questions whenever I can. :-)

David Strauss’s picture

Crell asked for my feedback on IRC.

Here it is: +1

Data integrity and lack of dependence on MySQL quirks is good.

cdale’s picture

Status: Needs work » Needs review
FileSize
7.1 KB

Finally found some time to do this.

I'm not 100% sure about the approach I've taken here, so looking for feedback.

Most of the new tests fail without the change to the sql_mode. The only test I missed from what I mentioned above was the division by zero test. I couldn't get the correct error message using the DBTNG layer.

Damien Tournoud’s picture

We will probably want to add ONLY_FULL_GROUP_BY in the list, to remove those silly "non-sense" partial GROUP BY queries like:

SELECT item_name, item_brand, COUNT(item_stock) FROM item GROUP BY item_brand;

(what item_name this query is supposed to return?)

Also, I'm not sure we want to support ANSI_QUOTES. We should settle on one quoting notation only, and the most frequent is probably the non-ANSI.

Dries’s picture

Status: Needs review » Needs work

- The line $this->exec("SET sql_mode='ANSI,TRADITIONAL'"); could have some more information to describe _why_ we do what we do. Currently, it only explains _what_ we do.

- Can you rename 'date1' to something without a number?

- The phpDoc of DatabaseAnsiSyntaxTestCase should be more verbose; i.e. explain the purpose of the class. You explained the purpose in this issue, but not in the code.

- I agree with all of Damien's comments in #11.

- t('Test handling of invalid data..') has two points.

- t('insert with zero as month in date unexpectedly succeeded.') does not start with a capital letter.

- $this->assertTrue(FALSE, t('Insert with zero as day in date unexpectedly succeeded.')); is indented too much.

- We only seem to test for duplicate data in testInsertBadData, so maybe it should be renamed to testInsertDuplicateData?

- I would consider merging testInvalidDateZeroDay, testInvalidDateZeroMonth() and testInvalidDateZero. Fewer test functions are better, and helps speed things up.

cdale’s picture

I will work on a patch that addresses all of the above issues raised, however, I'm unsure about turning off ANSI_QUOTES. The idea of this patch was to remove inconsistencies in SQL interpretation between the different database drivers. Whilst we opt for not using reserved words for table or field names to avoid the need for quoting, would it not be beneficial to have double quotes behave the same across all database drivers?

As far as I can tell, PostgreSQL, SQLite, DB2, MaxDB, Oracle, and even MsSQL use ANSI quoting. And for my mind, it is not so much an issue of how we quote fields, but more how double quotes are interpreted. Without ANSI quoting, MySQL would allow something akin too SELECT * FROM system WHERE name = "block";, whereas on just about any other database system, this would cause a syntax error, or unexpected results at the very least. This is not so much of a problem with core, but I feel could cause problems with contrib modules.

Given that alot of the community would use MySQL to develop their modules, they may not even be aware that the code they have written will not work on other database systems. Forcing MySQL to use ANSI quoting should cut down on a lot of subtle bugs that only appear when a user tries to run the code on a different database system that the module maintainer might have no idea about.

I'll put together the other notes, and will wait on feedback before removing the ANSI_QUOTES option.

Damien Tournoud’s picture

I read this wrong, of course we want ANSI_QUOTES. There is only one way of quoting strings in Drupal SQL: the single quote way.

cdale’s picture

Status: Needs work » Needs review
FileSize
7.32 KB
Failed: Failed to apply patch. View

Updated patch.

Crell’s picture

Status: Needs review » Needs work

Damient, that's not entirely true. Because most SQL in Drupal uses single quotes where quotes are needed, most SQL queries that I've seen do, in fact, use double-quotes to wrap the entire query. (Arguably with placeholders in D7 that reason goes away, just to keep life interesting.)

That said, I'm +1 on allowing the ANSI standard to work correctly. Enforcing its use may be a bit more difficult. :-)

The docblock on DatabaseAnsiSyntaxTestCase and most of the test methods have an invalid description as it wraps to a second line. If they need longer descriptions, provide a simple one-liner and them a longer paragraph after a blank line within the docblock.

Other than that this looks OK to me.

cdale’s picture

Status: Needs work » Needs review
FileSize
7.29 KB
Failed: Failed to apply patch. View

Re-roll with Crell's docblock suggestions.

Crell’s picture

Status: Needs review » Reviewed & tested by the community
Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

David Strauss’s picture

Status: Fixed » Needs work

I'm getting this when I try to install:

<em>PDOException</em>: <em>SELECT COUNT(*) AS expression
FROM 
{menu_links} menu_links
WHERE  (module = :db_condition_placeholder_16) AND (link_path = :db_condition_placeholder_17)  - 
Array
(
    [:db_condition_placeholder_16] =&gt; system
    [:db_condition_placeholder_17] =&gt; node
)
SQLSTATE[42000]: Syntax error or access violation: 1140 Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause</em> in <em>menu_link_save()</em> (line <em>2061</em> of <em>/home/straussd/domains/transactions.fkbuild.com/public_html/includes/menu.inc</em>).

I'm running MySQL 5.0.67 with InnoDB tables.

David Strauss’s picture

Priority: Normal » Critical

Reverting to "$this->exec('SET sql_mode=STRICT_ALL_TABLES');" fixes the error above for me.

cdale’s picture

That looks like it is caused by the ONLY_FULL_GROUP_BY option. Does setting the mode to ANSI,TRADITIONAL still cause the exception?

cdale’s picture

FileSize
862 bytes
Passed: 7985 passes, 0 fails, 0 exceptions View

I've just done some digging, and have found it is indeed related to the ONLY_FULL_GROUP_BY option. It appears a 'fix' was made in MySQL GA5.0.67 in one of no syntax error on SELECT id FROM t HAVING count(*)>2; or count(*) in order by which has introduced the above error.

A fix has been applied to MySQL 5.0.74 that addresses this. For the bug report describing this, which fits your above error exactly look at Behaviour different for agg functions with & without where - ONLY_FULL_GROUP_BY. The bug also appears to be in 5.1.28, and has been fixed in 5.1.31.

I've attached a follow up patch that removes the ONLY_FULL_GROUP_BY_OPTION for now. The option can be added back in once MySQL makes their next GA release, however, if this option does get put back in, a note should be made that MySQL versions 5.0.52 <=> 5.0.73 AND 5.1.23 <=> 5.1.30 will not work due to the last bug mentioned above.

cdale’s picture

Status: Needs work » Needs review
cdale’s picture

FileSize
1.57 KB
Passed: 7985 passes, 0 fails, 0 exceptions View

While we're here..

Dries’s picture

Status: Needs review » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.

Damien Tournoud’s picture

The date test introduced in that test are not useful (we don't use and don't want to use date types) and brakes badly on SQLite. See #350545: SQLite test failures where I have a patch to remove them.

burningdog’s picture

Status: Closed (fixed) » Active

"TRADITIONAL" is only implemented from MySQL 5.0.2 onwards. Running the query SET sql_mode='ANSI,TRADITIONAL' on my MySQL 4.1.21 installation returns
#1231 - Variable 'sql_mode' can't be set to the value of 'TRADITIONAL'

This breaks the drupal 7 installation on all MySQL versions prior to MySQL 5.0.2 at line 46 of includes/database/mysql/database.inc, which reads
$this->exec("SET sql_mode='ANSI,TRADITIONAL'");

After removing TRADITIONAL so that the SQL query runs, the installation no longer breaks at line 46 but fails just after the installation of the filters module with:

PDOException: SELECT table_name FROM information_schema.tables WHERE (table_schema = :db_condition_placeholder_1) AND (table_name = :db_condition_placeholder_2) - Array ( [:db_condition_placeholder_1] => drupal_7 [:db_condition_placeholder_2] => field_config ) 

SQLSTATE[42S02]: Base table or view not found: 1146 Table 'information_schema.tables' doesn't exist in db_table_exists() (line 2069 of /Users/roger/Sites/drupal/7/includes/database/database.inc).

David Strauss's suggestion to revert to "$this->exec('SET sql_mode=STRICT_ALL_TABLES');" doesn't work for me.

I'm not sure if the installer breaks because it requires the MySQL functionality that TRADITIONAL enforces, or because D7 is not supposed to install on MySQL 4?

Damien Tournoud’s picture

@burningdog: Drupal only supports MySQL > 5.0. It is not supposed to install on any prior version. Now, if you are saying that TRADITIONAL is only implemented on 5.0.2, we may have to bump the version number a little bit.

burningdog’s picture

Ah, that would be it then - thanks Damien :) Just assumed that D7 would work on MySQL 4.

I was wrong about TRADITIONAL only being implemented from 5.0.2 upwards - it was at the end of a sub-paragraph talking about something else. TRADITIONAL should be implemented from 5.0.

burningdog’s picture

Yes, I should've read the INSTALL.txt file before installing Drupal 7 to find out that it doesn't support any version of MySQL before 5, but I wonder if there's a more graceful way of telling the user that during the installation if, like me, they're still on an old version of MySQL (instead of the installation crashing with a seemingly unrelated error - "An HTTP error 500 occurred."). If anyone thinks this is a good idea, I'd be happy to contribute a patch to check for this.

Damien Tournoud’s picture

Status: Active » Closed (fixed)

@burningdog: this is of course welcome. Please open a separate issue for this.