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).

CommentFileSizeAuthor
#162 67349-sqlite-fix.patch773 bytesDamien Tournoud
#157 sqlite_final.patch45.5 KBchx
#156 sqlite_N+1.patch45.55 KBchx
#155 sqlite_N.patch45.53 KBchx
#153 67349-support-sqlite-databases.patch46.34 KBDamien Tournoud
#152 67349-support-sqlite-databases.patch46.19 KBDamien Tournoud
#151 67349-support-sqlite-databases.patch32.61 KBDamien Tournoud
#142 sqlite.patch43.56 KBchx
#140 67349-support-sqlite-databases.patch42.92 KBchx
#138 67349-support-sqlite-databases.patch42.94 KBDamien Tournoud
#137 67349-support-sqlite-databases.patch42.85 KBDamien Tournoud
#136 67349-support-sqlite-databases.patch42.83 KBDamien Tournoud
#135 67349-support-sqlite-databases.patch42.21 KBDamien Tournoud
#133 67349-support-sqlite-databases.patch41.38 KBDamien Tournoud
#131 sqlite_67349_131.patch41.33 KBchx
#130 67349-sqlite-support.patch41.34 KBDamien Tournoud
#129 67349-sqlite-support.patch40.42 KBDamien Tournoud
#128 67349-sqlite-support.patch38.22 KBDamien Tournoud
#127 67349-sqlite-support.patch37.21 KBDamien Tournoud
#125 67349-sqlite-support.patch37.25 KBDamien Tournoud
#122 67349-sqlite-support.patch36.8 KBDamien Tournoud
#119 67349-sqlite-support.patch38.31 KBDamien Tournoud
#118 67349-sqlite-support.patch35.27 KBDamien Tournoud
#116 67349-sqlite-support.patch37.28 KBDamien Tournoud
#114 67349-sqlite-support.patch22.38 KBDamien Tournoud
#114 081105 - sqlite.html_.gz98.42 KBDamien Tournoud
#113 67349-sqlite-support.patch21.3 KBDamien Tournoud
#110 67349-sqlite-support.patch22.15 KBDamien Tournoud
#109 sqlite.patch21.36 KBchx
#108 sqlite-test-run-1.txt35.28 KBDamien Tournoud
#104 sqlite.patch20.8 KBchx
#102 sqlite.patch48.56 KBchx
#100 sqlite.patch36.46 KBchx
#99 sqlite.patch38.98 KBchx
#96 sqlite_to_be.tgz7.29 KBchx
#87 sqlite_10.patch.txt61.02 KBparanojik
#79 drupal-5-1_sqlite.patch94.3 KBRoberto Gerola
#72 sqlite_for_5_1.patch79.25 KBparanojik
#71 sqlite_8.patch.txt80.23 KBparanojik
#70 sqlite_7.patch.txt80.76 KBparanojik
#67 sqlite_6.patch.txt79.18 KBparanojik
#62 sqlite_5.patch.txt74.61 KBparanojik
#57 sqlite.patch4_.txt75.01 KBparanojik
#53 sqlite.patch3_.txt77.02 KBparanojik
#52 sqlite.patch285.75 KBparanojik
#32 setup-screen.png36.24 KBparanojik
#31 sqlite.patch66.6 KBparanojik
#30 database.sqlite.inc_0.txt20.92 KBparanojik
#25 mysql-sqlite-bench.txt2.03 KBparanojik
#15 database.sqlite.inc3.txt13.29 KBparanojik
#14 database.sqlite.inc2.txt12.33 KBparanojik
#13 database.sqlite.2.txt23.16 KBparanojik
#12 database.sqlite.inc1.txt12.39 KBparanojik
#9 database.sqlite.inc0.txt9.5 KBAnonymous (not verified)
#8 database.sqlite2_0.txt23.17 KBAnonymous (not verified)
#6 database.sqlite2.txt23.17 KBAnonymous (not verified)
#1 database.sqlite.txt23.14 KBAnonymous (not verified)
database.sqlite.inc.txt10.32 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

