Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trivial cross-database query updates for 1.10 #3133

Closed
jlfranklin opened this issue May 19, 2018 · 8 comments
Closed

Trivial cross-database query updates for 1.10 #3133

jlfranklin opened this issue May 19, 2018 · 8 comments

Comments

@jlfranklin
Copy link
Member

To get Silkscreen 1.10 to pass all the tests with the PostgreSQL driver and most of the tests with SQLite, I had to adjust some of the database queries. The PR below contains three changes:

  • Convert two db_query('TRUNCATE') calls to use db_truncate() instead.
  • Convert a SELECT to avoid UNIX_TIMESTAMP(), a MySQL-specific function.
  • Call db_merge() multiple times, instead of once with an array passed to key()

As is policy with Silkscreen, if there are patches that can be applied upstream, they're submitted to the upstream project, especially when they have little to no performance or functional impact.

@docwilmot
Copy link
Contributor

Looks simple enough.

@jenlampton
Copy link
Member

jenlampton commented May 21, 2018

Backdrop only supports MySQL and MySQL equivalents like MariaDB for the database

Making any changes now that would make it appear that Backdrop could work on any database other than MySQL would be misleading. People might think it was okay to run Backdrop on one of these other Databases, when that would be setting them up for hardship in the long run.

Many of the database queries in core are going to be further optimized for MySQL performance improvements in 1.x, and the database abstraction layer is going to be removed in 2.x.

We're also planning on making other Database changes like switching from using timestamps to MYSQL Date formats to better leverage the built-in support for date handling.

Reminder: PostgreSQL and SQLite represent less than 1% of Drupal sites (and 0% of Backdrop sites) so it is not a priority for us to support this extreme edge-case.

@jlfranklin
Copy link
Member Author

Yes, I know Backdrop only supports MySQL. Patches like this are to reduce drift between Backdrop and Silkscreen, and not to specifically support multiple databases.

@jenlampton
Copy link
Member

jenlampton commented May 21, 2018

Okay, I'm tagging needs feedback because I'm not sure about the specific ramifications of these changes. (adding back RTBC tag then, too)

@vstemen
Copy link

vstemen commented May 21, 2018

jenlampton wrote in the chat channel,

Anyone want to weigh in on adding alternate database support to Backdrop?

Hi. Just to chime in and put my two cents worth in about supporting multiple databases.

I would like to see backdrop eventually have an abstraction layer to support
multiple databases, including non-sql based ones, for several reasons. To name
a couple,

  1. If stability, security, or political issues crop up in MySQL that are not
    being resolved, you are not tied to it.

  2. I have never been a fan of SQL based databases for a variety of reasons, of
    which would probably merit a whole other topic of discussion. Because of this,
    and the fact that I have not been really fully satisfied with any of the
    existing DB systems out there, I began development of my own non-sql database
    system sometime back. When I eventually get the time to finish it enough to
    release and put into production use, I would like to have the option to use it
    for Backdrop.

I realize that you are trying to maximize performance by using MySQL specific
features. However, it is my personal opinion that it is not worth the trade
off of flexibility. First of all, it is my belief, from my own experience and
testing over the years, that most of the performance issues are due to
fundamental design flaws in MySQL and SQL in general. It has always made us
a bit nervous setting up critical web sites for our business that rely on
MySQL. Unfortunately, so far, we have not really had any other suitable option
if we want to have CMS features without hand coding everything.

If you do use MySQL specific features, it should be hidden in the MySQL portion
of the code, under the abstraction layer, and not part of the API for accessing
the database.

@jenlampton
Copy link
Member

Thanks @vstemen, I'd misunderstood this issue. It's not actually about making Backdrop support other databases, but rather making it so there are less differences between Backdrop and Silkscreen. Silkscreen is a Backdrop fork that does support other databases.

@jlfranklin
Copy link
Member Author

To reinforce what @jenlampton just posted, the PR for this patch are not intended to provide support for other databases, but only to prevent drift between Backdrop and Silkscreen, a fork that tracks Backdrop in the style of Pressflow (for those of you who remember Drupal 6 and Pressflow.)

Development of Silkscreen has revealed a number of minor issues in tests and code that happen to work with MySQL, but don't for other databases. Where I have patches that can apply to Backdrop core without being disruptive, I file them here. @quicksketch has been gracious in reviewing them, even though the patches don't "fix" anything in Backdrop, such as #2889 (Tweak some tests to make them more robust.)

I also file the more disruptive patches, even though I have little expectation they'll be applied, such as #2890 (Refactor date SQL generation.) I think it is good open source etiquette to provide the patches and let the parent project maintainers make the call.

And then there are the real weird issues I sometimes run into, where I just don't have an answer. (#2864, #3131)

@quicksketch
Copy link
Member

I've merged in backdrop/backdrop#2197 into 1.x and 1.10.x.

For the record, my attitude towards these database-specific PRs has been to merge them as long as they do not have an impact on compatibility and they don't require complicated changes. Additionally @jlfranklin has taken on the burden of actually supporting non-MySQL databases, meaning support requests and compatibility issues would be handled by Silkscreen rather than Backdrop. As @jlfranklin then filters/fixes the issues, we don't have much downside or effort to make the changes requested by Silkscreen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants