DatabaseUpdateTestCase::testExpressionUpdate assertion on number of affected rows fails in PostgreSQL

Yesterday I have added comment in that related issue (as I thought that it was "needs work" for D7). But we can consider that issue done (commited D8) a keep open only this issue.

But I am a little bit confused about the direction to solve this in D7. We cannot easily backport D8 fix, as this fix uses new MySQL flag PDO::MYSQL_CLIENT_FOUND_ROWS which was introduced in PHP 5.3.5. But D7 should also support PHP 5.2.4 (or greater), according to INSTALL.txt.

One way is to fix the test only.

Another way to fix this is to go a way with the patch available for D7 (drupal-affected-rows-behavior-805858-51.patch) which adds a workaround for PostgreSQL (like it is in SQLite). But this will introduce new conditions to all update queries and this seem like an overkill to me. It can easily introduce additional complexity of queries and reduce their speed.. We should consider this even if they wanted to apply it only to PostgreSQL.

Also if we go with this patch, then the behavior will be different in D8/D9 and D7, which I do not think is somthing what we want (especially when this PostgreSQL behavior is not really a bug in the true sence, it is a regular PostgreSQL behavior).

So this one is a little bit more complicated as I have seen.

For the PostgreSQL part: don't you think it will be better to have the opt-in instead of changing the current behavior automatically? Maybe this could be more "user-friendly" solution, as I am not sure if this issue causes any troubles in the real world right now (for me it seems like that it only creates the test failure).

But I do not know if we can (in case of opt-in solution) force the testbot to use opt-in - maybe if we update the default settings.php, so all new installations will be using this "fix" and all existing installations will remain without this fix, unless explicitly set in settings.php. Would it work like this?

On the other side, SQLite installations seems to be using this patch for some time, so there the opt-out makes sense.

I also wonder if this existing "typo" is causing any problems right now? I cannot find any bug report to this, so I am curious to try this change what will happen in update queries.

-    $this->connection->query($stmt, $options);
+   $this->connection->query($stmt, array(), $options);

I also agree that we do not want to change anything in MySQL in this Drupal 7 stage, so this patch looks like better than #805858-52.

There are some new failures after this patch, like this:

exception: [Uncaught exception] Line 2284 of includes/database/database.inc:
PDOException: SQLSTATE[22P02]: Invalid text representation: 7 ERROR:  invalid input syntax for type bytea
LINE 2: WHERE ( (name = 'site_name') )AND ( (value  's:14:"Test; \...
                                                      ^: UPDATE simpletest463076variable SET value=:db_update_placeholder_0
WHERE ( (name = :db_condition_placeholder_1) )AND ( (value  :db_condition_placeholder_0) OR (value IS NULL ) ) ; Array()
 in variable_set() (line 1310 of /var/www/html/includes/bootstrap.inc).

This was also mentioned in the last comment in #805858: Affected rows inconsistent across database engines. Worth check if the reason is this change:

-     $this->connection->query($stmt, $options);
+   $this->connection->query($stmt, array(), $options);

The patch has a problem with bytea fields. It adds <> operator to the new conditions, but on the bytea field this operator is not valid. It seems like that the bytea fields should be skipped in unselectNoOpRows().

Ok, what I have found so far:

-    $this->connection->query($stmt, $options);
+    $this->connection->query($stmt, array(), $options);

This was changed also in D8 commit here, so it should be safe to fix it: #2167507: Fix rowCount query usage in pgsql and sqlite drivers . But it is questionable, if we do not have to backport all changes to have it work correctly, especially this:

-     return $stmt->rowCount();

The new test exception caused by patch is unrelated to this query() call change, but it is caused by bytea condition, as I have pointed out in #9.

In the final I still incline to opt-in solution for PostgreSQL - if it is possible to solve the bytea problem (not sure we know the field datatype we are querying).

Hmm, it seems like that in case the bytea field is in query, we need to skip the whole condition addition block for that query. Because if we are updating two columns and one of them is bytea, this code:

      if (in_array($field, $skip_fields)) {
        continue;
      }

will unset only the bytea column, but the second column will be still added as exclusion, so the query will be broken in the final.

This is the original query:

UPDATE simpletest393784test_serialized SET name=:db_update_placeholder_0, info=:db_update_placeholder_1
WHERE  (id = :db_condition_placeholder_1)

And this is the modified query after unselectNoOpRows() (only the name column in being checked additionally):

UPDATE simpletest393784test_serialized SET name=:db_update_placeholder_0, info=:db_update_placeholder_1
WHERE  (id = :db_condition_placeholder_1) AND ( (name <> :db_condition_placeholder_0) OR (name IS NULL ) ) 

But the correct expression should be this, as of patch #5 (which is unfortunately not working as the second column is bytea):

UPDATE simpletest393784test_serialized SET name=:db_update_placeholder_0, info=:db_update_placeholder_1
WHERE  (id = :db_condition_placeholder_2) AND ( (name <> :db_condition_placeholder_0) OR (name IS NULL ) OR (info <> :db_condition_placeholder_1) OR (info IS NULL ) ) 

So in case the bytea column is present in the query, it should not be modified. The result in this case should be:

UPDATE simpletest393784test_serialized SET name=:db_update_placeholder_0, info=:db_update_placeholder_1
WHERE  (id = :db_condition_placeholder_2)

Something like this should work.

Yes, it seems like that this is maybe not the correct way. With these patches the PostgreSQL will still behave differently when bytea field will be in the query. So the inconsistency will be here anyway.

I have also tried replacing the <> with NOT LIKE (which is afterwards modified by prepareQuery() to cast bytea fields to text), but this will cause another new failures on numeric/integer fields. Also the NOT LIKE behavior is not the same as <>, especially if the string contains specific characters.

I am adding a new patch where only test is modified. If we decide to go this way, something like this should work. No interdiff here, as this is a new approach from zero.

Testbot results seems good :)

testExpressionUpdate failure is gone. Apart from some weird/random upgrade tests failures which still needs to be looked at (they also slightly differs in each testbot run), there are only three additional test modules failures. Two are waiting for Fabianx/Framework manager review (#3264826, #3264866) and the other is FileFieldWidgetTestCase (#3264750).