FileSize
23.14 KB

initial database file...

Tobias Maier’s picture

Status: Active » Needs work
chx’s picture

There 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.

Anonymous’s picture

Im 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.

chx’s picture

the 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 be n.nid so we need to add as nid or upon fetch rename the the index.

Anonymous’s picture

FileSize
23.17 KB

This fixes the problem with creating aggregator feeds.

Anonymous’s picture

Status: Needs work » Fixed

This 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.

Anonymous’s picture

Status: Fixed » Needs work
FileSize
23.17 KB

oops, and the file...

Anonymous’s picture

FileSize
9.5 KB

new database include file

chx’s picture

I scream for benchmarks when I see SQL called from preg_replace_callback

Steven’s picture

  • No db_create_temporary? If this is not possible, it needs to be documented. Alternatively, given that SQLite is supposed to be fast, we could extend the SQL layer to require you to explicitly mark temporary tables when no longer needed. This would allow you to use non-temporary tables to emulate them, without polluting your database or doing a garbage collector.
  • The code style is all over the place. Especially function naming is poor (db_escape_string2 ?). Some doxygen seems mangled. Some variables are declared global, but unused.
  • This needs /much/ better commenting. Entire function bodies are uncommented. You need to explain what you are doing and why you are doing it.
  • db_connect() needs error handling and friendly output.
  •  * sqlite_udf_encode_binary in the PHP manual says sg. about first byte should not be 0x01 and there should not be 0x00 chars. 
     * But we can safely assume only UTF-8 strings will be CONCATed, no problem with that.
    

    Drupal offers no such guarantee, a user is free to submit any bytestream they want and pass it off as UTF-8.

paranojik’s picture

FileSize
12.39 KB
  • added error handling in db_connect()
  • added db_create_temporary() (not tested)
paranojik’s picture

FileSize
23.16 KB

fixed a typo in users table creation query (database.sqlite)

paranojik’s picture

FileSize
12.33 KB

fixed _sqlite_expand() (solves the above mentioned problems with user logins)

paranojik’s picture

FileSize
13.29 KB

some work on coding style and doxygen comments

Dries’s picture

Status: Needs work » Needs review

Looks like we might be ready for another review ...

Anonymous’s picture

The 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.

chx’s picture

that bench, in itself is not really useful -- care to tell us how it compares when using a mysql server on the same machine?

chx’s picture

Also, 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.

Dries’s picture

tachyonxv: you have to compare your benchmark results against a baseline scenario (a MySQL setup with the same amount of content / configuraiton).

paranojik’s picture

Queries 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)?

Steven’s picture

IIRC, 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.

chx’s picture

devel project has generate scripts

paranojik’s picture

Steven: 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.

paranojik’s picture

FileSize
2.03 KB

I'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.

chx’s picture

See 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.

chx’s picture

Document 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.

paranojik’s picture

I 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.

paranojik’s picture

I'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?

paranojik’s picture

FileSize
20.92 KB

I'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...

paranojik’s picture

FileSize
66.6 KB

The 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.

paranojik’s picture

FileSize
36.24 KB

the configuration form

chx’s picture

  • it was always my dream for Drupal to run with SQLite. We could ship ready-to-run with a sqlite database file already prepared, without any install whatsoever.
  • Still we need someone who will say "I want to maintain SQLite for Drupal". See #19 above.
  • Function altertable should be in a separate include file which is loaded on demand only. See xmlrpc() on how this is done.
  • How have you dealt with the dreaded alias bug of SQLite 2? see #5
paranojik’s picture

@#5 If I understand correctly that's not a bug. Sqlite docs: PRAGMA command syntax under PRAGMA full_column_names;.

chx’s picture

Oh, 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?

paranojik’s picture

I agree it would be very nice to ship drupal with a prebuilt database, but then even more problems arise:

  • How to support installation profiles,
  • who/when/how would build the database,
  • storing binary files in CVS,

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.

