MySQL runs preg_replace_callback needlessly and there is code duplication

The summary of this issue seems to be:

- chx has found a small optimization that can be made by simply doing sprintf rather than preg_replace_callback(). While there appears to be no measurable difference in performance with just core, logically, preg_replace_callback is going to be slower. And multiply that by sites that take 200-500 queries per page in order to generate, and you're looking at something probably fairly substantial.
- hswong believes that we the _db_query_callback makes a lot more sense as a general API function for all databases, rather than each db backend having to implement its own, and believes that moving this logic to the db-specific abstraction layers is a step backwards. However, he admits he is not using the core _db_query_callback.
- Currently in Drupal 6, in order to workaround for the fact that D6 doesn't support LOB handling [http://drupal.org/node/147947], each database abstraction layer that requires it needs to implement this in its own file anyway. This is why chx is frustrated. However, this kind of code duplication isn't very clean (since more database systems support LOBs than don't) and will need to be refactored in D7 when we have "proper" database abstraction handling, and that's why hswong is frustrated.

Could we please get a final decision on this from someone on high?

I just can say nothing other than code implementation, and its benchmarking result: http://drupal.org/node/172541#comment-298711.

With keeping _db_query_callback() as public for number of benefits (on the other hand, extend its abstraction with number of new features), we are still able to keep our overall performance as existing implementation:

                  +-------------+---------------+---------------+
                  | c1, n500    | c5, n500      | c10, n500     |
+-----------------+-------------+---------------+---------------+
| D6 HEAD         | 311 +- 33.5 | 1538 +- 337.9 | 3031 +- 736.6 |
+                 +-------------+---------------+---------------+
|                 | 309 +- 29.8 | 1504 +- 634.7 | 2972 +- 762.9 |
+                 +-------------+---------------+---------------+
|                 | 305 +- 30.8 | 1497 +- 330.8 | 3040 +- 702.0 |
+-----------------+-------------+---------------+---------------+
| D6 HEAD + patch | 304 +- 30.4 | 1526 +- 325.0 | 3015 +- 558.9 |
+                 +-------------+---------------+---------------+
|                 | 308 +- 30.5 | 1582 +- 826.6 | 3003 +- 739.0 |
+                 +-------------+---------------+---------------+
|                 | 307 +- 25.6 | 1515 +- 307.3 | 2974 +- 853.7 |
+-----------------+-------------+---------------+---------------+

Hope we can further more review about the actually needs of isolating (or extending) _db_query_callback(). The patch provided is just used as technical preview for D7 (even though it is working out with D6 CVS HEAD, with number of benefits), but not trying to promote as core feature within D6. Surly that we are always able to re-roll it back into current implementation in future, but isn't it really meaningful for doing that? I will not oppose any action taken by core committer, but just trying to express my concern by code, testing, and benchmarking. Please take it easy, and simply ignore this comment if you think it is something meaningless, childish, and not professional enough :)

the change seems very logical, and tested as function; i have no comment about it functionality, other than previous D7 technical preview; i guess we will asking for its benchmarking result, in order to have a final judgment, and so here it is (i reuse all DB, hardware, and drupal setting as http://drupal.org/node/172541#comment-298711):

                  +-------------+---------------+---------------+
                  | c1, n500    | c5, n500      | c10, n500     |
+-----------------+-------------+---------------+---------------+
| D6 HEAD         | 313 +- 49.3 | 1461 +- 211.7 | 2920 +- 712.3 |
+                 +-------------+---------------+---------------+
|                 | 302 +- 37.7 | 1491 +- 224.2 | 2995 +- 645.0 |
+                 +-------------+---------------+---------------+
|                 | 317 +- 79.8 | 1491 +- 244.6 | 2956 +- 877.6 |
+-----------------+-------------+---------------+---------------+
| D6 HEAD + patch | 303 +- 29.6 | 1469 +- 270.2 | 2898 +- 640.2 |
+                 +-------------+---------------+---------------+
|                 | 305 +- 26.4 | 1480 +- 273.6 | 2931 +- 597.3 |
+                 +-------------+---------------+---------------+
|                 | 302 +- 25.0 | 1472 +- 320.4 | 2960 +- 791.5 |
+-----------------+-------------+---------------+---------------+

hope this benchmarking result can speed up the discussion, and also help about finalizing it. hope we will able to release beta1 as soon as possible :)

I don't see why this is 'critical', it looks 'minor' to me but I'll settle with 'normal'. I'm going to hold off committing this patch until I had the time to read up on the forked thread. I'll make my own judgement about what is "totally, absolutely off topic".

I'm going to mark this "postponed". The performance tests show that this doesn't buy us anything so we can defer this discussion to Drupal 7. No harm done postponing this.

@chx: First of all, if you are questioning about the effectiveness (or even its fairness) of these benchmarking result, I will suggest for a benchmarking (or something relatively solid than just talking about "There is no doubt that preg_replace_callback is slower.") from YOU. On the other hand, I would like to quote my previous comment once again (http://drupal.org/node/172008#comment-299398):

BTW, if this benchmarking it not meaningful, even though having 3 different concurrency level (1, 5, 10), with each 3 set of result samples, for totally 18 times of benchmarking on top of same platform and settings, i would really interesting about what you would like to believe with?

i don't care to do a much larger scale of benchmarking if i am able to do so (with the help of a very simple script, all we need is time), but may you give me some hints about how can let you believe it as a scientific benchmarking result? i really hope to know what should we believe with, other than these solid result :(

Secondly, the need of the function hook is very clear: this is a very important hook for ALL OTHER DATABSE DRIVER (including pgsql and pdo_mysql), the remove of this function hook will ONLY benefit on current mysql and mysqli implementation. I would like to refer this topic to my latest research result (http://drupal.org/node/172541, http://edin.no-ip.com/html/?q=node/310 and http://edin.no-ip.com/html/?q=node/312), and it is something shouldn't mention once and once again within this issue. Please feel free to comment about that progress if you have any question.

Moreover, D7 is not too far from today: it should be open after 1~1.5 month from today, as we will have D6 beat1 within days. This is according to previous case in D5: we have beta2 1 month after beta1, and stable release 4~5 days after beta2. This case of D6 and D7. Without a relatively solid supporting, we shouldn't be too hurry about committing this patch (as it just propose 4 days before scheduled beta1 release), but postpone this discussion into D7.

Finally, the only comment that I can give about this patch is: THIS IS A CONSPIRACY! This is the first step of dropping ALL THE CHANCES of other databases supporting from Drupal core (including PgSQL and pdo_mysql)! The remove of preg_replace_callback() will give a very BAD IMAGE to other developers, when we are talking about develop other database driver for Drupal core: we will seems the need of this useful function hook as a lossy and meaningless additional handling; but it is required for ALL databases driver, just other than today's mysql/mysqli drivers; the contribution of other databases will become something not favour, as MySQL already own our current 90%+ of user base, and it don't need this additional handling. I guess your position is need not to doubt: this patch is not talking about performance boost nor reduce difficulties of code maintenance, but a STRIKE to other databases driver development (http://www.drupal4hu.com/node/64)!:

I am now officially against the pipe dream of database abstraction. It does not exist, SQL is just not standard enough despite all appearances. I want Drupal core to work with MySQL and that's it. Do you want to know the truth? This is already the state of affairs just noone admits it. Prove me wrong -- contact me if the number of hours you spent on testing Drupal 6 with PostgreSQL is bigger than twenty. Please be aware that I will probably publish your email.

Again, let the guys do whatever they want in contrib and leave core alone. I am in support of PDO to make their work easier but I do not want to waste my time ever again chasing down a similar issue.

I would be surprised if there would be more than ten percent who runs postgresql -- could be as low as five. With MySQL 5.0 (and soon 5.1), the incentives to use PostgreSQL are less and less.

P.S. your position about keeping abstraction layer for PDO supporting is totally meaningless: if MySQL 5.0 (and soon 5.1) will be the best, and should be the one and the only one of our choose (says together with PHP 5.x) on today, we should DROP ALL DATABASE ABSTRACTION LAYER FROM TODAY, AND WRITE NATIVE PHP 5.x + MYSQL 5.0/5.1 QUERIES WITHIN DRUPAL CORE! If we are considering about performance boost, reduce buggy code, and decrease the difficulties of development, it is meaningless for keeping these fancy and lossy abstraction, even for backward compatibility with MySQL 3.x/4.x, and forward compatibility with PDO + MySQL. Building an abstraction layer without consider about the needs of other databases is just a very silly joke (http://www.drupal4hu.com/node/64) :

We need to keep our db_* functions so if you want to do something different in a contrib database driver, then you can. We might want to use PDO to make the contrib authors' work easier, our system better (by standardizing on placeholders), but I do not want to do any more work because we pretend to support other databases. Use preg_replace on the queries or whatever, I do not want to care any more.

This issue will be "won't fix"ed then. No one will get killed by not having this in D6, and it leaves the door open for more database backend exploration work. It's irrelevant for Drupal 7 anyway.