Affected rows inconsistent across database engines

This is by design. Deal with it instead of hacking around it.

There is no bug, the name of the function is db_affected_rows(), not db_matched_rows().

The correct pattern is shown in variable_set():

  db_query("UPDATE {variable} SET value = '%s' WHERE name = '%s'", $serialized_value, $name);
  if (!db_affected_rows()) {
    @db_query("INSERT INTO {variable} (name, value) VALUES ('%s', '%s')", $name, $serialized_value);
  }

Notice the @ before the INSERT query.

Obviously this is sub-optimal, that's why we have implemented proper MERGE queries in Drupal 7.

It's not at all a bug. Most databases return the number of affected rows as a result of UPDATE and DELETE queries.

And yes, our db_affected_rows() has the same behavior of all the databases we support. That behavior is correctly documented as:

Determine the number of rows changed by the preceding query.

From #638702: insertion errors
I too am getting this since my lucid upgrade, but only with mysqli. mysql seems to work as before. Perhaps the flags passed to mysqli_real_connect() are not respected anymore?

From #22786: db_affected_rows code cleanup
Now that db_affected_rows() returns the number of rows matched instead of only changed we can get rid of the hacks that worked around this.
The referenced commit shows the flags mentioned by cdale: http://drupalcode.org/viewvc/drupal/drupal/includes/database.mysql.inc?r...

It would be nice to have a bigger list of what happened since the 2005 commit by Dries...

Wait a minute... The 2005 code passes integer 2 when connecting, and then immediately does a mysql_db_connect() without passing the connection and thus undoing it all?
EDIT: Sigh, never mind, this is not an issue here. If the link identifier is not specified, the last link opened by mysql_connect() is assumed.

Damien,

There is a bug in MySQL. And since most users use that silly database, providing a fix makes sense.

The documentation is WRONG. The db_affected_rows() function is expected to return the right number: the number of rows that the UPDATE matched. All the functions that do an UPDATE + INSERT are now broken in all of Drupal. Feel free to search and you will see that all the modules everywhere have this issue popping up.

MySQL sucks and I tell my users to switch to PostgreSQL, a true database, but that's not going to happen.

Somehow, the flag (2 or MYSQLI_CLIENT_FOUND_ROWS) was used to make sure the right result was being returned. That seems to not work any more with certain versions of MySQL. The problem is that most people cannot just choose which version to run. Their webhost has chosen for them.

Therefore, a fix in the function that does it wrong is the right solution, even if you do not like it.

Asking 1,000 people to change their code because of a bug in one database system, seems a bit much to me.

The use of the @ for the variable is because you are very likely to have multiple users hit that line of code and Core avoids any locking of the database. The result is pretty much the same, except that locking prevents anyone from reading anything from that table.

For all those tables that pretty much on the Admin will be able to change, the @ should never be necessary. Now if you think that an UPDATE + db_affected_rows() + INSERT is wrong, let me know how you'd do that procedure otherwise?!

Just like it was addressed a while back (circa 2005), it needs to be addressed again (circa 2011?)

Thank you.
Alexis Wilke

Bug of MySQL or not, we will not change that in Drupal 6, which is long frozen. For Drupal 7, PDO only supports the "modified" semantic in MySQL (at least on PHP 5.2). We picked this one as the baseline (see UpdateQuery_sqlite for the impact of this).

Back to by design.

Despite Damien saying that the “rows changed” behavior is by design, it is clear that the preferred behavior is for db_affected_rows() to return the number of rows matched, as evidenced by the MYSQLI_CLIENT_FOUND_ROWS flag being passed to mysqli_real_connect() by Drupal 6. This flag should cause the “matched” behavior, but it does not in many cases due to an apparent bug in the MySQL PHP extension library (mysqli.so). The relevant code, in php-5.3.3/ext/mysqli/mysqli_nonapi.c:

#if !defined(MYSQLI_USE_MYSQLND)
    if (mysql_real_connect(mysql->mysql, hostname, username, passwd, dbname, port, socket, CLIENT_MULTI_RESULTS) == NULL)
#else
    if (mysqlnd_connect(mysql->mysql, hostname, username, passwd, passwd_len, dbname, dbname_len,
                        port, socket, flags TSRMLS_CC) == NULL)
#endif

Note that if the build option is defined to use the MySQL Native Driver, mysqlnd_connect() is properly called with the passed in flags (including MYSQLI_CLIENT_FOUND_ROWS from Drupal). In this case db_affected_rows() after an UPDATE will return the number of matched rows, not the number of changed rows. On platforms where MySQL is built with this option, things work as intended.

