Use executeStatement for migration SQL

[christiaan]

Q	A
Type	bug
BC Break	no
Fixed issues	#1551

Summary

DbalExecutor now uses Connection::executeStatement (instead of executeQuery) because executeQuery is intended for read-only operations in DBAL.

See https://github.com/doctrine/dbal/blob/05f3985a266bb75c44e621e6a647027016061265/src/Connections/PrimaryReadReplicaConnection.php#L43

[stof]

Replacing the comment explaining why executeQuery is used today is a no-go. This ignores the fact that we tried that in the past, had to revert it due to regressions and added this comment to explain why the code is implemented this way.

-1 for this PR as it will introduce the same regression again.

[christiaan]

Ah thanks for pointing that out! I tried searching but could not find the relevant issues/code related to that rather brief comment. Doing a blame on that line also did not really help.

So if I understand it correctly executeQuery should be used for READ queries. But running an OPTIMIZE or REPAIR query using executeStatement is actually broken in DBAL and throws errors except when abusing executeQuery?

Some context; we had some mutations being accidentally executed on the replica and wanted to guard against that in development. (In production this is fixed by using a read only user for the replica connection.) So we added safeguards to our code that when executeQuery is being used it is only used for READ operations as prescribed by the PrimaryReadReplicaConnection. When building in that safeguard our migrations failed.

[stof]

PDO (and some other DB drivers) have 2 different methods depending on the result of the query (more than on the permission they need):

• an API returning a result set (which needs to be managed in memory)
• an API returning a number of affected rows

Running a query returning a result set through the API that does not expect it will generally cause the DB driver to fail. On the other hand, running a query that does not have a result set through the API supporting result sets won't crash (it will return an empty result set). As doctrine/migrations exposes a single addSql method, it must run those queries in the way that works.

For simple queries, SELECT (doing reads most of the time) returns a result set while UPDATE and DELETE don't. But this is not a 100% guarantee:

• SELECT might require write permissions (when reading the next value of a sequence for instance)
• Postgresql supports using UPDATE statements in Common Table Expressions, allowing to have a query that performs some writes but ends with a read query as the main query.
• some special queries require write permissions but have a result set (REPAIR) while being useful in DB migrations.

PrimaryReadReplicaConnection exposes an API that assumes that executeQuery can be used for read queries submitted to the replica, but it also exposes the ensureConnectedToPrimary method to allow handling cases where you have to issue a write query expecting a result set.

[greg0ire]

Doing a blame on that line also did not really help.

Blame is a limited tool. You should be using the git pickaxe for this. That's how I found about #1071