Use native MySQL statement preparation via PDO

I cannot recommend this:

• Prepared statements are actually extremely expensive, because take *two* round trips instead of one; most queries are not going to be reused anyway, so this they are prepared in pure waste;
• I can confirm that the C MySQL prepared statement API only supports single statement queries, but I don't see why this is not just a bug. The (very sparse) protocol documentation doesn't even mention this limitation.

Similar: http://stackoverflow.com/questions/2802298/can-phps-pdo-be-limited-to-a-...

If it's possible to do this, we should consider it critical, since Drupal 7 is actually much more vulnerable when there is an SQL injection than Drupal <= 6 becuase of the switch to PDO

We could clearly release without this. Splitting the difference.

The actual error is "SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active. Consider using PDOStatement::fetchAll(). Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute.: SAVEPOINT savepoint_1; Array"

This is being caught and lost in MenuRouterRebuildSubscriber::menuLinksRebuild().

I think the patch attached will get Drupal to install - only posting as it might point people towards the correct fix - as it most certainly is not.

And now without my .htaccess file.

Doh - forgot the whole point of the patch ... the changes in Connection.php. Third time lucky.

Thanks, Greg!

I don't know why we'd be getting that error about unbuffered queries. We already are setting buffered queries to true:

$connection_options['pdo'] += array(
      \PDO::ATTR_ERRMODE => \PDO::ERRMODE_EXCEPTION,
      // So we don't have to mess around with cursors and unbuffered queries by default.
      \PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE,
      // Make sure MySQL returns all matched rows on update queries including
      // rows that actually didn't have to be updated because the values didn't
      // change. This matches common behavior among other database systems.
      \PDO::MYSQL_ATTR_FOUND_ROWS => TRUE,
      // Because MySQL's prepared statements skip the query cache, because it's dumb.
      \PDO::ATTR_EMULATE_PREPARES => TRUE,
    );

5 years ago when we were developing this system MySQL 5.1 mattered. In 2015 I don't care about MySQL 5.1 at all, so the query cache issue is a non-issue. I am open to switching back to native prepares if the performance hit isn't too great. That will require benchmarks.

I'm reasonably sure we're never deliberately shoving multiple queries into one query string, so removing the ability to do so is not a notable downside.

I disagree with the premise of this issue (but not the patch).

Disabling emulated prepares is not about preventing executing multiple queries in a single exec() call. It's about security at a far more fundamental level.

It sends the data separately from the query and hence is 100% immune from injection. Not just the "should be" of emulated prepares or using `->quote()`, but 100% immune. At both a theoretical and practical level.

Prior to MySQL 5.1.21, there were issues such as query cache. Today, those are fixed. D8's minimum requirement was raised to 5.1.21 for that exact reason: https://www.drupal.org/node/1856262

The performance difference is trivial (one TCP round trip per query). If that is significant for you usage, that means you're executing too many queries in the first place.

Please don't compromise security overall because you're making design tradeoffs elsewhere.

The primary bottleneck for Drupal performance has not been the database since Drupal 6, or not in core anyway.

Drupal 7 was (nearly) able to serve authenticated page requests from a warm cache with zero database hits if using the memcache cache backend. The only one we didn't eventually fix was menu_get_item(), but that effort has been revived recently too - #1234830: Revert cache_menu patch removal, add a $conf setting instead (was: cache_menu: huge table size).

I think it's reasonable to have a critical issue open to ensure 8.x doesn't regress from 'at most one non-cache/state database hit with a warm cache on simple pages'.

For cold caches, again in Drupal 7, path alias whitelisting, entity multiple loading and menu link rebuilding all helped to cut down a lot of queries.

Drupal 8 should reduce the number of select queries on cache misses even more, because a lot of that has been replaced by YAML parsing. If this isn't true, that's also worth a critical issue to figure out what's wrong (/me looks sideways at link generation).

Actual sites with contributed modules are a completely different matter, but CMI/state/entity API may help with that too in 8.x on cache misses, and render caching definitely with warm caches.

So from a performance standpoint I'd like to see the following:

1. How many non-cache, non-state queries do we execute with warm caches, and can we get rid of all them?

2. How many select queries do we execute with cold caches (don't care about TCP overhead for insert/update that's bad anyway), and can we reduce them/make them multiple/offload?

3. How much of a regression is the native prepared statement in real terms regardless of #1/2?

We should know all of this before committing a patch here, not sure the answer to any of these should block it - just might mean prioritising being a bit lighter on the database.

see also: #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it

The performance difference is trivial (one TCP round trip per query). If that is significant for you usage, that means you're executing too many queries in the first place.

That's all but insignificant, especially in cloud settings. If you have 1ms round trip between your PHP application server and your MySQL server (which is on the low side in cloud settings), and do 10 queries per page (which is really nothing, even outside of Drupal), you add 10ms to your page processing time.

Patch no longer applies. I'll reroll today.

Rebase + one manual merge, I think due to some of the cache tag changes that got in recently.

I got here from reading On the (in)security of popular open source Content Management Systems written in PHP: Drupal Almost Offers Prepared Statements.

Earlier in the article, they say:

Some servers allow you to cache and reuse query identifiers for multiple prepared queries to reduce round trips.

Could this be a solution for dealing with the caching issue?

As far as I understand it we only allow one statement per query. PDO::MYSQL_ATTR_MULTI_STATEMENTS => FALSE in our mysql driver, see #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it.

So now this comes down to the round trip discussion where query and data is sent separately. An updated working patch would help here to start the profiling.

Removed the MySQL version risk from the issue summary, we depend on MySQL 5.5 for some time now.

OK, as a starting point let's do a fresh run against 8.3 w/ `\PDO::ATTR_EMULATE_PREPARES => FALSE` to see what happens now.

Experiment.

Failing tests revealed these changes:

• We can't reuse the same placeholder multiple times in the same query anymore.
• Some queries now fail earlier (in prepare() instead of execute()) so the failure does not trigger a StatementExecutionFailureEvent.
• Values are not casted to string automatically anymore. So for example using CONCAT_WS() on a string and an int will cause "Illegal mix of collations" error unless we use CONVERT() to cast explicitly.

I had a hard time understanding why StandardPerformanceTest needed to be changed so I'm writing the explanation for reference:
The request in DatabaseStorageExpirable::getMultiple() fails because the table does not exist (and the exception is caught).
With emulated prepares, it fails at execute so the query is still collected by collectPerformanceData().
With native prepares, it fails at prepare so the query is not collected.

I reused this module to do some basic profiling: https://git.drupalcode.org/sandbox/geerlingguy-2308763/-/tree/2.0.x?ref_...

With patch:

Results for 100 test runs
query() Simple - 0.220046 ms
select() Simple - 0.243607 ms
EntityQuery Simple - 1.092334 ms
query() with Joins - 0.848203 ms
select() with Joins - 0.904932 ms

Without patch:

Results for 100 test runs
query() Simple - 0.219357 ms
select() Simple - 0.233397 ms
EntityQuery Simple - 1.066444 ms
query() with Joins - 0.823562 ms
select() with Joins - 0.868673 ms

But we might need more advanced profiling?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest main.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.