Anonymous’s picture

It 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.

forngren’s picture

I agree it would be very nice to ship drupal with a prebuilt database, but then even more problems arise:

* How to support installation profiles,
* who/when/how would build the database,
* storing binary files in CVS,

Well, we don't have fill it with tables, if we just prebuild the database connection, installation will be alot easier.

chx’s picture

  • How to support installation profiles -- easy, you drop the tables preloaded for the default profile.
  • who/when/how would build the database -- you, whenever it fits you, by posting your database file right after install in a separate issue
  • storing binary files in CVS -- since when this is a problem? CVS can store binary files.
  • I am willing to maintain this port

    That'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.

chx’s picture

paranojik’s picture

* How to support installation profiles -- easy, you drop the tables preloaded for the default profile.

So... 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?)

* who/when/how would build the database -- you, whenever it fits you, by posting your database file right after install in a separate issue

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?)

* storing binary files in CVS -- since when this is a problem? CVS can store binary files.

I know it can, but it's quite expensive. On the other hand, there won't be that many revisions of the database....

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.

Sounds like a different debate... isn't it?

necromancer-1’s picture

so are we ready to fly? :)

forngren’s picture

It's probably to late to get this into Drupal 5, but why can't this be the first patch in 6? ;)

Great work paranojik!

coreb’s picture

Version: x.y.z » 6.x-dev

moving from x.y.z to 6.x-dev version queue

viperjason’s picture

Version: 6.x-dev » 5.0

So, then the question is, can this be used with 5.0 or do I need to use 4.7?

viperjason’s picture

I'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.

Steven’s picture

Version: 5.0 » 6.x-dev

Please don't change the properties of issues for no particular reason.

moshe weitzman’s picture

subscribing

viperjason’s picture

Sorry 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.

paranojik’s picture

Look 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...

viperjason’s picture

I 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.

paranojik’s picture

FileSize
85.75 KB

reapplied

paranojik’s picture

FileSize
77.02 KB

Rerolled.
To install all you have to do is select 'sqlite' and hit 'Save settings'.

MaikR’s picture

Okay, 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".

MaikR’s picture

It 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?

moshe weitzman’s picture

I 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.

paranojik’s picture

FileSize
75.01 KB

Rerolled. 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...).

forngren’s picture

As 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?

moshe weitzman’s picture

minor - install.sqlite.inc has error messages that refer to MySQL.

paranojik’s picture

@58: You can give your database file any (valid) name you want.

chx’s picture

As 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.

paranojik’s picture

FileSize
74.61 KB

@59: Thanks. Fixed, although they may still need corrections.

forngren’s picture

I 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.

MaikR’s picture

When 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?

forngren’s picture

a-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?

chx’s picture

CREATE TABLE "<?php exit; parse_error_in_worst_case"(donotremovethistable boolean); let it be. I always forget this nice trick. Very, very cool.

paranojik’s picture

FileSize
79.18 KB

Implemented as suggested by forngren.

forngren’s picture

I 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.

chx’s picture

forngren, 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.

paranojik’s picture

FileSize
80.76 KB

Keeping up with HEAD.

paranojik’s picture

FileSize
80.23 KB

Chain 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).

paranojik’s picture

FileSize
79.25 KB

This applies to Drupal 5.1.

ebruts’s picture

Have not try it yet but found these as i browsed through the patch.

+function _sqlite_year($date) {
+  return date("Y", strtotime($fate));
+}
+
+function _sqlite_month($date) {
+  return date("m", strtotime($fate));
+}
pukku’s picture

Hi! 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 do http://localhost/~pukku/Sites/drupal, and I get the following errors:

Notice: Trying to get property of non-object in /Users/pukku/Sites/drupal/includes/form.inc on line 325

Notice: Undefined variable: redirect in /Users/pukku/Sites/drupal/includes/form.inc on line 268

Notice: Undefined variable: base in /Users/pukku/Sites/drupal/includes/form.inc on line 461

