Updated: Comment #44

Problem/Motivation

When updating a database record in D7, according to the documentation UpdateQuery->execute() returned "the number of rows affected by the update", relying on PDO's return value.

It turned out that PDO's number of affected rows was highly dependent on the database engine and therefore not reliable:
In line with the ANSI SQL standard, both PostgreSQL and SQLite define "affected rows" as all rows matched by the query.
For MySQL, the definition however depends: While MyISAM always returned the number of rows that really had to be changed, on InnoDB it used to be the same, but starting with MySQL 5.1 this changed to returning all rows matched by the query.
This has been reverted with MySQL #29157, so InnoDB again matches MyISAM starting with MySQL versions 5.1.24 and 5.5.

With MySQL being our primary database platform, in D7 we implemented a workaround for SQLite but missed PostgreSQL and InnoDB 5.1.0 - 5.1.24. Finally the affected rows are only checked in our MySQL-based tests, but not otherwise used in core.

Proposed resolution

For Drupal 8, we decided to use the PDO::MYSQL_CLIENT_FOUND_ROWS flag which was introduced in 5.3 with PHP #44135, but is only consistently available only from PHP 5.3.5 with PHP #53425, which we're anyway requiring in D8.
This flag ensures consistency of MySQL's behaviour with PostgreSQL and SQLite, without relying on query rewrites or other workarounds.

Remaining tasks

Decide on a backport to Drupal 7 ensuring consistency between all database systems.

User interface changes

None.

API changes

In Drupal 8, update queries always return the number of rows matched by the query, which includes rows that didn't have to be updated because they're values wouldn't have changed.

#1266572: Workaround in UpdateQuery_sqlite for affected rows count causes certain updates to be suppressed
#1542186: PHP 5.4 "Illegal string offset" warning when install

Original report by simg

I've been having problems with the comment_notify module due to the fact that db_affect_rows function returns 0 even if there is an existing row in the comment_notify table for a given uid. This is because mysql returns 0 records on UPDATE if the new values are the same as the existing values.

ie The update statement "update comment_notify_user_settings set node_notify = 0 where uid = 1;"

returns

Query OK, 0 rows affected (0.00 sec)
Rows matched: 1 Changed: 0 Warnings: 0

If node_notify is already 0 (even though a uid = 1 exists !!)

The mysqli_info() function returns the 0 rows affected and not the rows matched = 1.

This problem is described here.

http://www.php.net/manual/en/function.mysql-info.php

I have fixed this by modifying the db_affect_rows() function in includes/database.mysqli.inc with the following code
(includes/database.mysql.inc requires a similar fix but needs to call mysql_info)

function db_affected_rows() {
global $active_db; // mysqli connection resource
$info_str = mysqli_info($active_db);
if (ereg("Records: ([0-9]*)", $info_str, $count) == false) {
ereg("Rows matched: ([0-9]*)", $info_str, $count);
}
return $count;
}

/* old code */
/* this function doesn't work due bugs in mysql (bugs #41283 and #41285) which where mysql doesn't count row updates if the values haven't changed
function db_affected_rows() {
global $active_db; // mysqli connection resource
return mysqli_affected_rows($active_db);
}*/

CommentFileSizeAuthor
#52 drupal-affected-rows-behavior-805858-51.patch5.7 KBDarren Oh
PASSED: [[SimpleTest]]: [MySQL] 40,912 pass(es). View
#52 interdiff-805858-45-51.txt1.14 KBDarren Oh
#45 d7_return_changed_rows_only_on_update-805858-45.patch4.9 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 41,237 pass(es). View
#45 interdiff-42-45.txt4.96 KBPancho
#43 d7_return_changed_rows_only_on_update-805858-42.patch5.24 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es). View
#34 return_all_matched_rows_on_update-805858-34.patch6.06 KBPancho
PASSED: [[SimpleTest]]: [MySQL] 55,373 pass(es). View
#27 drupal-805858-update-query-affected-rows.patch6.7 KBSteven Jones
PASSED: [[SimpleTest]]: [MySQL] 39,975 pass(es). View
#2 database.mysqli-db_affect_rows-805858.patch671 bytessimg
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch database.mysqli-db_affect_rows-805858.patch. Unable to apply patch. See the log in the details link for more information. View
#2 database.mysql-db_affect_rows-805858.patch514 bytessimg
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch database.mysql-db_affect_rows-805858.patch. Unable to apply patch. See the log in the details link for more information. View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Damien Tournoud’s picture

Status: Needs review » Closed (works as designed)

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

simg’s picture

FileSize
514 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch database.mysql-db_affect_rows-805858.patch. Unable to apply patch. See the log in the details link for more information. View
671 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch database.mysqli-db_affect_rows-805858.patch. Unable to apply patch. See the log in the details link for more information. View

Er, how can it be by design ??

The underlying bug in mysql is acknowledged.

And I have dealt with it. Yes, the fix is a little hacky but what is the alternative ?

Attached are patches that to fix database.mysqli.inc and database.mysql.inc

The following pattern which seems to be in use in a number of places throws a warning message - seems to be the cause of a fair few issues in the queue.

db_query('UPDATE {comment_notify_user_settings} SET node_notify = %d, comment_notify = %d WHERE uid = %d', $edit['node_notify_mailalert'], $edit['comment_notify_mailalert'], $user->uid);
if (!db_affected_rows()) {
db_query('INSERT INTO {comment_notify_user_settings} (uid, node_notify, comment_notify) VALUES (%d, %d, %d)', $user->uid, $edit['node_notify_mailalert'], $edit['comment_notify_mailalert']);
}

