[DX] [BC] Deprecate, reduce use of, or remove DBTNG

[quicksketch]

We should considering deprecating/reducing use of/removing DBTNG in Backdrop to accomplish the following goals:

• Improve performance by removing abstraction around constructing queries.
• Improve learnability by avoiding a Drupal-specific abstraction layer and syntax.

This introduces a need for substitutes in a number of places however:

• Alterability: Remove the ability to alter queries. No more alterableQueries or db_rewrite_sql(). Instead, any query that needs to be altered instead utilizes a View, that can be altered to affect the query. Or, avoid altering entirely through...
• Access control: Make access control queries explicitly defined when creating Views. Instead of automatically restricting listings, require listings to have access control explicitly defined (similar to explicitly defining the "published" state).
• Database connections: We can still use PDO to connect to the database. It's just as fast as a mysqli connection in benchmarks, and makes it so we can use a consistent syntax for doing queries, regardless of the back end.

Things we would NOT remove or build out later include:

• Schema API: As long as we support PostgreSQL (which we should for the initial release at least), Schema API is still needed to define and update tables.
• Entity abstraction: We should focus on ensuring entities are loaded and modified through the entity API alone, making it so it's still possible to cache and store entities in a swappable mechanism. Although things like MongoDB are probably not the target audience, efficient per-entity caching definitely is, so this benefits us for the time being.

The initial effort should be to provide a database system that can update usage of db_query() throughout core with a lightweight replacement directly against PDO. We can work on removing db_select/update/insert/delete queries over time and then remove them (and DBTNG) when finished.

[bojanz]

Having a query builder (db_select()) is super useful for contrib (which is why every system from Doctrine to Drupal has some kind of a query builder). As a contrib maintainer, I use it in most of my modules and would never want to go back to not having it.

I thought this fork was aimed at people who dislike D8, not D7. Haven't met any D7 developers who dislike dbtng (I always viewed it as a big improvement), but maybe I didn't look hard enough.

[mkalkbrenner]

I second @bojanz.

Even with DBTNG you can do D6-style queries using db_query() if you don't want to learn the API. And named parameters withing D7 are an improvement compared to D6 which are easier to understand for newbies than '%s' and %d.

In addition, keeping DBTNG will ease the port of D7 modules to backdrop. Same for Views 3. And we can expect the backdrop 1.0 release earlier.

To deal with performance improvements should be postponed to backdrop 1.1.

[davereid]

I respectfully disagree here, and if this were to land, I don't think I'd be able to participate code-wise with Backdrop from then on out. This is, unfortunately, one of my deal-breaker issues.

[quicksketch]

Thanks guys,

And named parameters withing D7 are an improvement compared to D6 which are easier to understand for newbies than '%s' and %d.

Let's be clear, I'm not saying that we return to Drupal 6's database layer. That was before PDO was introduced that allowed queries to be executed against any database backend in a consistent manner. That means that we wouldn't need have separate .inc files for Postgres and MySQL for example, everything would be handled by PDO in a consistent manner. We would keep named parameters, as they're supported natively by PDO and have numerous advantages: http://us2.php.net/manual/en/pdo.prepare.php

PDO, which provides database connection abstraction isn't the same thing as DBTNG, which is an abstraction around building the queries themselves.

To deal with performance improvements should be postponed to backdrop 1.1.

At least as far as removing APIs, we'd need to wait for 2.x, since changing APIs between minor releases is something we specifically set out to avoid.

@davereid Thanks for being clear about your position on this. I realize it's probably one of the most contentious proposals in Backdrop. Unlike some of our more cowboy issues, we'll make sure this one is viewed from all angles before committing anything.

From our goal outline, our number 1 concern is making code portable from D6 and D7. However, goals 2 (make Backdrop easy to learn) and 5 (make Backdrop fast) are at odds with DBTNG. Before anything is removed, we need hard numbers that DBTNG has a serious impact on performance. If removing DBTNG made Backdrop 10% faster, would it be worth it? What about 15%? Right now it's all hypothetical because we don't know how much impact it has. Unfortunately, DBTNG's impact on learning is hard to quantify, but considering you must learn both SQL and DBTNG anyway, I don't think there's any way that it can be a net positive for learnability.

[maxpearl]

I'm not a DBTNG expert by any means (yes, I even had to look up what the acronym meant), but I wonder if we can get hard (well, no, soft) #s by comparing Drupal 6 with and without this dbtng backport https://drupal.org/project/dbtng? Someone would have to write a module that specifically tested queries with and w/o dbtng. It might give an idea of the performance hit? That might be an easier task than ripping dbtng out of backdrop for testing.