Notice: Undefined index: #value in /Users/pukku/Sites/drupal/includes/form.inc on line 1041

Notice: Undefined variable: no_module_preprocess in /Users/pukku/Sites/drupal/includes/common.inc on line 1467

Notice: Undefined variable: no_theme_preprocess in /Users/pukku/Sites/drupal/includes/common.inc on line 1488

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).

moshe weitzman’s picture

would 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.

pukku’s picture

This 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...

pukku’s picture

Is 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

chx’s picture

SQLite3 requires PDO (RTFM, damnit!). I would much like to see this rerolled for Schema and included in D6.

Roberto Gerola’s picture

FileSize
94.3 KB

Patch 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

David Strauss’s picture

"Username is a requred field for MySQL and Postgres databases."

Required is misspelled.

David Strauss’s picture

Status: Needs review » Needs work

* 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.

David Strauss’s picture

On 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.

sebastien.barre’s picture

> 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...

David Strauss’s picture

Even Drupal 5 has a PHP 5-only database engine, mysqli.

hswong3i’s picture

Title: Sqlite port » Support SQLite databases
Version: 6.x-dev » 7.x-dev

subscribe :)

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 :)

aclight’s picture

subscribing

paranojik’s picture

FileSize
61.02 KB

I 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...

catch’s picture

subscribing.

greggles’s picture

Fwiw, 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.

hswong3i’s picture

I 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 on pdo_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 ;-)

BioALIEN’s picture

Things have slowed down considerably on this, but I would welcome the prospect of having this in D7.

Crell’s picture

Status: Needs work » Postponed

@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.)

xaweryz’s picture

subscribing

Robin Monks’s picture

subscribing

catch’s picture

Status: Postponed » Active

:)

chx’s picture

FileSize
7.29 KB
lilou’s picture

Status: Active » Needs review
Rainy Day’s picture

Subscribing.

chx’s picture

FileSize
38.98 KB

So 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 :)

chx’s picture

FileSize
36.46 KB

Discussed with Crell and now update_sql happily resets the preparequery static cache. Less code. Yay.

dmitrig01’s picture

+ public function supportsTransactions() {
+ return TRUE;;
+ }
two sems,

the
+ do {
+ $temp_table = $table . '_' . $i++;
+ } while ($this->tableExists($temp_table));
near the bottom isn't necesarry

chx’s picture

FileSize
48.56 KB

We 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.

chx’s picture

The 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.

chx’s picture

FileSize
20.8 KB

This 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.

Damien Tournoud’s picture

@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.

chx’s picture

Damien, 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 :)

Damien Tournoud’s picture

Further testing:

$test_id = db_insert('simpletest_test_id')->useDefaults(array('test_id'))->execute();

Don't seems to work (it returns 0).

I can't get parallel simpletests to work: I'm bumping into this error:

General error: 17 database schema has changed in database.inc on line 365.

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).

Damien Tournoud’s picture

FileSize
35.28 KB

Here is a full test log. It's the run of a slightly modified run-tests with debug stuff added.

chx’s picture

FileSize
21.36 KB

Here is a reroll. The update feed items test fail miserably when it tries to delete the feed. Strange. Slow. Very strange.

Damien Tournoud’s picture

FileSize
22.15 KB

Some more fun with SQlite... we now have only one database test failing! (complex queries)... ok actually two (log fails too, for some reason).

Damien Tournoud’s picture

Ok, 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:

In the general sense, a Merge query is a combination of an Insert query and and Update query.

chx’s picture

Damien, 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.

Damien Tournoud’s picture

FileSize
21.3 KB

Updated patch, with MergeQuery_sqlite killed.

It looks like SQLite has an issue replacing placeholders in HAVING clauses, so I've overriden SelectQuery for now.

Damien Tournoud’s picture

chx 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

chx’s picture

Damien, 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?

Damien Tournoud’s picture

FileSize
37.28 KB

Updated 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).

drewish’s picture

subscribing

Damien Tournoud’s picture

FileSize
35.27 KB

New 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.

Damien Tournoud’s picture

FileSize
38.31 KB

Here 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.

Damien Tournoud’s picture

Oh, and http://drupal.org/node/329080 is a prerequisite for the database tests to pass.

chx’s picture

I 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.

Damien Tournoud’s picture

FileSize
36.8 KB

Updated patch, based on and requiring #331213: DBTNG: Make it easier to write weird database drivers.

Some documentation clean-ups.

Damien Tournoud’s picture

Fun. I just found out why search tests are failing. The search query is something like this (over simplified):

SELECT type, sid, SUM(score) AS score FROM search_index GROUP BY type, sid ORDER BY score DESC;

In theory, it should work this way (documented on [1]):

Each term of an ORDER BY expression is processed as follows:
1. If the ORDER BY expression is a constant integer K then the output is ordered by the K-th column of the result set.
2. If the ORDER BY expression is an identifier and one of the output columns as an alias by the same name, then the output is ordered by the identified column.
3. Otherwise, the ORDER BY expression is evaluated and the output is ordered by the value of that expression.

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

chx’s picture

#331719: Make search query not ambigous should fix the search issue.

Damien Tournoud’s picture

FileSize
37.25 KB

- added more documentation
- created DatabaseStatementPrefetch in database/prefetch.inc, and simplified the SQLite implementation

Damien Tournoud’s picture

Damien Tournoud’s picture

FileSize
37.21 KB

Makes the new temporary test pass.

Damien Tournoud’s picture

FileSize
38.22 KB

Here with some fixes following chx review on IRC.

Damien Tournoud’s picture

FileSize
40.42 KB

And now with a cleaner rewrote DatabaseStatementPrefetch.

Damien Tournoud’s picture

FileSize
41.34 KB
The tests have finished running.
7227 passes, 0 fails, and 0 exceptions

Here 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).

chx’s picture

FileSize
41.33 KB

Note 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.

Crell’s picture

Priority: Normal » Critical
Status: Needs review » Needs work

prefetch.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():

if (empty($new_schema['indexes'][$index])) {
  unset($new_schema['indexes'][$index]);
}

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. :-)

Damien Tournoud’s picture

prefetch.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.

Fixed the documentation and name of member variables.

- Line 173, is the result set really not valid if empty? It should be valid, just, well, empty.

An empty result set means no traversable position will be valid.

- 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.

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.

- In fetchObject(), why can't we just default $class_name to stdClass and have a single code path?

Casting to an object is faster, I also removed the need for the function call.

- 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().

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.

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.

It's exactly because DatabaseStatement_sqlite::getStatement() must call PDO::prepare() after it's done that we need this function to be public.

- 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.

Cleaned-up. Next time, please keep the database layer clean.

- 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.

In that case, why not managing that in DatabaseConnection? Added back transaction support.

- DatabaseStatement_sqlite doesn't need to explicitly implement Iterator and DatabaseStatementInterface. It already does so by virtue of extending DatabaseStatementPrefetch.

I think it's cleaner that way.

- 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.

Fixed.

- In getStatement(), it's faster to check if the array is numeric with is_numeric(key($fields)). The query builders all do that.

Fixed.

- 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?

Added more comments.

From that point, those are chx fixes:

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().

Fixed.

- 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.

Added more docs.

- introspectSchema() : Oooo...

This is specific to SQLite. Added more doc.

- dropField():

if (empty($new_schema['indexes'][$index])) {
  unset($new_schema['indexes'][$index]);
}

unset() on a variable that doesn't exist is safe, so the if(empty()) is not necessary there.

That's voluntary: we should only remove the index if it has no more fields. Added a code comment.

chx’s picture

Status: Needs work » Needs review
Damien Tournoud’s picture

Two 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

Damien Tournoud’s picture

Fixed schema.inc, now that we have a SchemaAPI unit test.

Damien Tournoud’s picture

Updated to follow recent changes in queryTemporary().

