Fix and enhance postgres and mysql support in updates.inc
Cvbge - August 17, 2005 - 21:25
| Project: | Drupal |
| Version: | x.y.z |
| Component: | database system |
| Category: | task |
| Priority: | critical |
| Assigned: | Cvbge |
| Status: | closed |
Description
Postgres, since 6.4, has version() function so you can check db version from SQL. This can be used in updates.inc for example for conditional dropping of the column (which is available since 7.3, not 7.4 as stated in updates.inc).
I need to check how the version string is returned in different versions before making changes.
#1
Here's a function returning postgres db version. I'm not sure if it fits in updates.inc, maybe it should be somewhere else.
#2
Added 2 new functions to add or change column definition. The take care about mysql/pgsql. Code not tested, will test and update old code when I hear that this has a chanse to go in ;)
#3
Still more work needed.
#4
With attached patch I could make all updates without errors (my LOCKs patch was needed - http://drupal.org/node/22911). There are still some issues that should be addressed (see the patch). Also mysql update is not tested - please test.
There are some problems when updating from 4.6 to cvs. Maybe the update should be made by 4.6 base itself?
So I've started with fresh 4.6.3.
After changing the base to cvs, patching with the LOCKs patch and simple debug helper patch, on update.php?op=update I get following errors:
warning: pg_query(): Query failed: ERROR: column "cache" of relation "sessions" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71.
Warning: pg_query(): Query failed: ERROR: column "referer" of relation "watchdog" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71
Bad query:
Array
(
[0] => SELECT u.*, s.* FROM users u INNER JOIN sessions s ON u.uid = s.uid WHERE s.sid = 'd5b1aa940eaeab37fe690ce9cc500396' AND u.status < 3 LIMIT 1 OFFSET 0
[1] => SELECT r.rid, r.name FROM role r INNER JOIN users_roles ur ON ur.rid = r.rid WHERE ur.uid = 1
[2] => SELECT * FROM access WHERE status = 1 AND type = 'host' AND LOWER('127.0.0.1') LIKE LOWER(mask)
[3] => SELECT * FROM access WHERE status = 0 AND type = 'host' AND LOWER('127.0.0.1') LIKE LOWER(mask)
[4] => SELECT data, created, headers, expire FROM cache WHERE cid = 'variables'
[5] => SELECT COUNT(pid) FROM url_alias
[6] => SELECT name, filename, throttle, bootstrap FROM system WHERE type = 'module' AND status = 1
[7] => UPDATE sessions SET uid = 1, cache = 0, hostname = '127.0.0.1', session = 'messages|a:0:{}', timestamp = 1124545039 WHERE sid = 'd5b1aa940eaeab37fe690ce9cc500396'
[8] => INSERT INTO watchdog (uid, type, message, severity, link, location, referer, hostname, timestamp) VALUES (1, 'php', 'pg_query(): Query failed: ERROR: column "cache" of relation "sessions" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71.', 2, '', '/dt/d/update.php?op=update', 'http://localhost/dt/d/update.php', '127.0.0.1', 1124545039)
)
Fatal error: ERROR: column "referer" of relation "watchdog" does not exist query: INSERT INTO watchdog (uid, type, message, severity, link, location, referer, hostname, timestamp) VALUES (1, 'php', 'pg_query(): Query failed: ERROR: column "cache" of relation "sessions" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71.', 2, '', '/dt/d/update.php?op=update', 'http://localhost/dt/d/update.php', '127.0.0.1', 1124545039) in /var/www/dt/d/includes/database.pgsql.inc on line 94
Warning: Unknown(): A session is active. You cannot change the session module's ini settings at this time. in Unknown on line 0
As we can see the problem is that there is no cache column is session table - that's update_130. Also there's no column 'referer' in watchdog table - that's update_142. So I've commented out update_130 and 142, added the columns by hand in the db and started again from the begining.
After that I got another error:
warning: pg_query(): Query failed: ERROR: column "access" of relation "users" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71.
Bad query:
Array
(
[0] => SELECT u.*, s.* FROM users u INNER JOIN sessions s ON u.uid = s.uid WHERE s.sid = 'd5b1aa940eaeab37fe690ce9cc500396' AND u.status < 3 LIMIT 1 OFFSET 0
[1] => SELECT r.rid, r.name FROM role r INNER JOIN users_roles ur ON ur.rid = r.rid WHERE ur.uid = 1
[2] => SELECT * FROM access WHERE status = 1 AND type = 'host' AND LOWER('127.0.0.1') LIKE LOWER(mask)
[3] => SELECT * FROM access WHERE status = 0 AND type = 'host' AND LOWER('127.0.0.1') LIKE LOWER(mask)
[4] => SELECT data, created, headers, expire FROM cache WHERE cid = 'variables'
[5] => SELECT COUNT(pid) FROM url_alias
[6] => SELECT name, filename, throttle, bootstrap FROM system WHERE type = 'module' AND status = 1
[7] => UPDATE sessions SET uid = 1, cache = 0, hostname = '127.0.0.1', session = 'messages|a:0:{}', timestamp = 1124547147 WHERE sid = 'd5b1aa940eaeab37fe690ce9cc500396'
[8] => UPDATE users SET access = 1124547147 WHERE uid = 1
[9] => INSERT INTO watchdog (uid, type, message, severity, link, location, referer, hostname, timestamp) VALUES (1, 'php', 'pg_query(): Query failed: ERROR: column "access" of relation "users" does not exist in /var/www/dt/d/includes/database.pgsql.inc on line 71.', 2, '', '/dt/d/update.php?op=update', 'http://localhost/dt/d/update.php', '127.0.0.1', 1124547147)
)
user error:
query: UPDATE users SET access = 1124547147 WHERE uid = 1 in /var/www/dt/d/includes/database.pgsql.inc on line 94.
That's update_136. So, disable update_136 and do it by hand and start again.
After all that, when accessing update.php?op=update I noticed no errors. After pressing Update all updates were applied without any errors. But when accessed main page I got a screen as on attached screenshot.
#5
And the mentioned patch.
Critical because otherwise upgrades (at least for postgres) are impossibile.
#6
I need people to test mysql updates, as I changed mysql part too.
#7
A diffrent patch. Only adds 3 functions for adding and changing table columns, does not modifies any old updates.
There are still some issues to be solved, for example:
- when changing a column in mysql the PRIMARY KEY attribute is retained. In Postgres, due to the
nature of changing process (i.e. dropping the column) this attribute is dropped. How to workaround it?
Need to check other attributes too (e.g. NOT NULL, index etc).
- add more attributes? Like primary key, key (mysql: synonim dla unique), unique, index, auto increment (mysql: must be indexed and must have a default) ?
- in CHANGE when setting NOT NOLL, and DEFAULT is set, update the table to default WHERE column IS NULL ?
#8
Updated patch based on http://drupal.org/node/7582
Following functions are defined:
db_add_key(&$ret, $table, $column);db_drop_column(&$ret, $table, $column);
db_add_primary_key(&$ret, $table, $column);
db_drop_primary_key(&$ret, $table);
db_add_column(&$ret, $table, $column, $type, $attributes = array());
db_change_column(&$ret, $table, $column, $column_new, $type, $attributes = array());
drupal_pg_version();
db_change_column() still need some work, the rest should work as documented.
#9
Another version: I've decided to stop supporting postgresql 7.2.
7.2 is still maintained, but first 7.2 release come out in 2002-02-04, last one (7.2.8) in 2005-05-09.
Next stable line, 7.3, came out in 2002-11-27, which is almost 3 years ago. That version introduced many new features, see http://www.postgresql.org/docs/8.0/interactive/release-7-3.html for details.
Currently latest stable version is 8.0.3 from 2005-05-09
This means DROP COLUMN and dropping primary keys as DROP CONSTRAINT works and there's no need for drupal_pg_version() nor db_drop_column().
It'd be good to ask on drupal.org if people are still using Postgresql 7.2. Would it be possibile to create a poll with questions "which database you are using?" and answers "postgres 7.2, postgres 7.3, ... , mysql 3.0.x, mysql 4.0.x, ..." ?
#10
Updated to current head.
#11
Attaching fixed update_() functions up to update_143(). Not at least it executes those functions without errors, I still need to check if they are 100% ok (but I believe they are for 90% ;))
Besides this patch you need to apply the one from #10
#12
update_() all execute without errors, but still not ideal.
Fixed database.pgsql a bit too.
You should apply this patch as first.
#13
I've removed dropping columns, I'm not dropping postgresql 7.2 support after all.
Apply this patch as second.
#14
Fixed some small bugs. Now both parts in 1 patch.
After update there's a problem with themes? Or blocks? I don't know... The screen after update looks like http://cvbge.org/after_update.png
Any idea what can be the reason?
#15
The screen I got was because of lack of xtemplate engine. The template 4.6 was using was a bluemarine, which uses xtemplate in 4.6 branch.
HEAD branch does not have xtemplate which causes problems. I had to fix system table by hand:
update system set description = 'themes/engines/phptemplate/phptemplate.engine', filename='themes/bluemarine/page.tpl.php' where name='bluemarine';update system set filename='themes/engines/phptemplate/phptemplate.engine', name='phptemplate' where name='xtemplate';
#16
I've just checked mysql part of the patch (me! mysql! horrible! ;) - done mysqldump of fresh 4.6 install, then upgraded to head with original and patched drupal, doing dump for each update. Then diff'ed the dump, which showed only 1 change in database schema:
- KEY `title` (`title`)in boxes table, which is intentional, because database.mysql does not have KEY on title in that table.
Thus I consider this patch ready to be committed.
BTW, the problem with xtemplate from previous comment occurs for mysql too.
#17
One more comment: I've tried not to make any changes to mysql part of update_() functions when creating the patch.
#18
Do we really need these helper functions? For me, the code becomes harder to read with those. When dealing with SQL queries, I'd like to know _exactly_ what's going on.
#19
Do we *need* them? No, but using them is better then not using, IMO.
For example current update_115():
<?php
function update_115() {
$ret = array();
if ($GLOBALS['db_type'] == 'mysql') {
$ret[] = update_sql("ALTER TABLE {watchdog} ADD severity tinyint(3) unsigned NOT NULL default '0'");
}
else if ($GLOBALS['db_type'] == 'pgsql') {
$ret[] = update_sql('ALTER TABLE {watchdog} ADD severity smallint');
$ret[] = update_sql('UPDATE {watchdog} SET severity = 0');
$ret[] = update_sql('ALTER TABLE {watchdog} ALTER COLUMN severity SET NOT NULL');
$ret[] = update_sql('ALTER TABLE {watchdog} ALTER COLUMN severity SET DEFAULT 0');
}
return $ret;
}
?>
could be written as
<?php
function update_115() {
$ret = array();
db_add_column($ret, 'watchdog', 'severity', 'tinyint(3)', array('unsigned' => TRUE, 'not null' => TRUE, 'default' => '0'));
return $ret;
}
?>
This would take care of all differences between postgres, mysql (or future databases, sqlite?)
Another, even more drastic example is from update_137(), where 1 line of mysql sql code must be changed to 5(6) lines of postgres code:
<?php
function update_137() {
$ret = array();
if ($GLOBALS['db_type'] == 'mysql') {
$ret[] = update_sql("ALTER TABLE {locales_source} CHANGE location location varchar(255) NOT NULL default ''");
}
elseif ($GLOBALS['db_type'] == 'pgsql') {
$ret[] = update_sql("ALTER TABLE {locales_source} RENAME location TO location_old");
$ret[] = update_sql("ALTER TABLE {locales_source} ADD location varchar(255)");
$ret[] = update_sql("UPDATE {locales_source} SET location = location_old");
$ret[] = update_sql("ALTER TABLE {locales_source} ALTER location SET NOT NULL");
$ret[] = update_sql("ALTER TABLE {locales_source} ALTER location SET DEFAULT ''");
$ret[] = update_sql("ALTER TABLE {locales_source} DROP location_old");
}
return $ret;
}
?>
could be wrote simply as
<?phpfunction update_137() {
$ret = array();
db_change_column($ret, 'locales_source', 'location', 'location', 'varchar(255)', array('not null' => TRUE, 'default' => ''));
return $ret;
}
?>
In case of mysql it is not a big problem IMO, because the functions execute almost identical statements. For example
<?phpfunction db_change_column(&$ret, $table, $column, $column_new, $type, $attributes = array()) {
?>
which has large body and documentation, is in fact very simple and for mysql it only executes
<?php$ret[] = update_sql("ALTER TABLE {". $table ."} CHANGE $column $column_new $type $unsigned $not_null $default");
?>
Having this automated means I won't make some silly typo nor any other mistake when writing that code. Probably I won't have to fix nor add to the original patch at all!
Of course, I'll have to check if the function can be used. It does not work for *all* cases. But still it is huge help for me (or anyone who writes mysql and postgres code).
If the functions do no have bugs then all their calls should not have bugs.
And the last idea: maybe it'd be better to move the functions to updates.{mysql,pgsql}.inc and include them depending on $db_type, like it is done with database.{mysql,pgsql}.inc? That would make updates.inc less cluttered and code in the functions much simpler (in mysql case at least)
#20
Here's implementation of the last idea (not tested, just an example).
I'd prefer to keep the functions as 1 function after all, they are very similar.
#21
I still disagree with your statement that it's easier to read:
<?phpALTER TABLE {watchdog} ADD severity tinyint(3) unsigned NOT NULL default '0'"
?>
is easier to read and faster to write than:
<?phpdb_add_column($ret, 'watchdog', 'severity', 'tinyint(3)', array('unsigned' => TRUE, 'not null' => TRUE, 'default' => '0'));
?>
It might make sense for db_add_column() and db_change_column() (because the PostgreSQL alternative is complex) but it doesn't make sense for the other helper functions IMO. Either way, the proposed API functions are a lot less transparant.
No point moving this to separate files. This might change as soon the update system improvements hit core. If we move forward with this, we'll go with derivates of #14. I don't think I'll commit it with all the helper functions.
#22
Well, if you add time to write
<?phpswitch ($GLOBALS['db_type']) {
?>
Besides, do you actually *need* to see the real SQL? I think there's only one correct way to add/drop index(key)/primary key. How using a function would break anything?
Anyway, if you still don't agree I see two possibile compromises:
db_add_column()anddb_change_column()and write separate code for other cases, as you proposedI'd prefer the second choise, but will live with the first one too.
#23
I've forgot, there's of course 3rd possibility - leave mysql alone and use db_* functions for postgres only.
#24
Sorry, I still don't agree.
#25
Changed the patch as you asked, added small guide on mysql2pgsql conversion.
The only changes to mysql part are:
- removed AFTER timestamp in update_130()
- removed creation of index in update_131(), it's not in database.{mysql,pgsql}
Still need to test the patch.
#26
Tested as in #16, same result.
#27
Postgresql bugfixes
#28
More bug, which would not happen if I could use db_ functions and didn't forgot a small change in two places ;)
#29
Is this correct?
<?php+ case 'pgsql':
+ $ret[] = update_sql("ALTER TABLE {forum} RENAME shadow TO shadow_old");
+ break;
?>
#30
Yes, it was removed in update_105() in mysql part only. I've decided to support postgres 7.2 after all, so I've renamed the column to _old to be consistent. Executing or not executing this update should not change much now, but can be usefull in future or if someone needs to have quick list of "old" columns.
#31
The patch no longer applies against HEAD. Sorry. Could you update it once more? Thanks Cvbge!
#32
Updated.
#33
Committed to HEAD. Thanks.
#34