Having written a few minor D6 modules, and porting a couple of modules from 6 to 7, I feel mixed about this suggestion. I kinda liked the abstraction, and as an apprentice module developer, it really wasn't that hard to get my head around it. In some ways, it was more straightforward. So I'm not all that clear that goal 2 is horribly affected by leaving DBTNG in.

[mkalkbrenner]

I did some quick & dirty tests to put some numbers to this discussion. I simply used drupal 7 and drush to execute a query from the ThemeKey module 100000 times in different ways

Using DBTNG, 100000 times preparing and executing the query:

time drush ev 'for ($i = 0; $i < 100000; $i++) { db_select(themekey_properties, "tp")->fields("tp")->condition("enabled", 1)->condition("parent", 0)->condition("value", "", "<>")->orderBy("weight", "asc")->execute(); };'

real    0m49.321s
user    0m29.782s
sys 0m3.001s

Using DBTNG, preparing the query once and executing the query 100000 times:

time drush ev '$query = db_select(themekey_properties, "tp")->fields("tp")->condition("enabled", 1)->condition("parent", 0)->condition("value", "", "<>")->orderBy("weight", "asc"); for ($i = 0; $i < 100000; $i++) { $query->execute(); };'

real    0m26.499s
user    0m9.549s
sys 0m2.530s

Using PDO, 100000 times preparing and executing the query:

time drush ev '$dbh = new PDO("mysql:dbname=test;host=localhost", "test", "test", array(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE, PDO::ATTR_EMULATE_PREPARES => TRUE)); for ($i = 0; $i < 100000; $i++) { $stm = $dbh->prepare("SELECT tp.* FROM themekey_properties tp WHERE (enabled = :db_condition_placeholder_0) AND (parent = :db_condition_placeholder_1) AND (value <> :db_condition_placeholder_2) ORDER BY weight asc", array()); $stm->execute(array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => 0, ":db_condition_placeholder_2" => "")); }'

real    0m19.492s
user    0m3.302s
sys 0m3.505s

Using PDO, preparing the query once and executing the query 100000 times:

time drush ev '$dbh = new PDO("mysql:dbname=test;host=localhost", "test", "test", array(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE, PDO::ATTR_EMULATE_PREPARES => TRUE)); $stm = $dbh->prepare("SELECT tp.* FROM themekey_properties tp WHERE (enabled = :db_condition_placeholder_0) AND (parent = :db_condition_placeholder_1) AND (value <> :db_condition_placeholder_2) ORDER BY weight asc", array()); for ($i = 0; $i < 100000; $i++) { $stm->execute(array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => 0, ":db_condition_placeholder_2" => "")); }'

real    0m19.232s
user    0m2.908s
sys 0m3.430s

Using DBTNG, 200000 times preparing and executing the query:

time drush ev 'for ($i = 0; $i < 200000; $i++) { db_select(themekey_properties, "tp")->fields("tp")->condition("enabled", 1)->condition("parent", 0)->condition("value", "", "<>")->orderBy("weight", "asc")->execute(); };'

real    1m49.697s
user    1m5.402s
sys 0m6.369s

Using PDO, 200000 times preparing and executing the query:

time drush ev '$dbh = new PDO("mysql:dbname=test;host=localhost", "test", "test", array(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE, PDO::ATTR_EMULATE_PREPARES => TRUE)); for ($i = 0; $i < 200000; $i++) { $stm = $dbh->prepare("SELECT tp.* FROM themekey_properties tp WHERE (enabled = :db_condition_placeholder_0) AND (parent = :db_condition_placeholder_1) AND (value <> :db_condition_placeholder_2) ORDER BY weight asc", array()); $stm->execute(array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => 0, ":db_condition_placeholder_2" => "")); }'

real    0m39.947s
user    0m6.279s
sys 0m6.768s

My interpretation of the results:

• Preparing a query using PDO takes "no" time, meaning it's not measurable.
• If we double the number of queries from 100000 to 200000 using PDO, it takes the double amount of time. If we assume that the time for preparing a query in PDO and the real execution time on the MySQL database are not identical, it looks like PDO preparing and executing nearly takes no time. This means the approx. 20 seconds for 100000 queries and 40 seconds for 200000 are nearly the pure query time of MySQL itself.
• DBTNG's query builder puts an (surprisingly) big overhead to the execution time. Building the query in PHP seems to take more time than executing the query on MySQL!

Due to the fact that I didn't expect these results I ask myself if quick&dirty testing is correct or if I missed something like a PDO setting which is required.

From a contrib developer point of view I still want to keep DBTNG and I'm convinced that it will ease the porting of D7 moduls to backdrop.

From a performance point of view, we should remove DBTNG.

On the other hand we should assume that a a normal page request will not cause thousands of queries and does a lot more that just building queries. It will be extremely difficult to measure the average performance gain. So we don't know if it worth the effort to remove DBTNG.

[mkalkbrenner]

I performed two more tests to proof that the query builder causes the performance loss.

Using DBTNG without query builder via db_query(), 100000 times preparing and executing the query:

time drush ev 'for ($i = 0; $i < 100000; $i++) { db_query("SELECT tp.* FROM themekey_properties tp WHERE (enabled = :db_condition_placeholder_0) AND (parent = :db_condition_placeholder_1) AND (value <> :db_condition_placeholder_2) ORDER BY weight asc", array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => 0, ":db_condition_placeholder_2" => "")); } '

real    0m21.534s
user    0m5.959s
sys 0m2.495s

Using DBTNG without query builder via db_query(), 200000 times preparing and executing the query:

time drush ev 'for ($i = 0; $i < 200000; $i++) { db_query("SELECT tp.* FROM themekey_properties tp WHERE (enabled = :db_condition_placeholder_0) AND (parent = :db_condition_placeholder_1) AND (value <> :db_condition_placeholder_2) ORDER BY weight asc", array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => 0, ":db_condition_placeholder_2" => "")); }'

real    0m41.033s
user    0m10.922s
sys 0m4.769s

These numbers are comparable to pure PDO.

Based on these numbers I come back to my solution I proposed yesterday:

Even with DBTNG you can do D6-style queries using db_query() if you don't want to learn the API. In addition, keeping DBTNG will ease the port of D7 modules to backdrop. Same for Views 3. And we can expect the backdrop 1.0 release earlier.

So again, I strongly recommend to keep DBTNG for D7 backwards compatibility and to lower the effort to migrate modules. But for the maximum performance we should start replacing the usage of the query builder by db_query() whenever possible. That's not a must for 1.0 and can be done continuously.

This approach should satisfy both sides: @quicksketch to reintroduce SQL for a part of the targeted audience and to improve performance. And @davereid to keep the query builder for (rare) usage where it makes sense. For example the builder makes sense and causes less errors whenever queries have to be build dynamically using if ... else constructs.

[quicksketch]

Thanks @mkalkbrenner for the synopsis. There are some great numbers in here.

DBTNG's query builder puts an (surprisingly) big overhead to the execution time. Building the query in PHP seems to take more time than executing the query on MySQL!

Wow. This is shocking. I'm sure results would vary, but getting this result is beyond what I would have expected. Perhaps something like query caching was decreasing the amount of time spent retrieving the data.

I believe there's also performance overhead even when using db_query(), since the return object from db_query() is still a Drupal-created object. I think we'd also see a slowdown from the act of iterating of the results, since Drupal has to implement it's own ArrayIterator to do the looping. So even the result of db_query() probably has overhead, even if the query is relatively cheap. We might try amending your test with looping over a result set too, and see what impact that has.

But for the maximum performance we should start replacing the usage of the query builder by db_query() whenever possible.

This is essentially what Drupal itself has been recommending even since DBTNG was introduced. It's also the reason why DBTNG isn't really giving Drupal database abstraction, since developers are discouraged from using it on the grounds of performance.

I strongly recommend to keep DBTNG for D7 backwards compatibility and to lower the effort to migrate modules.

I had considered potentially putting DBTNG's query builder into a module and letting it be a dependency if needed for easing porting, but I'm not sure that'd make anyone happy.

[quicksketch]

Based on @mkalkbrenner's tests, I did 3 separate query approaches (DBTNG, db_query(), and raw PDO) to see what impact looping over the results had. Here are the 3 query approaches:

time drush ev 'for ($i = 0; $i < 10000; $i++) { $result = db_select(block, "b", array('fetch' => PDO::FETCH_ASSOC))->fields("b")->condition("status", 1)->condition("theme", "bartik")->orderBy("region", "asc")->orderBy("weight", "asc")->execute(); foreach ($result as $row) { null; } };'

time drush ev 'for ($i = 0; $i < 10000; $i++) { $result = db_query("SELECT b.* FROM block b WHERE (status = :db_condition_placeholder_0) AND (theme = :db_condition_placeholder_1) ORDER BY region asc, weight asc", array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => "bartik"), array('fetch' => PDO::FETCH_ASSOC)); foreach ($result as $row) { null; } };'