Damien Tournoud’s picture

... and following recent change on how empty insert queries are handled.

We are still on par with MySQL implementation on test results (100% pass).

catch’s picture

Needs 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.

chx’s picture

The 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.

webchick’s picture

Status: Needs review » Needs work

Feedback 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:

// We don't use mt_rand here, because there is no way to have a predicable
// sequence of random number since PHP 5.2.1.

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".

+  * This should be private, but can't for technical reasons.

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.

+class DatabaseStatement_sqlite extends DatabaseStatementPrefetch implements Iterator, DatabaseStatementInterface {

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.

+// SQLite specific install functions

That should be in a @file PHPDoc block. And yes, I know MySQL's doesn't do that, but MySQL's is wrong. :P

+    $placeholders = array_fill(0, count($this->insertFields), '?');

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.

chx’s picture

Status: Needs work » Needs review
FileSize
43.56 KB
  1. The mt_rand stuff was a misunderstanding. Although the SQL "RAND(n) function returns a random floating-point value v in the range 0 <= v < 1.0. [...] N [...] is used as the seed value, which produces a repeatable sequence of column values" but nowhere does it say that this should be repeatable if the server software changes. I think just mt_rand will do well. Especially that "[...] we're unlikely to see another change in the algorithm so results from 5.2.1 should remain constant in future versions of php" Edit: "RAND() is not meant to be a perfect random generator, but instead is a fast way to generate ad hoc random numbers which is portable between platforms for the same MySQL version." -- it says, same MySQL version. I presume, then, that same SQLite version aka. same PHP version is a reasonable assumption.
  2. Yes,you can haz. Lots of comments added.
  3. Introspection is already filed at #301038: Add a cross-compatible database schema introspection API.
  4. To make me sleep better at night I have added a unset($statement) to DatabaseStatementPrefetch::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.
cburschka’s picture

Trying 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.

cburschka’s picture

Note 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. ;)

chx’s picture

You 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.

Damien Tournoud’s picture

I think just mt_rand will do well. Especially that "[...] we're unlikely to see another change in the algorithm so results from 5.2.1 should remain constant in future versions of php" Edit: "RAND() is not meant to be a perfect random generator, but instead is a fast way to generate ad hoc random numbers which is portable between platforms for the same MySQL version." -- it says, same MySQL version. I presume, then, that same SQLite version aka. same PHP version is a reasonable assumption.

Right, 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.

cburschka’s picture

You 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.

Thanks. 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

Status: Needs work » Needs review

Resetting to needs review due to earlier testing bot snafu

webchick’s picture

Okay, picking up where I left off: schema.inc!

+  protected function buildColsSql($tablename, $schema) {

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. ;)

+    /*
+    a VARCHAR(10),
+    b NVARCHAR(15),
+    c TEXT,
+    d INTEGER,
+    e FLOAT,
+    f BOOLEAN,
+    g CLOB,
+    h BLOB,
+    i TIMESTAMP,
+    j NUMERIC(10,5)
+    k VARYING CHARACTER (24),
+    l NATIONAL VARYING CHARACTER(16)
+    */

Looks like a hunk of debug code. Please remove.

+   *   The field specification array, as taken from a schema definition

Needs a period.

+    // TODO: keys_new

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:

  public function dropPrimaryKey(&$ret, $table) {
    $ret[] = update_sql('ALTER TABLE {' . $table . '} DROP PRIMARY KEY');
  }

to:

+  public function dropPrimaryKey(&$ret, $table) {
+    $new_schema = $this->introspectSchema($table);
+    unset($new_schema['primary key']);
+    $this->createNewTableWithOldData($ret, $table, $new_schema);
+  }

But the implications of having such a function are very powerful.

However, on that note:

+    $this->createNewTableWithOldData($ret, $table, $new_schema);

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...

Damien Tournoud’s picture

- 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.

Damien Tournoud’s picture

And prefetch.inc was missing...

Damien Tournoud’s picture

Reran 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.