However, on many platforms, including Fedora/RHEL/CentOS up through the current Fedora 14, MySQL is built without the MYSQLI_USE_MYSQLND option set, causing mysql_real_connect() to instead be called, ignoring the passed in flags, and therefore causing db_affected_rows() after an UPDATE to return the number of changed rows.

The obvious solution is to fix this code:

--- php-5.3.3/ext/mysqli/mysqli_nonapi.c.dist   2010-05-26 00:28:43.000000000 -0700
+++ php-5.3.3/ext/mysqli/mysqli_nonapi.c        2010-11-29 00:49:18.878514875 -0800
@@ -232,7 +232,7 @@ void mysqli_common_connect(INTERNAL_FUNC
 #endif

 #if !defined(MYSQLI_USE_MYSQLND)
-       if (mysql_real_connect(mysql->mysql, hostname, username, passwd, dbname, port, socket, CLIENT_MULTI_RESULTS) == NULL)
+       if (mysql_real_connect(mysql->mysql, hostname, username, passwd, dbname, port, socket, flags) == NULL)
 #else
        if (mysqlnd_connect(mysql->mysql, hostname, username, passwd, passwd_len, dbname, dbname_len,
                                                port, socket, flags TSRMLS_CC) == NULL)

I don’t think that the original code was intentional—it seems nothing but an oversight in the code revision, shown around line 215 of the relevant code revision. I have submitted this patch to php.net.

The better performing solution would be for PHP to call the MySQL Native Driver, but this is potentially disruptive due to an incompatibility with legacy password fields, which Fedora/RH uses by default.

I know that fixing the MySQL PHP library on your webhost is not an option for many, but I thought it important to document the root cause, and the apparent reason why different people experience different behavior.

Let's quickly discuss what to do here before RC.

The way I understand this:

• Traditionally, MySQL has returned the number of *affected rows* (ie. modified) to a DML query, not the number of *matched rows*. Whether or not this is an infringement to ANSI SQL seems debatable.
• Since InnoDB, this behavior is not really consistent (see http://bugs.mysql.com/bug.php?id=29157): "Whether the number of rows updated is different from the number of rows found is engine specific."
• A flag can control this behavior MYSQL_ATTR_FOUND_ROWS, but it is not supported by PDO before PHP 5.3 (see http://bugs.php.net/44135).

Because MYSQL_ATTR_FOUND_ROWS is not supported by PHP 5.3 *and* because the number of affected rows cannot be reliably returned by MySQL, I fear that we are going to have to:

• Standardize the return value of DML queries to be the *affected rows*.
• Rewrite the UPDATE queries for all engines so that we explicitly only match rows that are going to be changed (ie. rewrite UPDATE table SET a = "val1" WHERE b = "val2" into UPDATE table SET a = "val1" WHERE b = "val2" AND a <> "val1"). The code to do that already exists in the SQLite driver.

Although the matched row count might be preferred over the changed count in some cases, it doesn't look technically feasible to ensure such behavior across all db/php combinations.

I also don't see how rewriting the queries improves the situation much. It just reduces the matched row count and the workaround in #3 would still be necessary.

So always returning affected rows rather than attempting to get matched rows seems like the best option.

Use a merge query. There is nothing else we can do. The return value of DML queries is no way in hell cross db compatible and thanks to PDO it's broken on MySQL too.

This is why Merge queries were added in the first place. Using Update in that fashion doesn't work.

Tempted to close again, but it's definitely not critical.

There is a test for this in core, which is failing on PostgreSQL currently,
basically it updates a field to be field * field (i.e. squares the value) on a table that has one row with the field == 1, which causes the number of rows affected to differ on MySQL and PostgreSQL, causing the test to fail.

Looks like we might need to take the sqlite specific code and apply it to the postgreSQL implementation too.

This is possibly a different issue, but I wanted to flag it up here.

Correct tag

Hijacking this issue based on comment #14 for discussion.

From that comment it seems we need to do:

• Standardize the return value of DML queries to be the affected rows.
• Rewrite the UPDATE queries for all engines so that we explicitly only match rows that are going to be changed (ie. rewrite UPDATE table SET a = "val1" WHERE b = "val2" into UPDATE table SET a = "val1" WHERE b = "val2" AND a <> "val1"). The code to do that already exists in the SQLite driver.

Does that mean that we need to pop the code from the SQLite driver into the parent driver, and then MySQL is a special case that doesn't need it? Or do we put the code in a utility method in the parent driver, and then call that method in all the drivers except MySQL?

Does that mean that we need to pop the code from the SQLite driver into the parent driver, and then MySQL is a special case that doesn't need it? Or do we put the code in a utility method in the parent driver, and then call that method in all the drivers except MySQL?

All the drivers need that. Even on MySQL the count is not guaranteed to be exactly the matched rows.

Pushing back down to normal based on #18.

@marcingy This was marked as 'major' because tests are failing in a secondary environment because of this issue. Has that been addressed in some other issue?

Let's have someone run the tests and report which one fails rather than leaving this as major 'just in case'.

I ran the test suite with PostgreSQL 9.1 and the only test that failed was testExpressionUpdate in UpdateTest.php

Like was mentioned in comment #19, this test fails because there is no condition specified for the UPDATE. So PostgreSQL will say all rows in the table matched whereas MySQL will say all rows matched but only 3 were affected.

Not sure about implementing the workaround the SQLite driver uses since the PostgreSQL driver uses prepared statements. I'm not too familiar with how to add a condition to an UPDATE statement when the condition is an expression either?

Here's a patch that does what #22 asks in D8. Mostly to see what the testbot makes of it.

Testbot likes all kind of changed to Postgres and SQLite specific files because it only runs MySQL. Delete the files and most likely all test are still passed.

We need an environment to test both Postgres and SQLite specific changes!

Patch doesn't apply anymore, but let's first decide on our route to go:

In D8, we're now requiring PHP 5.3.10, so we should be able to set the flag MYSQL_ATTR_FOUND_ROWS.

This should mean that we are now able to consolidate db_affected_rows() behaviour for all db platforms on either the number of matched rows or the number of updated rows, while currently:
- MySQL returns just the changed rows
- Postgres returns all matched rows
- SQLite returns just the changed rows, but only thanks to a workaround (UPDATE table SET a = "val1" WHERE b = "val2" into UPDATE table SET a = "val1" WHERE b = "val2" AND a <> "val1"). )

Our options would be as follows:

A) Return just the changed rows, meaning mean we had to rewrite the UPDATE queries for all engines the way we currently only do for SQLite, because according to #14 and #22, MySQLs changed rows are not reliable anyway.

B) Return all matched rows, meaning we would appropriately set the MYSQL_ATTR_FOUND_ROWS flag on MySQL, remove the SQLite workaround, and be done, with the additional benefit of being in line with most other database platforms around.

