I have an updated version of the db layer for sqlite 2 (no PDO stuff). The only thing is tring to get logins to work without having to hack core. I have $user = user_load(array('uid' => 2)) in index.php to fix it. Some selects are al;so not working, so it needs more work before it is production ready. I have it on a local test site, but its not too useable at the moment. This is the database inc and the creation script working for 4.7. Most of the code is from chx's sandbox, though blobs are done MySQL's way and table locking doesnt do anything. Some rendering likewatchdog and taxonomy isnt done right and user login is impossible (name is a reserver word).
Comment | File | Size | Author |
---|---|---|---|
#162 | 67349-sqlite-fix.patch | 773 bytes | Damien Tournoud |
#157 | sqlite_final.patch | 45.5 KB | chx |
#156 | sqlite_N+1.patch | 45.55 KB | chx |
#155 | sqlite_N.patch | 45.53 KB | chx |
#153 | 67349-support-sqlite-databases.patch | 46.34 KB | Damien Tournoud |
Comments
Comment #1
Anonymous (not verified) CreditAttribution: Anonymous commentedinitial database file...
Comment #2
Tobias Maier CreditAttribution: Tobias Maier commentedComment #3
chx CreditAttribution: chx commentedThere was someone (disappeared) who made another version which did not change the SQLs but the results.
But I do not think this worth pursuit. SQLite 2 is not good enough for Drupal. The write lock problems will kill your site. SQLite3 you need.
Comment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedIm guessing the "AS" stuff IS needed to get the SELECT * queries working... I'll see if it can withstand apache bench on a test setup, but I guess it'll have locking/ concurrency issues. I guess this'll involve hacking core stuff(!) or will not work for some obscure reason.
Comment #5
chx CreditAttribution: chx commentedthe big prob with sqlite2 is that when you have
select n.nid from node n inner join term_node tn on n.nid=tn.nid
then upon fetch the index will ben.nid
so we need to addas nid
or upon fetch rename the the index.Comment #6
Anonymous (not verified) CreditAttribution: Anonymous commentedThis fixes the problem with creating aggregator feeds.
Comment #7
Anonymous (not verified) CreditAttribution: Anonymous commentedThis is my second attempt at this which has a few majod differences. The * hack is gone and _sqlite_expand works in this version. DISTINCT also works now too. There is a new UDF to support GREATEST too. This should be bug free, at least for core drupal. It should do what it is meant to do, though there is probably still a few bugs in it.
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedoops, and the file...
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentednew database include file
Comment #10
chx CreditAttribution: chx commentedI scream for benchmarks when I see SQL called from preg_replace_callback
Comment #11
Steven CreditAttribution: Steven commentedDrupal offers no such guarantee, a user is free to submit any bytestream they want and pass it off as UTF-8.
Comment #12
paranojik CreditAttribution: paranojik commentedComment #13
paranojik CreditAttribution: paranojik commentedfixed a typo in users table creation query (database.sqlite)
Comment #14
paranojik CreditAttribution: paranojik commentedfixed _sqlite_expand() (solves the above mentioned problems with user logins)
Comment #15
paranojik CreditAttribution: paranojik commentedsome work on coding style and doxygen comments
Comment #16
Dries CreditAttribution: Dries commentedLooks like we might be ready for another review ...
Comment #17
Anonymous (not verified) CreditAttribution: Anonymous commentedThe newer one looks better. Ive run some tests and its reasonably responsive, though it needs something like APC to get a decent speed.
ab -n 100 -C PHPSESSID=A9zO4ZhLJyNBA7VY0bmFwEi7vF2 -c 5 http://sumumo/drupalite/
The result from that takes about 9 seconds on a site with some content with it.
There seems to be a cache problem if cache is set to on. The cron.php file also has
SELECT GREATEST(c.last_comment_timestamp AS last_comment_timestamp, n.changed) as last_change, n.nid AS nid FROM node n LEFT JOIN node_comment_statistics c ON n.nid = c.nid WHERE n.status = 1 AND ((GREATEST(n.changed, c.last_comment_timestamp) = 0 AND n.nid > 0) OR (n.changed > 0 OR c.last_comment_timestamp > 0)) ORDER BY GREATEST(n.changed, c.last_comment_timestamp) ASC, n.nid ASC LIMIT 100 OFFSET 0
which fails, which could be something to do with an "AS". Sqlite should be able to do temporary tables though SQL and it should be possible to implement this.
Comment #18
chx CreditAttribution: chx commentedthat bench, in itself is not really useful -- care to tell us how it compares when using a mysql server on the same machine?
Comment #19
chx CreditAttribution: chx commentedAlso, someone needs to stand up and say 'I will maintain SQLite'. This will include continous testing of Drupal w/ SQLite, porting the updates... quite a task. Especially that SQLite does not support ALTER TABLE though I am very well aare of the fact that this has been coded around.
Comment #20
Dries CreditAttribution: Dries commentedtachyonxv: you have to compare your benchmark results against a baseline scenario (a MySQL setup with the same amount of content / configuraiton).
Comment #21
paranojik CreditAttribution: paranojik commentedQueries into a temporary table work already. I'm working on the ALTER TABLE command, but I'm affraid of the impact of all the processing a query has to go through before it actually gets to sqlite_query(). Does anybody have some scripts to setup a testing site (for benchmaking)?
Comment #22
Steven CreditAttribution: Steven commentedIIRC, db_affected_rows() returns the number of /matched/ rows in an UPDATE query, not the number of changed rows. That is, if you set a row to the same value it has now, it still counts.
sqlite_changes() does not seem to have that behaviour. It is necessary to avoid extra counting queries in cache and search.
Comment #23
chx CreditAttribution: chx commenteddevel project has generate scripts
Comment #24
paranojik CreditAttribution: paranojik commentedSteven: I am not able to find such a description of mysql_affected_rows(). Please take a look at http://dev.mysql.com/doc/refman/5.0/en/mysql-affected-rows.html and http://si2.php.net/mysql_affected_rows.
Comment #25
paranojik CreditAttribution: paranojik commentedI've setup 2 equal sites on mysql an sqlite using the devel module scripts (thanks chx). Results are attached. Although the results don't show that, the site setup took much much longer with sqlite... Looking forward for your interpretation.
Comment #26
chx CreditAttribution: chx commentedSee a) Drupal core, db_connect, the comment on CLIENT_FOUND_ROWS b) http://dev.mysql.com/doc/refman/4.1/en/mysql-real-connect.html on more of this constant.
Comment #27
chx CreditAttribution: chx commentedDocument Length: 14826 bytes 17444 bytes
Care to explain the the difference? I thought you are benching the same Drupal w/ separate databases. Setting this up can be non-trivial, I admit... but maybe you could create a dump of the data from one DB and load it back (if that works) to the other.
However, this looks promising. When I have written the sqlite layer I thought that we could ship Drupal w/ the database in place as SQLite files are just that -- plain files, so Drupal is ready-to-run on PHP5.
Comment #28
paranojik CreditAttribution: paranojik commentedI was also wondering where that difference came from. Apart from the intial database setup, I took the exactly same steps to build the test site. I'll try again the way you said.
Comment #29
paranojik CreditAttribution: paranojik commentedI've setup drupal the way chx suggested, but there's still the same difference. Drupal on mysql does not output the same front page as drupal on sqlite. I double checked: all nodes have the same timestamp, all are promoted, and none is sticky. As I see it, the front page should look like this: node-50, node-49, node-48,.... and that is what drupal on sqlite outputs. On the other hand the front page of Drupal on mysql looks like this: node-50, node-36, node-35. node-34, node-33,.... Any idea, anyone?
Comment #30
paranojik CreditAttribution: paranojik commentedI've put some effort into modifying/adopting jenseng's ALTER TABLE support for sqlite. The code is a mess and I still have to check if it can actually run a Drupal database update, but it's a start...
Comment #31
paranojik CreditAttribution: paranojik commentedThe alter table function is almost entirely rewriten. It now supports a syntax which is very close to mysql's. It supports dropping and adding of primary keys, dropping, changeing and adding columns.
The install procedure works, but the configurtaion screen (as it looks now) is confusing for setting up sqlite, because you don't need a password or a host, only a path and filename. The attached screenshot shows how to fill in the form to install drupal with sqlite.
Comment #32
paranojik CreditAttribution: paranojik commentedthe configuration form
Comment #33
chx CreditAttribution: chx commentedxmlrpc()
on how this is done.Comment #34
paranojik CreditAttribution: paranojik commented@#5 If I understand correctly that's not a bug. Sqlite docs: PRAGMA command syntax under PRAGMA full_column_names;.
Comment #35
chx CreditAttribution: chx commentedOh, that's awesome, I never realized it -- but it was filed as a bug and this PRAGMA was never added to it.
Alas, you went far with coding with clarifying the biggest question -- are you willing to maintain this port on the long run?
Comment #36
paranojik CreditAttribution: paranojik commentedI agree it would be very nice to ship drupal with a prebuilt database, but then even more problems arise:
I am willing to maintain this port. I've been running drupal 4.5 with sqlite for almost two years. When 4.7 was released I decided to upgrade it and thanks to tachyonxv I started looking more into it (it's good to see more people are interested).
@#19 I've already ported all updates (from 110 on) and also performed the update of the 4.5 site. If anybody needs them, please email me.
Patch implements suggested demand loading of altertable function.
Comment #37
Anonymous (not verified) CreditAttribution: Anonymous commentedIt seems to work fine on Drupal 5.0. The forum seems to still spit a few errors due to count and distinct with several colums. The short name support should make it work better. The forum works correctly now too. Im not sure about performance, but it may be faster than the previous version. It seems to return error responces if it has something like 20 connections at the same time, though you probably wouldn't be using sqlite for something that busy.
Comment #38
forngren CreditAttribution: forngren commentedWell, we don't have fill it with tables, if we just prebuild the database connection, installation will be alot easier.
Comment #39
chx CreditAttribution: chx commentedThat's awesome, man! I do not remember whether I mentioned my crazy idea about storing some stuff which change relatively infrequently in SQLite? Like, system table is the ideal candidate.
Comment #40
chx CreditAttribution: chx commentedComment #41
paranojik CreditAttribution: paranojik commentedSo... The pregenerated database would only containt the tables for modules in the "default" profile and we change drupal_install_profile() so that the unneeded tables are dropped (call hook_uninstall for those modules?)
Would that be the issue that changes the database schema? (Would I as a maintaner of this port be notified about this kind of changes?)
I know it can, but it's quite expensive. On the other hand, there won't be that many revisions of the database....
Sounds like a different debate... isn't it?
Comment #42
necromancer-1 CreditAttribution: necromancer-1 commentedso are we ready to fly? :)
Comment #43
forngren CreditAttribution: forngren commentedIt's probably to late to get this into Drupal 5, but why can't this be the first patch in 6? ;)
Great work paranojik!
Comment #44
coreb CreditAttribution: coreb commentedmoving from x.y.z to 6.x-dev version queue
Comment #45
viperjason CreditAttribution: viperjason commentedSo, then the question is, can this be used with 5.0 or do I need to use 4.7?
Comment #46
viperjason CreditAttribution: viperjason commentedI'm a new user, so my next question may seem a bit dumb, but how do I instal drupal after downloading the .inc file for sqlite. I still get the error that mysql is not supported.
Comment #47
Steven CreditAttribution: Steven commentedPlease don't change the properties of issues for no particular reason.
Comment #48
moshe weitzman CreditAttribution: moshe weitzman commentedsubscribing
Comment #49
viperjason CreditAttribution: viperjason commentedSorry about changing the version. Didnt think about it changing it for the project. I just thought it was in reguard to my particular post. Anyways, I found a settings.php and changed that for sqlite. I also found the sqlite.patch file here and loaded that and patched the right files in 5.0, but still no luck. I still get errors in trying to find some tables. It seems that the installation looks for the tables and if it doesnt find them it just errors out. Can we have a patch so that the tables will automatically get generated if they dont exist? The easiest way to tell if they dont exist is checking the file size of the DB. If its 0 then obviously no tables have been created in SQLite.
Comment #50
paranojik CreditAttribution: paranojik commentedLook at attachment of #32 for how to install. It's akward. CHX already gave an idea how to make this right. I'll implement it in the end of next week, I hope...
Comment #51
viperjason CreditAttribution: viperjason commentedI looked at that, but I think the issue is that if we dont have mySQL then you dont get that configuration page. I just get a bunch of SQL errors. I can happily wait until next week for something different.
Comment #52
paranojik CreditAttribution: paranojik commentedreapplied
Comment #53
paranojik CreditAttribution: paranojik commentedRerolled.
To install all you have to do is select 'sqlite' and hit 'Save settings'.
Comment #54
MaikR CreditAttribution: MaikR commentedOkay, I will try to install this patch and run Drupal 5.1 on a NAS (QNAP TS-201). This NAS has SQLite version 2.8.14 and PHP version 4.4.2 preinstalled.
As with a lot of those NAS's it is not possible to update PHP or install MySQL. But those boxes are very nice to run a small Drupal installation.
So I hope I will get a clean install up and running, so I can make a "TS-201 Drupal install package".
Comment #55
MaikR CreditAttribution: MaikR commentedIt seems to run nicely. Two problems as far as I can see.
1) I can't change the permissions for the settings file to read only. The internal server simply rejects my changes. Is there some other way I should try?
2) I can't prevent the db from being downloaded. I've tried changing the .htaccess file in the site root, but that didn't solve the problem. Is it possible that the internal server ignores .htaccess files?
Comment #56
moshe weitzman CreditAttribution: moshe weitzman commentedI applied this to very recent DRUPAL-5 and am real pleased with the result.
A few minor gliches I saw:
- 1 hunk failed for me in system.install where we define minimum DB version for sqlite. might be a non issue
- on the post-install confirmation page, i got 1 warning about a failed query into menu table
- i am seeing on warning on 'admin/logs/status/sql'. it is "warning: sqlite_query() [function.sqlite-query]: near "SHOW": syntax error in /Library/WebServer/htdocs/dr5/includes/database.sqlite.inc on line 250.". if you are in a hurry, just unhyperlink the link that points there on the status report. if you have some time, add some cool DB stats on that page.
Comment #57
paranojik CreditAttribution: paranojik commentedRerolled. No status report for now. As far as I know, SQLite does not track statistics like MySQL does. I'll think of something to show on that page (the database size, maybe...).
Comment #58
forngren CreditAttribution: forngren commentedAs maikr pointed out in #55 there's a huge security issue in the patch. How can we prevent the db from being downloaded? On Apache servers it's possible to fix with .htaccess, but what about other servers?
If we ship Drupal with a premade db, we will probably lose the good security reputation that we currently have. One way could be to give the db a random name and store it in settings.php. The best would be to keep the db outside of the webroot, but that's not possible on all hosts. Thoughts?
Comment #59
moshe weitzman CreditAttribution: moshe weitzman commentedminor - install.sqlite.inc has error messages that refer to MySQL.
Comment #60
paranojik CreditAttribution: paranojik commented@58: You can give your database file any (valid) name you want.
Comment #61
chx CreditAttribution: chx commentedAs far as I know the web server process needs to be able to write the database file anyways, so we could copy the 'skeleton' shipped to a random name and store that in settings.php. And/or AJAX callback could easily check whether the database is downloadable. Yes, this is something to think on but definitely solvable.
Comment #62
paranojik CreditAttribution: paranojik commented@59: Thanks. Fixed, although they may still need corrections.
Comment #63
forngren CreditAttribution: forngren commentedI have some ideas for howto secure sqlite:
Look at http://www.sqlite.org/cvstrac/wiki?p=SqliteWebSecurity
Suggested flow on howto install Drupal with sqlite:
0) We patch .htaccess to deny any access to any file following the schema below. It's possible to use regexp, right?
1) User visits install and selects sqlite
2) User inputs are completely ignored. There must be nicer way to do this for javascript-enabled users.
3) A database is created, named after the following the schema: sites/[current-site]/database-[random[a-zA-Z0-9]*6-8i.e. a 6 to 8 characters long string containing symbols from the range a-zA-Z0-9].php
4) CREATE TABLE "<?php exit; parse_error_in_worst_case"(donotremovethistable boolean);
5) The rest of drupal setup
Just my 0.02€. I apologize for not posting any code.
Comment #64
MaikR CreditAttribution: MaikR commentedWhen using that code to generate a unique name, wouldn't it be relatively easy to write a script to get the database by probing it's name?
Yes, I understand it's much safer the a default name. But is it secure enough?
Comment #65
forngren CreditAttribution: forngren commenteda-z = 26 chars
A-Z = 26 chars
0-9 = 10 chars
62 chars in total.
(62^6)+(62^7)+(62^8) = 221 918 520 426 688 combinations
I know that that ain't a buttload of combinations, but I you add the <?php hack, transfer times over internet, .htaccess and the possibility to put the database outside of your webroot (new feature req.) I think it's sufficient. There will however be trouble on hosts where files are listed by default, even with the <?php hack. We must STRONGLY encourage users to secure their db.
Does that make sense?
Comment #66
chx CreditAttribution: chx commentedCREATE TABLE "<?php exit; parse_error_in_worst_case"(donotremovethistable boolean);
let it be. I always forget this nice trick. Very, very cool.Comment #67
paranojik CreditAttribution: paranojik commentedImplemented as suggested by forngren.
Comment #68
forngren CreditAttribution: forngren commentedI don't want to be a pain in the ass and I really do appreciate the work you do, but it's easier to find the weak spots than the solid ones. What about throwing on some code in .htaccess to block all attempts to access to the databasefile-pattern, not only the physical one. That will make it harder for crackers since they will receive a 403 error independent if they hit the right url or not.
Once again, thanks for the efforts.
Comment #69
chx CreditAttribution: chx commentedforngren, we are not using .htaccess to stop downloads, that'd be Apache specific. Much rather, we give the database a php extension and then throw in a <?php very near in the beginning (as a table name) to fool poor PHP parser to think it's a PHP script, and then immediately exit. Now, either you will get a particularly nice and rather informative SQLite database file header with no usable information whatsoever or a parse error.
Comment #70
paranojik CreditAttribution: paranojik commentedKeeping up with HEAD.
Comment #71
paranojik CreditAttribution: paranojik commentedChain tested the patch and noticed that it relies on webroot being writeable. Also, sqlite error messages were printed as error numbers (missing call to sqlite_error_string).
Comment #72
paranojik CreditAttribution: paranojik commentedThis applies to Drupal 5.1.
Comment #73
ebruts CreditAttribution: ebruts commentedHave not try it yet but found these as i browsed through the patch.
Comment #74
pukku CreditAttribution: pukku commentedHi! This is referring to the patch posted in comment #72, for Drupal 5.1.
Running Apache 1.3, PHP 5.2 (Apache from Mac OS X 10.4 default install, PHP5 self-compiled and installed).
I downloaded a fresh version of Drupal 5.1 and applied the patch using
patch -p0 < /path/to/sqlite_for_5_1.patch.txt
. I then go dohttp://localhost/~pukku/Sites/drupal
, and I get the following errors:The form just consists of the words "To set up your Drupal database, enter the following information.", but there are no fields to enter any configuration information.
In case it matters, I do not have any MySQL or PostgreSQL extensions for PHP installed (this is my home machine, and I wanted to see if I could just get by with SQLite).
Comment #75
moshe weitzman CreditAttribution: moshe weitzman commentedwould be lovely to refresh this patch for D6 so that sqlite is a core RDBMS or a contrib that requires no patching. the schema patch is about to go in which will automatically make almost all contrib modules available to a new DB engine like sqlite.
Comment #76
pukku CreditAttribution: pukku commentedThis is a followup to my previous comment (#74). The errors seem to have been a configuration issue with php.
It seems to be working now. However, I would make it more clear on the initial form that for SQLite you don't have anything to set...
Comment #77
pukku CreditAttribution: pukku commentedIs there a way to specify that it should use SQLite 3, not 2? I want to use devel.module, and its tables require the AUTOINCREMENT feature, which doesn't seem to be in SQLite 2.
Thanks,
Ricky
Comment #78
chx CreditAttribution: chx commentedSQLite3 requires PDO (RTFM, damnit!). I would much like to see this rerolled for Schema and included in D6.
Comment #79
Roberto Gerola CreditAttribution: Roberto Gerola commentedPatch for Drupal 5.1 with the option to select the path and the name of the database.
Default path, if not specified, is sites/all.
Specify the database path in the form, for example : sites/db/database.db
Comment #80
David Strauss"Username is a requred field for MySQL and Postgres databases."
Required is misspelled.
Comment #81
David Strauss* The table schemas need to be ported to the new schema API.
* Many comments need to be cleaned up. Drupal core comments should be sentences or near-sentences.
* "includes/sqlite.inc" seems to defy database abstraction file naming conventions.
* Your SQLite code will need to support the schema API for modifications and table creation.
You may want to wait until Drupal 7 to really push this. The database layer changes in Drupal 6 are the largest ever. You will face an uphill battle trying to keep up with schema API and other changes.
Comment #82
David StraussOn the other hand, the huge DB changes in Drupal 6 might be an excuse to overhaul it even more by including support for a new database engine. Either way, you have your work cut out for you. :-)
Another reason you might want to wait for Drupal 7 is discussion of supporting only PHP 5. It would certainly ease issues like using PDO. On the other, other hand, I certainly have no objection to including a PHP 5-only database engine in Drupal 6. MySQLi is already PHP 5-only.
Comment #83
sebastien.barre CreditAttribution: sebastien.barre commented> On the other, other hand, I certainly have no objection to including a PHP 5-only database engine in Drupal 6.
That's great.
So what's the current status? I would definitely be nice to see SQLite3 supported in Drupal...
Comment #84
David StraussEven Drupal 5 has a PHP 5-only database engine, mysqli.
Comment #85
hswong3i CreditAttribution: hswong3i commentedsubscribe :)
P.S. I will try to include the needs of SQLite into my latest D7 database abstraction layer exploration (http://drupal.org/node/172541). As Garally2 able to support for both MySQL, PgSQL, Oracle, DB2, MSSQL, plus SQLite (http://www.garfieldtech.com/blog/database-abstraction#comment-446), I guess my propose may also able to cover the needs of SQLite as I am just trying to clone their idea (http://edin.no-ip.com/html/?q=node/310).
I would like to join the SQLite backend development, too. Please free feel to contact me if anythings I could help about this issue :)
Comment #86
aclight CreditAttribution: aclight commentedsubscribing
Comment #87
paranojik CreditAttribution: paranojik commentedI finally resumed working on sqlite support. This patch provides support for the schema API and also tries to address some of the problems mentioned in previous comments. Drupal installs without problems. I also tried the 6.x-DEV version of CCK in order to test the "alter table" statement and did not encounter any problems.
This still needs a lot of work. Suggestions are welcome...
Comment #88
catchsubscribing.
Comment #89
gregglesFwiw, I believe that any work on this needs to be postponed for a little while to see what happens with Larry's PDO work ( http://www.garfieldtech.com/blog/database-abstraction ). Also, subscribe.
Comment #90
hswong3i CreditAttribution: hswong3i commentedI dig into this topic for 2 days together with my personal research project Siren, based on my proposed personal battle target for D7. The progress is quite positive: SQLite + Drupal is now able to functioning with pdo_sqlite, active most core optional modules, and pass a simple AB benchmarking test together with MySQL, PostgreSQL and Oracle. Here is the minor footnote for this progress.
BTW, I did some tricky handling, e.g. skip
sqlite
implementation but focus onpdo_sqlite
for SQLite v3.x support, skip ALL upgrade abstraction, and also hardcode the database file path. I try to reuse most of the existing founding on #87 and that of Siren's pdo_pgsql progress.This implementation is still far from perfect, but should able to be a technical preview about what we are still missing out for D7 core SQLite database supporting. Please feel free to comment about it ;-)
Comment #91
BioALIEN CreditAttribution: BioALIEN commentedThings have slowed down considerably on this, but I would welcome the prospect of having this in D7.
Comment #92
Crell CreditAttribution: Crell commented@BioALIEN: PDO-SQLite support is pending on the move to PDO: http://drupal.org/node/225450
After that, it should be fairly easy. (I know, famous last words.)
Comment #93
xaweryz CreditAttribution: xaweryz commentedsubscribing
Comment #94
Robin Monks CreditAttribution: Robin Monks commentedsubscribing
Comment #95
catch:)
Comment #96
chx CreditAttribution: chx commentedComment #97
lilou CreditAttribution: lilou commentedComment #98
Rainy Day CreditAttribution: Rainy Day commentedSubscribing.
Comment #99
chx CreditAttribution: chx commentedSo here we are. Some schema changing methods are not implemented yet. The query method in database.inc is not nice but with SQLite, after a schema change your prepared statements are no longer valid and you need to re-preapre. There is an API which automates this and PDO does not use it so I need to. There is quite some code duplication in there but I already mailed Larry hopefully he will love the piece a bit and get it up and kicking. Drupal at least installs with this patch no further testing had been made yet. And yes, I am willing to maintain the SQLite subsytem if necessary and unlike the previous such offer mine is quite good -- it'll be some time before I disappear :)
Comment #100
chx CreditAttribution: chx commentedDiscussed with Crell and now update_sql happily resets the preparequery static cache. Less code. Yay.
Comment #101
dmitrig01 CreditAttribution: dmitrig01 commented+ public function supportsTransactions() {
+ return TRUE;;
+ }
two sems,
the
+ do {
+ $temp_table = $table . '_' . $i++;
+ } while ($this->tableExists($temp_table));
near the bottom isn't necesarry
Comment #102
chx CreditAttribution: chx commentedWe are getting there. I have added a test for Schema API. Of course, a through unit testing for that would be enormous work but even this simple test revealed quite some bugs.
Comment #103
chx CreditAttribution: chx commentedThe test is not there to commit into core right now. I have not yet run in MySQL or pgsql -- not even ran in the browser, just command line. It's there to show that the black magic that is SQLite schema change API works. If it so happens that the test already passes in MySQL and pgSQL then life is good and we can commit the test. If not, we will open another issue for it, nothing is lost, this issue is about getting SQLite support in core. It's just a lot easier this way.to verify that schema works.
Comment #104
chx CreditAttribution: chx commentedThis sqlite driver has most tests passing, had no time yet to check whether all passes. It's highly recommended to add http://drupal.org/node/302396 to your install before you test unless you have a big bad machine. Edit: sqlite is part of php so the sqlite queries now count in the 30 seconds time out.
Edit2: testing instructions, provide a database name like sites/default/files/whatever so that the sqlite driver actually can write the table. Username and password are ignored.
Comment #105
Damien Tournoud CreditAttribution: Damien Tournoud commented@chx, woo great work.
I'm not sure that I understand all of it, but I confirm it indeed works :)
The only issue I ran on is that SQLite information_schema uses a slightly different name than MySQL and PostgreSQL (information_schema_tables instead of information_schema.tables), so Simpletest cleanup fails. Maybe we need to abstract a schema::getTablesLike() or something.
Comment #106
chx CreditAttribution: chx commentedDamien, you mean all tests pass? About informationSchema http://drupal.org/node/301038 is what you want. Expect a patch there not too far in the future :)
Comment #107
Damien Tournoud CreditAttribution: Damien Tournoud commentedFurther testing:
Don't seems to work (it returns 0).
I can't get parallel simpletests to work: I'm bumping into this error:
Oh, and some tests seems to hang a while when network operations are requested (like refreshing an RSS feed). But this maybe linked to my test environment (my laptop is not connected to the Internet here).
Comment #108
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is a full test log. It's the run of a slightly modified run-tests with debug stuff added.
Comment #109
chx CreditAttribution: chx commentedHere is a reroll. The update feed items test fail miserably when it tries to delete the feed. Strange. Slow. Very strange.
Comment #110
Damien Tournoud CreditAttribution: Damien Tournoud commentedSome more fun with SQlite... we now have only one database test failing! (complex queries)... ok actually two (log fails too, for some reason).
Comment #111
Damien Tournoud CreditAttribution: Damien Tournoud commentedOk, the current MergeQuery implementation is wrong. INSERT OR REPLACE is not a merge query, because it would kill the fields that are not defined in the query. Quoting Crell:
Comment #112
chx CreditAttribution: chx commentedDamien, possible. Please observe that i wanted that for the simplest case only which and I thought
if ($this->expressionFields || $this->updateFields || $this->excludeFields)
will be enough. Feekl free to exterminate this mergequery implementation and fall back to the naive implementation. We have transactions, work they will. I see that now we are rolling with supportTransactions FALSE however that was only a desperate attempt from me to get the thing working, feel free to make it TRUE.Comment #113
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdated patch, with MergeQuery_sqlite killed.
It looks like SQLite has an issue replacing placeholders in HAVING clauses, so I've overriden SelectQuery for now.
Comment #114
Damien Tournoud CreditAttribution: Damien Tournoud commentedchx found out that pdo_sqlite fails to bind correctly numeric values... which hurts only when they are used in expression (such as COUNT(name) > :count) [1].
In that updated patch, we substitute numeric values ourselves, before passing the query to PDO::prepare().
All database tests pass, except the same logging test. Several tests fail when notices are generated in the tested site, which can mean that watchdog() is failing somehow.
Total: 7010 passes, 111 fails, and 387 exceptions
Full results attached.
[1] http://bugs.php.net/bug.php?id=45259
Comment #115
chx CreditAttribution: chx commentedDamien, awesome work! XML-RPC uses this construct
range(0, count($xmlrpc_value->data) - 1) === array_keys($xmlrpc_value->data)
instead of the preg-implode you utilized. Also, what about using array_fill instead of array_pad in InsertQuery_sqlite?Comment #116
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdated patch. It turns out that SQLite can't count correctly the affected rows of an UPDATE query, so we workaround that too.
The search test suite passed at one point, but not currently (I'm trying to track down the regression).
Comment #117
drewish CreditAttribution: drewish commentedsubscribing
Comment #118
Damien Tournoud CreditAttribution: Damien Tournoud commentedNew update. We are very near 100% pass... most of the test that fails also fail on MySQL now. Need some love on the code, and on comments.
Comment #119
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere we are. Except Search (some strange math issues, needs investigation), I think this is functionally as good as the MySQL implementation.
Still needs love on optimization and clean-up.
Comment #120
Damien Tournoud CreditAttribution: Damien Tournoud commentedOh, and http://drupal.org/node/329080 is a prerequisite for the database tests to pass.
Comment #121
chx CreditAttribution: chx commentedI am working on a cleanup. I created a separate issue #331213: DBTNG: Make it easier to write weird database drivers out of my DB:TNG fixes.
Comment #122
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdated patch, based on and requiring #331213: DBTNG: Make it easier to write weird database drivers.
Some documentation clean-ups.
Comment #123
Damien Tournoud CreditAttribution: Damien Tournoud commentedFun. I just found out why search tests are failing. The search query is something like this (over simplified):
In theory, it should work this way (documented on [1]):
So the result should be sorted by the third output column (we are in case 2). But SQLite messes up and tries to sort using the search_index.score column (or some strange internal derivative, because it makes no sense to sort by that column in a grouped query...).
That bug is apparently documented since 2005 (see [2]).
[1] http://www.sqlite.org/lang_select.html
[2] http://www.sqlite.org/cvstrac/tktview?tn=1521
Comment #124
chx CreditAttribution: chx commented#331719: Make search query not ambigous should fix the search issue.
Comment #125
Damien Tournoud CreditAttribution: Damien Tournoud commented- added more documentation
- created DatabaseStatementPrefetch in database/prefetch.inc, and simplified the SQLite implementation
Comment #126
Damien Tournoud CreditAttribution: Damien Tournoud commentedNote that the patch above depends both on #331213: DBTNG: Make it easier to write weird database drivers and #329080: Statics in objects are not per-instance.
Comment #127
Damien Tournoud CreditAttribution: Damien Tournoud commentedMakes the new temporary test pass.
Comment #128
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere with some fixes following chx review on IRC.
Comment #129
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd now with a cleaner rewrote DatabaseStatementPrefetch.
Comment #130
Damien Tournoud CreditAttribution: Damien Tournoud commentedHere is an updated patch, following a very keen review by chx. The patch also includes a fix to a MergeQuery in the aggregator (see #332002: MergeQuery should refuse to execute if there are no key fields for the general issue).
Comment #131
chx CreditAttribution: chx commentedNote that you need to put the sqlite database in a directory writeable by Apache. Failing to do so will give you some error messages but not enough. This new patch gives you more information by throwing a PDO exception if
$statement->execute($args);
fails as well.Comment #132
Crell CreditAttribution: Crell commentedprefetch.inc:
- I like that you split off the prefetch logic to its own class. That should make later wacky drivers easier, too.
- DatabaseStatementPrefetch::$driver_options should be $driverOptions.
- I don't get why you have $fetchOptions and $defaultFetchOptions. Same for $fetchStyle. Why can't you just define it once, and the initial definition is the default? (Coming back later...) Ah, I see why you're doing that, down in fetch(). Clever. Can we include a note in the docblock for the default* variables as to why we're doing it? It's non-obvious until several hundred lines later.
- The rest of the DB system uses $connection for the connection object, not $database_connection. Let's stick with that in the constructor for consistency.
- Line 173, is the result set really not valid if empty? It should be valid, just, well, empty.
- In next(), the comment "We are fetch rows as PDO::FETCH_ASSOC..." doesn't parse grammatically. I also don't quite get what we're doing in that method as a result.
- In SelectQuery, I marked off blocks of methods that implement certain interfaces. I wonder if we should do the same here? I don't know of any actual standard for that, but it could be useful to do on large, complex setups like this.
- In fetchObject(), why can't we just default $class_name to stdClass and have a single code path?
- I'm curious why you're going the extra step of using the iterator methods internally "as PHP would have done", when in many cases it would be faster to skip the extra method calls. Eg, in fetchAll*() and fetchCol().
sqlite/database.inc:
- Why must PDOPrepare() be public? What "technical reasons"? I don't get why DatabaseStatement_sqlite::getStatement() can't just call PDO's prepare after it's done cleaning up the integer silliness.
- queryRange() doesn't need the BC str_replace() in there. That's already handled by the db_*() functions. There's another patch somewhere to remove that from the other implementations, too, as it's not needed.
- supportsTransactions() should not be hard coded to return TRUE. Let's support the transactions connection property in settings.php, even though we can/should default it to TRUE. It can be useful to have a kill switch for transactions in some cases, such as for testing.
- DatabaseStatement_sqlite doesn't need to explicitly implement Iterator and DatabaseStatementInterface. It already does so by virtue of extending DatabaseStatementPrefetch.
- In getStatement(), all internally-generated placeholders should be in the form :db_*, as that's documented as reserved for internal use. I recommend :db_statement_placeholder_$i, or something like that, to avoid possible conflicts elsewhere.
- In getStatement(), it's faster to check if the array is numeric with
is_numeric(key($fields))
. The query builders all do that.- In execute(), I'm unclear from the code comment what sort of post-processing we're doing and why. What post processing do those databases do?
sqlite/query.inc:
- Inserting all-default values when no fields are specified is an error. The "all default" case is handled with useDefaults(). There's another issue to make no-fields a no-op: #321100: Empty insert statements should fail gracefully
- In UpdateQuery_sqlite::execute(): "Get the fields use in the update query, and remove those that are already in the condition." I think that's useD, not use.
- In UpdateQuery_sqlite::execute(): "The IS NULL operator is badly managed by DatabaseCondition." How so, and how should we fix it?
- It took me a while to figure out what the removeFieldsInCondition() thing was all about. I get it from the class docblock, but that's almost too far away from where the action is. Can we move that description to either execute() or removeFieldsInCondition() and expand on it, or put an expanded version there? (And dear god, I can't believe these sorts of issues have lasted this long in PDO. Some drivers always return affected and others always return matched? Yeesh!)
- Does DeleteQuery_sqlite need the same remove-fields treatment as UpdateQuery_sqlite? They both are supposed to return the number of affected (not matched) rows.
sqlite/schema.inc:
- In processField(), there's a call to db_type_map(). That turns right back around and calls the active schema object to call getFieldTypeMap(). That's wasteful, and potentially a bug if the schema object in question is not the active database. Just go ahead and call $this->getFieldTypeMap().
- I understand why there's the createNewTableWithOldData() song and dance because I've talked to chx about it before. Most people won't. That should be clarified in the documentation, probably in the docblock of that method.
- introspectSchema() : Oooo...
- dropField():
unset() on a variable that doesn't exist is safe, so the if(empty()) is not necessary there.
Overall, I really dig this. My comments above are all tweaky stuff. This looks really solid, and the documentation is as extensive as it should be. Awesome work guys!
It also validates all the over-engineering we did in DBTNG in the first place; SQLite is as wacky as Oracle by the looks of it, but there are enough layers that we can compensate. :-)
Let's get the blocker patch in and then get this tested and polished and in ASAP. I'll even go as far as bumping it to critical. :-)
Comment #133
Damien Tournoud CreditAttribution: Damien Tournoud commentedFixed the documentation and name of member variables.
An empty result set means no traversable position will be valid.
Added comments following your convention. next() is the implementation of Iterator::next() and just need to update the valid flag if passed the end of the array. By the way, fixed rewind() that should also update the valid flag.
Casting to an object is faster, I also removed the need for the function call.
We generally only use $this->next(), in order to keep track of the current position in the result set.
Note that PDOStatement::fetchAll() is defined as "returns an array containing all of the remaining rows in the result set". We try to keep that.
It's exactly because DatabaseStatement_sqlite::getStatement() must call PDO::prepare() after it's done that we need this function to be public.
Cleaned-up. Next time, please keep the database layer clean.
In that case, why not managing that in DatabaseConnection? Added back transaction support.
I think it's cleaner that way.
Fixed.
Fixed.
Added more comments.
From that point, those are chx fixes:
Fixed.
Added more docs.
This is specific to SQLite. Added more doc.
That's voluntary: we should only remove the index if it has no more fields. Added a code comment.
Comment #134
chx CreditAttribution: chx commentedComment #135
Damien Tournoud CreditAttribution: Damien Tournoud commentedTwo small improvements:
- we now retry to execute the query if SQLite returned error 17 "the schema has changed". It makes running the tests more robust.
- we now support the RAND() function, required by devel_generate
Comment #136
Damien Tournoud CreditAttribution: Damien Tournoud commentedFixed schema.inc, now that we have a SchemaAPI unit test.
Comment #137
Damien Tournoud CreditAttribution: Damien Tournoud commentedUpdated to follow recent changes in queryTemporary().
Comment #138
Damien Tournoud CreditAttribution: Damien Tournoud commented... and following recent change on how empty insert queries are handled.
We are still on par with MySQL implementation on test results (100% pass).
Comment #139
catchNeeds to be updates to CHANGELOG.txt and INSTALL.txt - there's currently no indication of the feature apart from it showing up when you install if you've got SQLite enabled.
The database 'name' field actually needs to be the full path to the database (and that needs to be writable by apache) - this could use a description if sqlite is available to clarify what needs to be entered. Minor niggle though and could be a followup patch.
Everything else, very nice. Not going to pretend to review the patch itself, but install and tests ran smoothly with the above caveats.
Comment #140
chx CreditAttribution: chx commentedThe installer is already ongoing a change (on my desktop computer no patch, ETA this weekend) -- even if we decide not to go the whole nine yards and kill the installer alltogether it will be a full Drupal that runs the installer quite soon. Can we put off form altering the installer until that happens? Would be a quite a bit easier. Also, can we put off writing INSTALL.sqlite.txt until that happens? I have attached a patch that has no changes at all just added a line to CHANGELOG.
Comment #141
webchickFeedback from first hour spent on this patch. Since this involved opening includes/databases/X.inc, includes/databases/mysql/X.inc, and includes/databases/sqlite/X.inc and trying to figure out wtf was going on, most of the current feedback is pretty shallow. :P I figure by another 2-3 hours I should understand this enough to be in a position to commit it, unless Dries beats me to it. ;)
I skipped over includes/database/prefetch.inc entirely for now; trying to understand the rest first.
Comments:
While this comment is very technically accurate, it doesn't help explain why this is the case. Even reading the bug report mentioned below didn't help much. It would be helpful to know why we need a predictable sequence of random numbers.
Also? That needs some English help. "predictable" and "numbers".
Same thing here. Please don't assume that I know what the technical reasons for why this can't be private. :) According to chx, it's because you want another class which is not a direct sub-class to be able to call this and there's no such thing as "package-level" scope in PHP. The comment should reflect this.
It would be very helpful to know, since this is one of first "exotic" database, why you were not able to extend from PDOStatement here. chx wrote up a bunch of stuff in IRC about it, but nothing is remotely reflected in the code comments either here or in DatabaseStatementPrefetch from what I can tell.
That should be in a @file PHPDoc block. And yes, I know MySQL's doesn't do that, but MySQL's is wrong. :P
It could be because it's 1:30am, but I sat staring at that line for 5 minutes trying to figure out what it did. I can haz inline comment?
Questions as I was going through:
1. Is there a reason introspectSchema is only in sqlite and not in other backends? Answer: yes. chx has not written it yet for the other two but is planning to. Will be in a follow-up issue.
2. Since both mysql and sqlite have a transactionSupport variable, shouldn't this go into DatabaseConnection? Answer: Yes, in a follow-up patch.
3. Is there a reason that Oracle/MSSQL drivers can't use things like sqliteCreateFunction? Answer: Yes; SQLite is special here because it's embedded in PHP.
Comment #142
chx CreditAttribution: chx commentedunset($statement)
toDatabaseStatementPrefetch::execute
, as the variable is local, not an object property, the variable would nicely get out of scope and be destroyed at the end of the execute but why not destroy as soon as possible? (yes there are inline comments) Given circumstances (now documented in code as well, see PDOPrepare doxygen) I really see it necessary to emphasise the destroying this object so that the core coder who touches this code two years from now won't save it somewhere accidentally.Comment #143
cburschkaTrying to install a patched site and choosing SQLite, nothing seems to be happening. I submit the DB configuration form and it reloads with default values.
Edit: This appears to be unrelated to SQLite, as the same happens with MySQL.
Installer regression I guess; I'll file it.
Comment #144
cburschkaNote that when installing an SQLite site, the SQLite database will be created in the codebase root directory; it will also be publically downloadable.
Of course both of these should no longer be a problem once chx is done with the new installer. ;)
Comment #145
chx CreditAttribution: chx commentedYou should name your database something like sites/default/files/.ht.sqlite because a) it needs to be in a writeable directory. b) .ht* is stopped by Apache from downloading.
Comment #146
Damien Tournoud CreditAttribution: Damien Tournoud commentedRight, I got the change on mt_rand() wrong. The sequence changed in PHP 5.2.1, but the sequence is still the same for a given seed. It's perfectly ok to use here.
Comment #147
cburschkaThanks. If the initial installer would be likely to remain in the stable release, I'd suggest explaining this in the installation form - but I guess that once Drupal comes with SQLite out of the box, manually installing another SQLite database will be rare enough for this not to matter.
Edit: For what it's worth, I am now using the SQLite driver on my test site, and it is working stably and fast. Good job - I hope it won't be much longer before it gets committed.
Comment #149
webchickResetting to needs review due to earlier testing bot snafu
Comment #150
webchickOkay, picking up where I left off: schema.inc!
We do not abbreviate function names, and this function is missing PHPDoc. Since it's not defined in includes/databases/schema.inc I know you made it up. ;)
Looks like a hunk of debug code. Please remove.
Needs a period.
Can that please be a bit less cryptic? :P
The dropField() function is significantly more complex here than it is in pgsql/mysql. I asked why we don't do just a straight-up ALTER TABLE here, and was told that SQLite only supports a subset of ALTER TABLE, and we have to handle the index removal ourselves (http://www.sqlite.org/lang_altertable.html for reference). Let's please add a comment to this effect, as I can see someone coming along and trying to "optimize" this later.
This introspectSchema() function looks COOL. I imagine it's a little bit of a performance detriment to go from:
to:
But the implications of having such a function are very powerful.
However, on that note:
While it might not be as technically accurate, I think $this->alterTable() would make a lot more sense to people coming from an SQL background. It's also far less typing. ;)
The dropField function looks quite a bit more complex than simply performing an ALTER TABLE here.
Questions that came up while reviewing:
Q: Why are dropIndex/addIndex/addUniqueKey not using this fancy introspectSchema() stuff?
A: Because they are not part of the table structure in any database but MySQL.
Q: Why does SQLite use function createIndexSql/createKeySql and pgsql use function _createIndexSql/_createKeySql?
A: Because pgsql is neglected and using legacy function prefixing which we don't need anymore since the shift to OOP.
(ACTION ITEM: Someone please make a follow-up issue for this.)
Q: Since all three database backends have function createKeySql, should that be moved to includes/databases/schema.inc?
A: No, because there's not a great way to genericize it.
Things I still need to look at in more detail:
prefecth.inc ... dun dun dunnnn...
Comment #151
Damien Tournoud CreditAttribution: Damien Tournoud commented- Removed the strange comments.
- Renamed buildColsSql() to createColumnsSql().
- Renamed createNewTableWithOldData() to alterTable() and added some comments to the other functions to explain why we need this.
Comment #152
Damien Tournoud CreditAttribution: Damien Tournoud commentedAnd prefetch.inc was missing...
Comment #153
Damien Tournoud CreditAttribution: Damien Tournoud commentedReran the whole test suite... gosh, one of the recently introduced test was failing...
It turns out that SQLite does not return the number of affected rows correctly when doing a DELETE query without conditions (it optimizes that operation by simply dropping the table and recreating it).
Added a workaround to DeleteQuery... all tests pass again.
Comment #154
webchickLet's please add (much) more documentation here. Reading this should be able to answer questions like:
a) What does prefetch result sets mean?
b) How would I know if I'm using such an engine?
c) How do I know if the engine I'm writing a db back end for needs to implement this?
prefetches.
$columnNames.
...in this result set.
What on earth is a fetch style? :)
Two periods.
We try not to abbreviate variable names. Or is this coming from PDO so we have to?
Needs period.
You already did a count, why not do the if on $this->resultRowCount?
Maybe "ensure that the array starts with a clean slate"?
Needs PHPDoc.
Needs PHPDoc.
Also, $a2? $a3? :( Are there better names for these? If not, can we at least comment why they're so bad?
Don't need a break; after a return;
I want one more pass at this after these changes are implemented, and then I think this is ready to go.
Comment #155
chx CreditAttribution: chx commentedMuch more comments to prefetch.inc per webchick's review.
Comment #156
chx CreditAttribution: chx commentedA ton of small comments and spacing fixes per keithsmith. I love you guys.
Comment #157
chx CreditAttribution: chx commentedcreateColumsSql now uses a
$sql_array
and implode instead of cutting down the last chars.Comment #158
webchickOk. I've now been through this patch about 6 times, have installed and played around with it, and am finding a really hard time trying to find anymore flaws with it.
No doubt there are some that will crop up eventually, since the patch is quite large, but since this is new functionality that will break nothing for anyone else, it doesn't seem that there's any further reason to hold back this patch.
So.....
Committed to HEAD. :) Great work, everyone!
Comment #159
robertDouglass CreditAttribution: robertDouglass commentedNice stuff. I immediately went to try it. I grabbed HEAD and created a new db by typing
sqlite3 drupaltest.db
on the command line. I then went through the installer using "drupaltest.db" as the db name, leaving username and password empty. The following was printed to out and execution stopped:I posted here because I don't otherwise know how to describe the bug(?) and know that everybody with this fresh on their minds will see it.
Comment #160
robertDouglass CreditAttribution: robertDouglass commentedIs this the problem?
PDO drivers => sqlite2, sqlite, pgsql, mysql
pdo_sqlite
Comment #161
catchNice work everyone:
@robertDouglass - comments in #139 might help - installer just needs full path to a writable file, no need to create the db from the command line. I went through exactly the same thing testing the patch. Opened a new issue for INSTALL.txt/installer - #337993: No install instructions for SQLite
Comment #162
Damien Tournoud CreditAttribution: Damien Tournoud commentedLast minute changes always hurt the most. There is a parenthesis missing in createColumsSql()...
Comment #163
webchickD'oh. That's weird. I installed it with SQLite a couple times and didn't run across that problem. Must not have tried the very latest patch.
Committed with the correct name. ;) Thanks, Damien!
Comment #164
kylehase CreditAttribution: kylehase commentedJust tried Drupal 7-dev and noticed sqlite option on install. Interesting feature. Subscribing.