webchick’s picture

Status: Needs review » Needs work
+/**
+ * @file
+ * Database interface code for engines that need to fully prefetch the result sets.
+ */

Let'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?

+ * An implementation of DatabaseStatementInterface that prefetch all data.

prefetches.

+  protected $columNames = NULL;

$columnNames.

+   * The number of rows of this result set.

...in this result set.

+   * Holds the current fetch style (which will be used by the next fetch).

What on earth is a fetch style? :)

+   * Holds supplementary current fetch options (which will be used by the next fetch)..

Two periods.

+    'constructor_args' => array(),

We try not to abbreviate variable names. Or is this coming from PDO so we have to?

+   * Executes a prepared statement

Needs period.

+    $this->resultRowCount = count($this->data);
+
+    if (count($this->data)) {

You already did a count, why not do the if on $this->resultRowCount?

+    // This is necessary to ensure that the array with start in a clean state.

Maybe "ensure that the array starts with a clean slate"?

+  public function getQueryString() {

Needs PHPDoc.

+  public function setFetchMode($fetchStyle, $a2 = NULL, $a3 = NULL) {

Needs PHPDoc.

Also, $a2? $a3? :( Are there better names for these? If not, can we at least comment why they're so bad?

+          return array_values($row);
+          break;

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.

chx’s picture

Status: Needs work » Needs review
FileSize
45.53 KB

Much more comments to prefetch.inc per webchick's review.

chx’s picture

FileSize
45.55 KB

A ton of small comments and spacing fixes per keithsmith. I love you guys.

chx’s picture

FileSize
45.5 KB

createColumsSql now uses a $sql_array and implode instead of cutting down the last chars.

webchick’s picture

Status: Needs review » Fixed

Ok. 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!

robertDouglass’s picture

Nice 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:


PDOException: CREATE TABLE {variable} (
name VARCHAR(128) NOT NULL DEFAULT '', 
value TEXT NOT NULL, 
 PRIMARY KEY (name
);
 - 
Array
(
)
SQLSTATE[HY000]: General error 1: near ";": syntax error in /Users/robert/drupalconszeged/sqllite/includes/database/database.inc on line 494

Call Stack:
    0.0650   1. {main}() /Users/robert/drupalconszeged/sqllite/install.php:0
    0.1286   2. install_main() /Users/robert/drupalconszeged/sqllite/install.php:1188
    0.9063   3. drupal_install_system() /Users/robert/drupalconszeged/sqllite/install.php:149
    0.9287   4. module_invoke() /Users/robert/drupalconszeged/sqllite/includes/install.inc:570
    0.9288   5. call_user_func_array() /Users/robert/drupalconszeged/sqllite/includes/module.inc:520
    0.9288   6. system_install() /Users/robert/drupalconszeged/sqllite/includes/module.inc:0
    0.9288   7. drupal_install_schema() /Users/robert/drupalconszeged/sqllite/modules/system/system.install:344
    0.9298   8. db_create_table() /Users/robert/drupalconszeged/sqllite/includes/common.inc:3379
    0.9298   9. DatabaseSchema->createTable() /Users/robert/drupalconszeged/sqllite/includes/database/database.inc:1817
    0.9300  10. update_sql() /Users/robert/drupalconszeged/sqllite/includes/database/schema.inc:380
    0.9300  11. DatabaseConnection->query() /Users/robert/drupalconszeged/sqllite/includes/database/database.inc:1744

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.

robertDouglass’s picture

Is this the problem?

PDO drivers => sqlite2, sqlite, pgsql, mysql
pdo_sqlite

catch’s picture

Nice 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

Damien Tournoud’s picture

Status: Fixed » Needs review
FileSize
773 bytes

Last minute changes always hurt the most. There is a parenthesis missing in createColumsSql()...

webchick’s picture

Status: Needs review » Fixed

D'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!

kylehase’s picture

Just tried Drupal 7-dev and noticed sqlite option on install. Interesting feature. Subscribing.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.