time drush ev '$dbh = new PDO("mysql:dbname=backdrop;host=localhost", "root", "root", array(PDO::MYSQL_ATTR_USE_BUFFERED_QUERY => TRUE, PDO::ATTR_EMULATE_PREPARES => TRUE)); for ($i = 0; $i < 10000; $i++) { $stm = $dbh->prepare("SELECT b.* FROM block b WHERE (status = :db_condition_placeholder_0) AND (theme = :db_condition_placeholder_1) ORDER BY region asc, weight asc", array()); $stm->execute(array(":db_condition_placeholder_0" => 1, ":db_condition_placeholder_1" => "bartik")); while ($row = $stm->fetch(PDO::FETCH_ASSOC)) { null; }}'

All three used PDO::FETCH_ASSOC when looping over the results. I did the tests once with the foreach()/while() loop removed, and once in place to see how much time it took to loop over the results with each approach.

DBTNG no looping: 6.01s
db_query no looping: 2.43s
PDO no looping: 0.87s

DBTNG with looping: 6.57s (0.56s looping only)
db_query with looping: 2.88s (0.45s looping only)
PDO with looping: 1.22s (0.35s looping only)

So this turned up some interesting results:

• Looping is indeed slower with DBTNG and db_query() than it is with PDO directly, but perhaps insignificantly so.
• Something about db_query() still makes it slower than direct PDO when executing a query.

But overall, my concern in my last comment seems to be unwarranted: DBTNG does not have a significant impact on the looping over results.

[mkalkbrenner]

I strongly recommend to keep DBTNG for D7 backwards compatibility and to lower the effort to migrate modules.

I had considered potentially putting DBTNG's query builder into a module and letting it be a dependency if needed for easing porting, but I'm not sure that'd make anyone happy.

@quicksketch I think it will make a lot of people happy to keep DBTNG as it is for 1.0 but provide documentation about best practices how to (not) use it based on our numbers.

[rudiedirkx]

I think DBTNG should stay. It's very useful for reusing sophisticated queries. You can have a base SelectQuery object and extend that with conditions and joins per use case. Sometimes writing SQL is much easier. Sometimes it's not.

If you don't want to use it, just use db_query(). Shouldn't have a performance loss...

[micahw156]

I think it will make a lot of people happy to keep DBTNG as it is for 1.0 but provide documentation about best practices how to (not) use it based on our numbers.

Speaking as one of those "self-taught people who hacks on Drupal to build sites", I built a bunch of stuff this summer based on Services and Services Views. Then I read a blog post that showed how stuff could be made faster using db_select() so I started using that. Then I read in the documentation for db_select() that I should use db_query() wherever possible because it's faster, so I did that for most of my queries. None of it was that hard to figure out, and now my only problem is that I want to go back through the sites I built for this project and replace Views with either db_query() or db_select(), whichever works better, because either of those are significantly faster than Views.

So I guess what I'm trying to say is that -- as a site builder -- if Views is going into Backdrop core, I'd rather see the time and effort be put into narrowing the gap between Views and hand-coded db_select() statements.

Yes, I understand, every low-level performance gain helps. Yes, go through core and make it use db_query() wherever possible, but from a standpoint of prioritizing issues and getting a working product out the door, eliminating DBTNG (and making site and module builders rewrite all of our queries) might not be the most important thing to fix right now.

[deekayen]

When I pictured a modification to the database layer, what I saw was just removing the ability to have pluggable backends (i.e. only support MySQL/MariaDB) on the primary layer.

Supporting PostgreSQL is, to me, like trying to support having some Python scripts. Python is a perfectly legit language, but that doesn't mean it belongs in Drupal/Backdrop. The platform bet on PHP, not a mix of scripting languages. I think it's reasonable to shape this discussion around supporting one DB, too. Maybe that targets schema more than dbtng?

The larger issue I see with DBTNG isn't really performance related as much as personal identity issues. Do you build queries with/from objects in a form which doesn't resemble SQL whatsoever, or do you write SQL with some placeholders? Which do you use when? Will the debate about one being better than the other ever resolve? It's like the cliffhanger end of a 1980s Batman or Rocky and Bullwinkle episode that was never really resolved before D7 launched. It's a compromise that needed a side to win the battle.

[joachim-n]

DBTNG is useful for when you have API functions that take various parameters which affect the query. Without it, you have to assemble the query string from bits of SQL stuck together, which isn't terribly maintainable.

[DirectorHaas]

Thinking just from the contrib module dev outlook, leaving -in- anything from D7 which would be complex and time consuming to rewrite if taken out, like all DBTNG functions, would be good (to leave the calls in).

It may be fairly critical to prove to contrib authors that it's pretty easy to convert most D7 modules to Backdrop 1.0 and have a strong initial adoption rate. While obviously this is not the only issue for Backdrop to succeed, it could be important for Version 1.

I think this is in agreement w/ @davereid "I respectfully disagree here, and if this were to land, I don't think I'd be able to participate code-wise with Backdrop from then on out. This is, unfortunately, one of my deal-breaker issues." unless his comment was attributed to the prior comments instead of the main post.

Obviously, if anyone was depending on the overlay module as the most important issue...well :)

[patcon]

Improve learnability by avoiding a Drupal-specific abstraction layer and syntax.

A less drastic change would be to use this to deal with the above...? https://github.com/doctrine/dbal

[WorldFallz]

I was pointed to this issue by oadaeh from https://github.com/backdrop/backdrop-issues/issues/44.

I guess to basically reiterate my point.

I totally get the engineering for 80% philosophy. But there appears to be enough of desire to retain DBTNG for reasons other than MSSQL compatibility.

And though I'm totally biased, if there isn't really a major performance penalty, and devs can continue to happily ignore DBTNG by using db_query instead of db_select, and as a pleasant side-effect we can include other non mysql/maria/postgres users, I'm really not sure I grok the reasons for removing it.

Feels a little like throwing out the baby with the bathwater imo.

[rlnorthcutt]

As @micahw156 pointed out, its been basic advice to avoid db_select and use db_query when possible, for performance reasons. I have done so and not been hurt at all. I do run across db_select on projects and often don't see why it was chosen... while it is sometimes VERY useful, I've found that its a relative minority. So - removing it doesn't bother me at all.

At the same time, I think its REALLY important to keep the laundry list of changes for 1.0 as low as possible... we have to make some choices and leave some things out. Removing DBTNG and rewriting everything that uses it sounds like a sizable expense that might be better used elsewhere.

As a potential compromise, what about the idea of porting DBTNG to a contrib module? I'm not sure if that would be more/less effort, but it might allow some modules to be ported more easily with an additional dependency.

[yann-yinn]

As a developper i use a lot dbtng (db_select() mainly ) : this makes my life easier when building dynamic / complex / paginated queries; i also do like its tag system that help me several times on projects. This the only new API i was really happy with, as a developper, in D7 ...

[quicksketch]

I totally get the engineering for 80% philosophy. But there appears to be enough of desire to retain DBTNG for reasons other than MSSQL compatibility.

I'm definitely picking up on this sentiment. After hearing the feedback here and from the discussions I've had with developers on Backdrop, I think straight-up removing DBTNG is probably going to be taken off the table for 1.0. We may still deprecate it, move it to separate module, reduce it's usage when possible, or do other things for reasons of performance or learn-ability, but with our number 1 priority being an easy upgrade path, removing DBTNG has a huge cost. I've retitled the issue as such. I'd still like to explore possibilities here and benchmark the overall cost of DBTNG, but from the overall cost-of-change perspective, removing DBTNG has been a tough-sell.

[DirectorHaas]

@quicksketch "but with our number 1 priority being an easy upgrade path" = Cool

( Also, performance gains w/ the registry issue(s) = Nice :+1: )

[matt2000]

Yuck, boo, terrible idea. I just need to be on record. We should keep DBTNG and EntityFieldQuery. I can do in work with EFQ code that takes hundreds of lines of Views code.

[fgm]

There is at least one very good thing in DBTNG and that is support for MERGE queries, the syntax for which varies a lot between DB engines. Over the course of releases, their use has slightly increased (44 in 7.0, 48 in 7.24), and ISTR contrib has been using them more too. It would be a pity to lose that advantage.

[quicksketch]

I really like Merge queries too. Though I don't think DBTNG is needed for this functionality.

