postgres support
hunmonk - July 15, 2007 - 02:26
| Project: | CVS integration |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | normal |
| Assigned: | hunmonk |
| Status: | closed |
Description
attached patch adds postgres support to the module. it's still a bit of a work in progress. some notes:
- 'user' is a reserved keyword in postgres, so i needed to fix that in the mysql upgrade, and come up with another name. 't_user' was all i could think of, as in 'table_user' -- but i'm guessing we'll need to pick something better. :)
- mysql has an index on the 'branch' column in the cvs_files table, which i believe only indexes the first ten characters. i can't figure out a way to generate the same kind of index in postgres. at the moment it's just indexing the entire field.
- tested the table installs on postgres, and they work w/o errors.
- added an uninstall function while i was at it, and it also tested fine on postgres.
- the user -> t_user column change in the code is totally untested, although i'm feeling relatively confident that i got them all.
- cvs_update_7() is untested.
| Attachment | Size |
|---|---|
| cvs_postgres.patch | 9.09 KB |
#1
i don't think you need to bother capturing the results of db_query() in cvs_install().
bunch of tabs in cvs_update_7().
can't you quote 'user' to make it pgsql safe? username would be better than t_user...
#2
@drewish: in that case, please set the status appropriately. ;)
@hunmonk: very cool, thanks for the start. haven't tested yet, but visual inspection of the patch revealed 1 other minor readability personal-preference thing, aside from the issues that drewish mentioned. in cvs_uninstall(), for the huge block of variables to delete, i'd rather just populate an array of variable names and then foreach() it, much like we do with the tables to drop. so, instead of:
variable_del('cvs_email_address');variable_del('cvs_use_file');
variable_del('cvs_default_repo');
...
I'd rather see:
$variables = array('cvs_email_address',
'cvs_use_file',
'cvs_default_repo',
...
);
foreach ($variables as $variable) {
variable_del($variable);
}
That's just a personal preference, but I find it's nicer to just add something to an array than copy-paste a whole other variable_del() line. But, certainly all those tabs gotta go, if nothing else. ;)
Will test/review more closely once there's a new patch.
Thanks!
-Derek
#3
drupal=> create table foo ("user" text);
CREATE TABLE
so, yes we can, but i really don't like using reserved words -- seems like a prescription for future trouble. i'm ok with username, but have a question: why are we storing that in the first place? can't we just get that from the users table?
thoughts, anyone?
will fix the other issues shortly.
#4
attached should fix all of the above concerns. i decided to change user -> username instead of overriding the reserved word for postgres -- i really think this is the better long-term solution.
#5
{cvs_messages}.user is the account name of the CVS user who committed a given change. In general, this is the same as the drupal username, but not always. Furthermore, it's entirely possible that someone commits to a repo who doesn't have a drupal account at all, in which case the uid field is 0, but the user field is filled in.
Therefore, if we're going to rename the field anyway, maybe it should be called "cvs_user" to make it clear this is the username of the CVS account. While we're at it, perhaps we should rename {cvs_accounts}.name to also be "cvs_user" to match.
Thanks for the array-driven implementation for deleting variables in cvs_uninstall(). However, variable_del() does some other stuff, besides *just* removing the entry from the {variables} table. It also clears the entire 'variables' entry in the cache, and unsets the $conf entry for the particular variable. Not sure we need to do about clearing the $conf entry, since we're going to just reload the page after we hit the uninstall hook, so we'll bootstrap again and make a new $conf array. However, if you put the manual pruning of {variables} *before* any of the calls to variable_del() (like you had originally), then the cache will be appropriately cleared by the time we're rebuilding $conf.
Thanks for working on this!
-Derek
p.s. If you care about pgsql support for this module, I encourage you to take a look at http://drupal.org/node/136866.
#6
ok, we should be close now:
again, i'm pretty sure i got all the renames, but we really should test this in the real world a bit. :)
#7
visual inspection reveals no problems. updating status accordingly. ;) agreed, this requires a more careful review and testing before we can commit, but it's certainly close.
#8
missed renaming the primary key for {cvs_accounts}. attached corrects.
#9
A) does
UNIQUE (name)automatically create an index in pgsql?B) tabs in cvs_uninstall()
otherwise, i tested cvs_update_7() on MySQL and it's working fine. altering the field for the primary key works, and the primary key is then on cvs_user, which is great. a clean install also works, once i patched the d.o testing profile to use the new field names. ;)
Attached patch includes the (trivial) changes for the test profile, and fixes the tabs in cvs_uninstall(). So, if you're happy with (A), I think this is RTBC.
#10
yep.
committed to HEAD. won't backport this, since we've decided that postgres support will start w/ 5.x
NOTE: this is only a first step, postgres still is not fully supported for the module! the xcvs scripts are still mysql-specific. we'll be addressing that by having xcvs bootstrap the database.
#11
so it turns out that the generate-cvs-passwd.php script wasn't updated for this change. nnewton showed up in IRC and helped up figure out what the problem is.
#12
i'm confused. i don't see that file as part of this module -- are you thinking of another module perhaps? if so, please file an issue there and mark this fixed again.
#13
whoop. disregard that attachment -- it was in error...
#14
yeah the script isn't in CVS, but this change affected it so i made a note on the issue. i'd forgotten that dww wasn't going to be back around to take care of it. i looks like nnewton got it fixed though.
#15