The remaining issue with Fedora/RHEL/CentOS (#13) should be fixed with PHP 5.3.5 as well, see bug, changelog.

Is this correct? And if yes, do you agree that we should proceed with option B for D8?
[edit:] And for D7, do you agree we should give option A a try and see how it performs for large updates?

(Adding SQLite tag as well because it might be just as well affected depending on our decision.)

Yes, let's proceed with option (b) for Drupal 8.

According to #26, testExpressionUpdate fails on Postgres because of this inconsistency. Can't currently reproduce because D8 doesn't install on Postgres, but this should be still the case, therefore major priority (test failure in a secondary environment).

@Damien:
Nice, then I'll be preparing a patch.

[edit:]
We're currently having a flag $queryOptions['sqlite_return_matched_rows'] which will now go away in D8.
Now if in D7 we would be rewriting all update queries to consistently return changed rows only, how about implementing something like $queryOptions['return_changed_rows'] in D8, rewriting the update queries in D8 as well thereby mimicking the (then consistently fixed) pre-D8 behavior?

Not at all surprising - just for our documentation:

The SQLite documentation seems to be slightly misleading:

int sqlite3_changes(sqlite3*);
This function returns the number of database rows that were changed or inserted or deleted by the most recently completed SQL statement

However, according to SQLite lead Richard Hipp the behavior is indeed by design, calling matched but not physically changed rows "no-op changes":

UPDATE OR IGNORE means that the change is not applied if it would have resulted in a uniqueness or check constraint violation. The change still occurs if it is a "no-op" change - if the value being changed too is the value that was in the table originally. If you want to avoid a no-op change, use a WHERE clause:
UPDATE OR IGNORE mytable SET idint=1 WHERE idint<>1;

A bit less misleading is PostgreSQL documentation:

The count is the number of rows updated, including matched rows whose values did not change. Note that the number may be less than the number of rows that matched the condition when updates were suppressed by a BEFORE UPDATE trigger. If count is 0, no rows were updated by the query (this is not considered an error).

The documentation of the actual method makes it clear though:

char *PQcmdTuples(PGresult *res);
This function returns a string containing the number of rows affected by the SQL statement that generated the PGresult.

So if there was any doubt, we can now rest assured that this is expected behaviour and not some widespread inaccuracy.

So here is a patch for D8.

Setting PDO::MYSQL_ATTR_FOUND_ROWS when opening the connection should be fine, even though not relevant to non-update queries.
Also, removing the workaround for SQLite and clarifying documentation.
Finally, I refactored the relevant testExpressionUpdate() leaving in just the actual test of algebraic expressions, and actually doing a better job there. The number of affected rows is moved to a separate testUpdateAffectedRows() where we can focus more on that, including a detailed explanation.

Tested the patch against UpdateTest() on both MySQL and SQLite, and everything worked as it should. While performance didn't seem to improve on SQLite, it didn't degrade on MySQL either.

This looks really good. Please consider it RTBC if it passes.

Passed the MySQL tests as well.
Untested on postgreSQL because of #2001350: [meta] Drupal cannot be installed on PostgreSQL. It shouldn't be affected at all, though, because postgreSQL has always been correct, rendering #19 a non-issue, at least for the route we chose.
Don't want to set it RTBC myself though, but maybe someone else wants to do a second review?

#34 proves that we are not relying on the old behavior anywhere in core anymore (I highly suspected that, but that's a good proof). The new behavior is well tested. RTBC.

Committed d790fb7 and pushed to 8.x. Thanks!

Cool, I'll take care of the change notice.
Now what about D7? We're still inconsistent there, too and can't count on MYSQL_ATTR_FOUND_ROWS, so we can only make it consistent on counting really changed rows.
Rewriting all queries like in patch #27 should mostly cut it. I only think that we can implement the D8 behavior as an option only for PHP 5.3.5. So if contrib wants D8 behaviour be backported to D7, contrib can require PHP 5.3.5.

while waiting on the change record,
https://drupal.org/core-gates#documentation

Use tag “Needs change notification” to indicate the change node(s) have not yet been created, and move to a 'critical task'.

Updated issue summary and added a change notification.

Now moving to D7.
Given that MySQL is our primary db platform, given that it is quite easy to rewrite the query, so it matches the behaviour of MyISAM and most versions of InnoDB, given that this already was done as workaround for SQLite, and given that the tests would pass then without modification, I guess we should use the query rewrite workaround for all db drivers.
On MyISAM we actually don't need the query rewrite, and If it should turn out it adds a performance hit we might investigate differentiating between the two engines. But that should be neither necessary nor worth the pain.

Refactored patch in #27 for D7.
Removed 'return_matched_rows' flag as it wouldn't be consistent either. If we want it to be consistent, we'd need yet another workaround for PostgreSQL and SQLite, and I don't think that's worth it.
We probably want to test it in all db scenarios and maybe also do some profiling on MySQL. But first let's see what the MySQL bot says.

Update issue summary using the template

Nice. To be sure this doesn't hurt performance, we could use some profiling on MySQL.
Adjusting tags. Also added related issue to the issue summary.

Move workaround to a protected function avoiding code duplication between generic implementation and pgsql driver.

Oops.

added related issue #1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed

added another related issue

45: d7_return_changed_rows_only_on_update-805858-45.patch queued for re-testing.

@Pancho what do we need to move this forward for D7? #45 seems like the way to go, is there anything specific I can profile?

Adding the updated fields to the WHERE condition improves MySQL performance by 400% in my tests. This should not be delayed any further.

Fails on PostgreSQL.

Works now.

From the Drupal 8 change notice:

In Drupal 7, this used to be inconsistent, depending on database engine and version:
- MySQL / InnoDB: changed rows only
(except on MySQL 5.1.0 - 5.1.24: all rows matched by the query)
- MySQL / MyISAM: changed rows only
- PostgreSQL: all rows matched by the query
- SQLite: changed rows only

However if I'm reading https://www.drupal.org/requirements/database right we never supported MySQL 5.1.0 - 5.1.24 anyway; if so, there is no actual bug affecting sites that use MySQL here. And then the lowest-risk fix by far would be to just fix this in PostgreSQL specifically... this is pretty low-level stuff to be messing with in a stable release more than we have to.

Adding the updated fields to the WHERE condition improves MySQL performance by 400% in my tests.

That however is pretty intriguing :) What specific tests show this kind of performance improvement?

There’s a problem with API module parsing. Something to do with bytea fields. Need to fix that.

Adding a parent issue.

Btw: Do we really need to "fix" this instead of changing the test for this specific case? Because this is not a bug in the true sence, it is a regular PostgreSQL behavior. Introducing new conditions to all update queries just to workaround this seem like an overkill to me. It can easily introduce additional complexity of queries and reduce their speed.. And that is not what we need. Even if we only apply it to PostgreSQL.

Great example is D8/D9 - here is the behavior to report all matched rows (for all database drivers). It does not make sense to make D7 postgreSQL driver to report differently.

I think we can close this issue and carry on any D7 work in #3264471: DatabaseUpdateTestCase::testExpressionUpdate assertion on number of affected rows fails in PostgreSQL. I'll try to ensure credit is carried across as we're definitely using the D7 patches that started off here.

Not sure if that means it'd be best to move this back to D8 and mark as Fixed based on the commit to D8 in #38?

I'll do that for now, but if something else makes more sense that's fine.