I had always thought that merge queries were using something special (like MySQL's ON DUPLICATE KEY UPDATE ability) to do the query in one single pass. But it turns out that merge queries are simply a SELECT, then INSERT/UPDATE query combined together in a single function (see MergeQuery::execute).

If we keep this "merge" query functionality built the same way, we could easily incorporate the ability to do a merge into drupal_write_record(), which oddly supports insert and update but not merge.

[quicksketch]

This week we dropped support for actual database abstraction in #232 and #279, removing SQLite and Postgres support respectively. DBTNG itself is still there, but it's purpose now at this point is primarily for the dynamic query building and compatibility with D7. Again, at this point DBTNG is still going to exist (in some compatible form) for 1.0, but long-term I think we're still likely to look at reducing or eliminating its use. With officially dropping support for non-MySQL databases, having a full-blown abstraction layer for the long-term seems excessive.

[fgm]

Indeed, if Backdrop drops everything but the MySQL variant of SQL, db_merge() becomes far less useful, especially as its MySQL implementation, even on D8, does not use actual MySQL merge query syntax, but a portable sequence of select/(insert|update) : see core/lib/Drupal/Core/Database/Query/Merge.php#execute() without any per-engine optimization : a Backdrop implementation of db_merge() could make use of the MySQL-specific INSERT .. ON DUPLICATE KEY UPDATE syntax.

[quicksketch]

Moving this to 2.x. For the 1.x branch, we'll continue using DBTNG syntax. I still think it should be on the chopping block, especially after removing support for both databases other than MySQL and entity storage other than SQL. The entire system is now over-abstracted for the target systems we're trying to support.

[jenlampton]

Haven't met any D7 developers who dislike dbtng

Really? I have. I've also met a ton of developers of other systems who know MySQL, and get really frustrated by Drupal's DBTNG.

I myself often spend hours trying to read documentation on how to get DBTNG to do something complex, then I give up, and write my own MySQL query in minutes.

[fgm]

In that perspective, would you go to plain PHP mysqlnd API ? PDO ? DBAL ? something else ?

Given the history of the PHP projects with low-level APIs, the mysqlnd option seems a bit risky. DBAL would come with a nicer than current API, especially regarding schema migrations.

[jlfranklin]

I appreciate DBTNG for what it's trying to accomplish. The three major benefits I see are:

• Translation of intent to SQL that works for the underlying database engine. For example, SQLite doesn't have TRUNCATE. You have to use DELETE FROM without a WHERE clause. DBTNG doesn't handle date conversions, like FROM_UNIXTIME(), but it could. (#2890)
• Helps prevent lazy devs from writing dangerous queries such as db_query("UPDATE {foo} SET bar=$sql_injection_vulnerable_variable").
• Makes adding conditional parts of a query easier to code, like this:

    if (empty($foo)) {
      $query->condition('bar', $baz);
    }

I agree with @jenlampton, complex queries are hard to do, and can be really hard to read. I often put the intended SQL in a comment before the block of code so others coming after me (including future me) have a fighting chance of understanding what it's trying to do.

For all its warts, I don't know that I could do a better job designing a database abstraction layer, and I have tried. It would be easier to convert Backdrop's database layer to an ORM (think: RoR's ActiveRecord) instead of a safe SQL construction kit, but that would have huge ripple effects across all of core and contrib.

I'm not in favor of just dumping it and returning to db_query() all the time. Without something similar, the number of SQL-injection security updates will spike. (Oh, and it's a critical part of Silkscreen.)

If someone has some ideas on how to improve DBTNG, I'm all ears.

[fgm]

Beyond its actual internal design, the main issue I have with DBTNG as it stands is that it could easily be a normal PHP package usable via Composer, but a few implementation details prevent it from being one. Then again, Composer deployments are possibly more of a thing for Drupal than Backdrop, considering the respective targets.

[klonos]

Just wanted to chime in and say that as a novice dev, I personally find DBTNG easier to read/understand and then also tinker with, rather than using "direct" MySQL queries via db_query(). In my head, "direct" MySQL queries add another thing that Drupal/Backdrop newbies will need to learn (SQL) in order to contribute, so basically adds to the learning curve.

My 2c.

[jenlampton]

I often put the intended SQL in a comment before the block of code so others coming after me (including future me) have a fighting chance of understanding what it's trying to do.

I always do this in my custom modules! :)

The problem with DBTNG for new people is they they can see and manipulate simple queries, but they can see and manipulate simple queries in MySQL, too. Complex queries is where they get stuck because of lack of (or inadequate) documentation -- and that trips up the pros too. At least with MySQL we have all the same resporses for help as the rest of the world, so Google and stack overflow can come to our rescue. With DBTNG we're stuck relying on the 1 or 2 people who really understand it to write good docs.

I'm also skeptical that DBTNG is any more secure than db-query() for preventing SQL injection. The only real protection it offers is that it looks unfamiliar to people, so they will be more likely to read the docs, or copy an existing implementation. This makes the difficulty into a feature, and that just feels like we're doing it wrong. There were some huge security holes in DBTNG queries in core -- and there probably still are. If the experts can't figure out how to use it correctly, I have little hope that everyone else will.

I'm curious to see what replacing the DBTNG query builder with the Views query builder looks like. That's the direction I'd like to go for Deprecating DBTNG, and I'm hoping that will make it possible to provide backwards compatibility for a full major release cycle.

On Sun, Apr 7, 2019, 9:43 AM Gregory Netsas notifications@github.com wrote:

Just wanted to chime in and say that as a novice dev, I personally find DBTNG easier to read/understand and then also tinker with, rather than using "direct" MySQL queries via db_query(). In my head, "direct" MySQL queries add another thing that Drupal/Backdrop newbies will need to learn (SQL) in order to contribute, so basically adds to the learning curve.

My 2c.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/61#issuecomment-480608571, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR_osE8X4Cs_cEHARAgfKOnPwzFWtks5veiAmgaJpZM4A_kp_ .

[klonos]

... Complex queries is where they get stuck because of lack of (or inadequate) documentation -- and that trips up the pros too.

That's true.

At least with MySQL we have all the same resporses for help as the rest of the world, so Google and stack overflow can come to our rescue.

...also true.

[klonos]

If there is consensus about deprecating DBTNG (with whatever/something), shouldn't we be discouraging its usage by adding watchdog warnings in functions like db_select() / db_update() / db_delete() etc.? I mean, the less people are using them, the less work will need to be done when we actually deprecate them.

[jenlampton]

Maybe? I think we shouldn't start with the notices until we're sure a out this - which means we need to prove bc is possible. As it is now the removal wohldnt happen till at least 2.x, more likely 3.x, so we have plenty of time.

On Sun, Apr 7, 2019, 11:47 AM Gregory Netsas notifications@github.com wrote:

If there is consensus about deprecating DBTNG (with whatever/something), shouldn't we be discouraging its usage by adding watchdog warnings in functions like db_select() / db_update() / db_delete() etc.? I mean, the less people are using them, the less work will need to be done when we actually deprecate them.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/61#issuecomment-480618567, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR4z_ss4QkEVDmkEoEfzGveDEP24nks5vej1ZgaJpZM4A_kp_ .

[twmfc]

I'm a bit player but in D7 I use code like:

$result = db_select('node', 'n')->fields('n', array('nid, title'))->$range(0, 10)->addTag('node_access');

a fair bit and I'd be disappointed to lose this in BD

[herbdool]

shouldn't we be discouraging its usage by adding watchdog warnings in functions like db_select() / db_update() / db_delete() etc.?

I don't think there was consensus yet. Even then this would result in spamming the watchdog since the queries are used all over core right now. First, they should be eliminated from core. Second, it seems this needs to be done first as well:

The initial effort should be to provide a database system that can update usage of db_query() throughout core with a lightweight replacement directly against PDO.

[jlfranklin]

First, they should be eliminated from core.

I think we need a plan, first. The plan has to handle the features the DBTNG brought us. Check out some of Larry Garfield's blog posts he had during development of DBTNG, in particular Drupal 7 database plans and Drupal Databases: The future is now to get an idea of what they were thinking when they built DBTNG.

Whatever plan we come up with has to be able to support hook_query_alter(), used in a number of node access modules and some other places. (Domain Access, IIRC, uses it.)

Second, it seems this needs to be done first as well:

The initial effort should be to provide a database system that can update usage of db_query() throughout core with a lightweight replacement directly against PDO.

DBTNG is already PDO based. The DatabaseConnection class extends class PDO. All of the engine-specific variants are descended from class DatabaseConnection. Does this mean we can start from the DBTNG code and improve upon it, rather than dump it in the bin and start over?

I don't think db_query() all the time is the right answer. That requires moderately complex queries to be built with string concatenation, which is just as ugly and far more prone to SQL-injection bugs.

Putting the SQL construction in a single place -- down in the database layer -- made auditing for SQL errors far easier. As long as the database layer is clean, anything that uses the database API is safe, too. Auditing is reduced to verifying use of the API and checking a handful of calls that pass fragments of SQL down. (e.g., SelectQuery::addExpression())

[jenlampton]

Again: we currently have three query builders in core. Replacing DBTNG with something else won't solve that. We need to switch to using Views for anything that needs access control, and see what other use-cases we have that need to be addressed.

We don't need to replace all the features that DBTNG brought us, we only need to solve the use-cases that exist for Backdrop. Database abstraction, for example, is no longer necessary.

On Tue, Apr 9, 2019, 7:48 AM John Franklin notifications@github.com wrote:

First, they should be eliminated from core.

I think we need a plan, first. The plan has to handle the features the DBTNG brought us. Check out some of Larry Garfield's blog posts https://www.garfieldtech.com/blog-topics/sql he had during development of DBTNG, in particular Drupal 7 database plans https://www.garfieldtech.com/blog/drupal-7-database-plans and Drupal Databases: The future is now https://www.garfieldtech.com/blog/database-tng-lands to get an idea of what they were thinking when they built DBTNG.

Whatever plan we come up with has to be able to support hook_query_alter(), used in a number of node access modules and some other places. (Domain Access, IIRC, uses it.)

Second, it seems this needs to be done first as well:

The initial effort should be to provide a database system that can update usage of db_query() throughout core with a lightweight replacement directly against PDO.

DBTNG is already PDO based. The DatabaseConnection class extends class PDO. All of the engine-specific variants are descended from class DatabaseConnection. Does this mean we can start from the DBTNG code and improve upon it, rather than dump it in the bin and start over?

I don't think db_query() all the time is the right answer. That requires moderately complex queries to be built with string concatenation, which is just as ugly and far more prone to SQL-injection bugs.

Putting the SQL construction in a single place -- down in the database layer -- made auditing for SQL errors far easier. As long as the database layer is clean, anything that uses the database API is safe, too. Auditing is reduced to verifying use of the API and checking a handful of calls that pass fragments of SQL down. (e.g., SelectQuery::addExpression())

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/61#issuecomment-481284174, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR7v-F5XFC4lU6L-l2PgPBr22ilpEks5vfKhAgaJpZM4A_kp_ .

[herbdool]

I'm personally not invested in removing or keeping DBTNG. Though I'm willing to put money on it not being removed any time soon. Sounds like there would be a lot of work and I don't imagine it's a high priority for anyone developing for Backdrop.

I read this thread with interest, including the data on how much impact it has on query time. But I didn't see any data on how it impacts page load time from the end user's perspective. Can we put a number to it roughly? Is 10%, 50%? I'm betting it's at the low end - how does that impact the priority of this? Are there other things we can do in core that are easier and have an equal or bigger impact on performance?

[jenlampton]

My interest in this issue is primarily for learnability and simplicity and consistency, not speed, since DBTNG was one of the hardest things for new Drupal 7 adopters to learn. For me, the performance gain was a nice side affect that also lines up with our principles.

However, I think until 2020-2021 our focus should be on improvement in compatibility with Drupal 7, and this issue should be postponed until at least then.

When we do get to this, we should aim for backwards compatibility for at least one release cycle, and if we can get there, it may be posible to shift DBTNG to contrib!

On Tue, Apr 9, 2019, 11:29 AM Herb notifications@github.com wrote:

I'm personally not invested in removing or keeping DBTNG. Though I'm willing to put money on it not being removed any time soon. Sounds like there would be a lot of work and I don't imagine it's a high priority for anyone developing for Backdrop.

I read this thread with interest, including the data on how much impact it has on query time. But I didn't see any data on how it impacts page load time from the end user's perspective. Can we put a number to it roughly? Is 10%, 50%? I'm betting it's at the low end - how does that impact the priority of this? Are there other things we can do in core that are easier and have an equal or bigger impact on performance?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/61#issuecomment-481375477, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSR5qhIGywI-VIdNXjdEEKb-DFPWkBks5vfNv3gaJpZM4A_kp_ .

[klonos]

However, I think until 2020-2021 our focus should be on improvement in compatibility with Drupal 7, and this issue should be postponed until at least then.

👍 ...our focus till end of 2021 should be to smoothen the migration/upgrade process of D6/D7 sites to Backdrop.

[twmfc]

We need to switch to using Views for anything that needs access control, and see what other use-cases we have that need to be addressed.

Perhaps I'm missing the point here, but my understanding is that Views are just a further abstraction away from the raw sql used to access the database (MySQL only in BD but a range of DB's in Drupal and Silkscreen).