Damien Tournoud’s picture

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.

simg’s picture

OK, thanks for the info.

Strangely, I can't find any reference to what the @ actually does?

Not that it really matters but shouldn't db_affected_rows() be database independent ? The difference between "affected rows" and "matched rows" seems like a mysql specific concept and is surely a bug in mysql ?

Damien Tournoud’s picture

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

Damien Tournoud’s picture

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.

simg’s picture

Looks like this issue was fixed quite nicely in 2005 but I guess the error has some how crept back in ...

http://drupal.org/node/22786

simg’s picture

Yes, sure. But generally databases don't differentiate between rows changed and rows matched.

Both MS SQL Server and PostgreSQL return the rows matched value whether or not they internally perform a physical write.

Rows matched is the value that should be used by the mysql implementation of db_affect_rows() especially given how it's being used ...

Eric_A’s picture

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...

Eric_A’s picture

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.

AlexisWilke’s picture

Priority: Critical » Normal
Status: Closed (works as designed) » Active

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

Damien Tournoud’s picture

Status: Active » Closed (works as designed)

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.

teliseo’s picture

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.

Damien Tournoud’s picture

Version: 6.16 » 7.x-dev
Priority: Normal » Critical
Status: Closed (works as designed) » Active

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.
carlos8f’s picture

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.

chx’s picture

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.

Crell’s picture

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

Crell’s picture

Priority: Critical » Normal

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

Steven Jones’s picture

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.

Steven Jones’s picture

Issue tags: +PostgreSQL

Correct tag

Steven Jones’s picture

Title: db_affect_rows() not behaving as expected. causing "insert" errors in number of places. » Affected rows inconsistent across database engines
Component: mysql database » database system
Priority: Normal » Major

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?

Damien Tournoud’s picture

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.

marcingy’s picture

Priority: Major » Normal

Pushing back down to normal based on #18.

Steven Jones’s picture

Priority: Normal » Major

@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?

catch’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal

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

posulliv’s picture

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?

Steven Jones’s picture

Status: Active » Needs review
FileSize
6.7 KB
PASSED: [[SimpleTest]]: [MySQL] 39,975 pass(es). View

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

chemical’s picture

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!

Pancho’s picture

Status: Needs review » Needs work
Issue tags: +sqlite

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.)

Damien Tournoud’s picture

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

Pancho’s picture

Priority: Normal » Major

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).

Pancho’s picture

Assigned: Unassigned » Pancho

@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?

Pancho’s picture

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.

Pancho’s picture

Status: Needs work » Needs review
FileSize
6.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55,373 pass(es). View

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.

Damien Tournoud’s picture

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

Pancho’s picture

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?

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

#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.

alexpott’s picture

Title: Affected rows inconsistent across database engines » Change notice: Affected rows inconsistent across database engines
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed d790fb7 and pushed to 8.x. Thanks!

Pancho’s picture

Issue tags: +needs backport to D7

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.

YesCT’s picture

Category: bug » task

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'.

Pancho’s picture

Version: 8.x-dev » 7.x-dev
Category: task » bug
Priority: Critical » Major
Issue tags: -Needs change record

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.

alexpott’s picture

Title: Change notice: Affected rows inconsistent across database engines » Affected rows inconsistent across database engines
Pancho’s picture

Status: Active » Needs review
FileSize
5.24 KB
PASSED: [[SimpleTest]]: [MySQL] 40,341 pass(es). View

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.

Pancho’s picture

Issue summary: View changes

Update issue summary using the template

Pancho’s picture

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.

Pancho’s picture

Issue tags: -Needs profiling
FileSize
4.96 KB
4.9 KB
PASSED: [[SimpleTest]]: [MySQL] 41,237 pass(es). View

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

Pancho’s picture

Issue tags: +Needs profiling

Oops.

Pancho’s picture

Pancho’s picture

Issue summary: View changes

added another related issue

stefan.r’s picture

stefan.r’s picture

@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?

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs profiling

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

Darren Oh’s picture

Status: Reviewed & tested by the community » Needs work

Fails on PostgreSQL.

Darren Oh’s picture

Assigned: Pancho » Unassigned
Status: Needs work » Needs review
FileSize
1.14 KB
5.7 KB
PASSED: [[SimpleTest]]: [MySQL] 40,912 pass(es). View
Darren Oh’s picture

Darren Oh’s picture

Status: Needs review » Reviewed & tested by the community

Works now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 52: drupal-affected-rows-behavior-805858-51.patch, failed testing.

Status: Needs work » Needs review
David_Rothstein’s picture

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?

Darren Oh’s picture

Status: Needs review » Needs work

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

  • alexpott committed d790fb7 on 8.3.x
    Issue #805858 by Pancho, Steven Jones, simg: Fixed Affected rows...

  • alexpott committed d790fb7 on 8.3.x
    Issue #805858 by Pancho, Steven Jones, simg: Fixed Affected rows...

  • alexpott committed d790fb7 on 8.4.x
    Issue #805858 by Pancho, Steven Jones, simg: Fixed Affected rows...

  • alexpott committed d790fb7 on 8.4.x
    Issue #805858 by Pancho, Steven Jones, simg: Fixed Affected rows...