Using views will traverse a path that looks like this:

Views--->DBTNG--->sql--->db

Specifically, as I understand it, Views will utilise hook_query_alter() to add tags, via DBTNG, to build up the final sql query.

Therefore, I do not see Views as a replacement for DBNTG; rather I see Views as a query builder that makes use of DBTNG along with providing a GUI.

Pleased to see that the consensus appears to be to defer this until further down the line as I'm sure this will better reward all of the hard work the Team have done with BD thus far!

[jlfranklin]

Using views will traverse a path that looks like this:

Views--->DBTNG--->sql--->db

That is accurate. There are a couple things that Views does with dates that could be pushed down into the database layer. (#2890) Beyond that, Views uses DBTNG to build the queries.

[jenlampton]

Views are just a further abstraction away from the raw sql used to access the database

yes, exactly!

Views does not need to use DBTNG in order to achieve both access control and alterable queries, as it existed before DBTNG did. IF we can switch everyone over to using Views instead of DBTNG directly, we would have abstracted away the DBTNG, making it easier to remove that part.

That would make our types of queries look like: 1) db_query--->sql--->db 2) Views--->sql--->db

...rather than: 1) db_qurey--->sql--->db 1) DBTNG--->sql--->db 1) Views--->DBTNG--->sql--->db

The views query builder would certainly need changes - but I suspect we may be able to do that in a way that would have the least impact on contrib. Since views exist now, maintainers could begin switching things over to views in the future at any time. If that were our path away from DBTNG, then both could safely exist for a long while, and changes to Views (to remove DBTNG) may not need any further changes to contrib.

Caveat: This of course, is all theoretical until we try to write some code :)