DB Upgrade Queries
Geoffrey Sneddon
From discussion on #201[1], there was a large discussion about how to
run the upgrade (or whether to do so automatically at all). I firmly
think we should do the upgrade automagically. I also believe we would
best benefit from some generic way to run a specific upgrade script on
a db version change. The attached patch provides such a way to do so.
The upgrade.sql on #201 would be placed as system/schema/mysql/
upgrades/1377.sql.
However, with the current patch (which, I must add, is totally
untested), it doesn't actually run: the subclasses (MySQLConnection
and SQLiteConnection) redefine the upgrade() method (which, until now,
has done absolutely nothing). It should be safe to just remove these
methods.
[1]: http://trac.habariproject.org/habari/ticket/201
--
Geoffrey Sneddon
<http://gsnedders.com/>
Geoffrey Sneddon
Having done more work on it, we now have a full tested method for
upgrading, including programmatically upgrading things as well
(through DatabaseConnection::programmatic_upgrade( $new_version )).
The attached patch also fixes any breakages caused by r1377 (thereby
fixing half of #201). The only thing missing from this patch is an
empty "system/schema/sqlite/upgrade" folder (as a patch cannot express
that).
Michael C. Harris
So, without me trying to understand any of this, if this patch gets
accepted I can just svn up and everything will be magically fixed?
--
Michael C. Harris, School of CS&IT, RMIT University
http://twofishcreative.com/michael/blog
Geoffrey Sneddon
Yeah.
Michael C. Harris
> On 9 Mar 2008, at 22:13, Michael C. Harris wrote:
[snip]
> > So, without me trying to understand any of this, if this patch gets
> > accepted I can just svn up and everything will be magically fixed?
>
> Yeah.
Fantastic. Then, can I beg someone who has the power to commit it? I
know I'm not the only one with this issue at the moment. See BigJibby,
for example.
Owen Winkler
> On Sun, Mar 09, 2008 at 11:46:29PM +0000, Geoffrey Sneddon wrote:
>> On 9 Mar 2008, at 22:13, Michael C. Harris wrote:
>
> [snip]
>
>>> So, without me trying to understand any of this, if this patch gets
>>> accepted I can just svn up and everything will be magically fixed?
>> Yeah.
>
> Fantastic. Then, can I beg someone who has the power to commit it? I
> know I'm not the only one with this issue at the moment. See BigJibby,
> for example.
>
I have a couple of problems with the patch. As I told Geoffrey
yesterday, there seems to be little sense in committing code that says,
literally, "This database-specific code needs to be moved into the
schema-specific functions" without actually doing so.
This patch can be improved by calling a new function in the parent
(maybe parent::upgrade_sql()) from each schema's upgrade() function
instead of executing that switch.
I'm being picky about this because there is already a handful of places
that perform this switch, making it difficult to maintain database
engine additions or changes. If we were to add Postgres support, for
example, we'd need to locate each of these switches and add a Postgres
case. By depending on the schema classes, we can avoid that type of
maintenance by having a single switch. Unfortunately, this patch code
can't be retrofitted with that later -- it will be better to just add
that to the patch up front.
Also, it might be a better idea to rethink the override of the upgrade()
function. If we're going to call from the parent down, then we should
probably just rename DatabaseConnection's upgrade() to run_upgrade()
make the child's upgrade() function do the programmatic upgrades. Or
maybe, as Geoffrey originally suggested, we should use version-based
function names, ala upgrade_1450(). Better yet, store the engine value
in the DatabaseConnection descendant object as discovered in the one and
only switch.
In any case, the current patch revision isn't quite it yet, I think.
I'm not sure of the ramifications for users upgrading from 0.4 to 0.5 if
everyone testing uses this code to change their databases, but then we
switch to something else. I'm thinking that the work to get it in a
good place is minimal compared to the difficulty we might have due to
those changes later.
If you really can't wait, you should be able to run the SQL from the
patch to do the character set change, or just patch this into your own
Habari install, upgrade, and revert.
Owen
Michael C. Harris
> Michael C. Harris wrote:
> > On Sun, Mar 09, 2008 at 11:46:29PM +0000, Geoffrey Sneddon wrote:
> >> On 9 Mar 2008, at 22:13, Michael C. Harris wrote:
> >
> >>> So, without me trying to understand any of this, if this patch gets
> >>> accepted I can just svn up and everything will be magically fixed?
> >> Yeah.
> >
> > Fantastic. Then, can I beg someone who has the power to commit it? I
> > know I'm not the only one with this issue at the moment. See BigJibby,
> > for example.
[snip convincing reasoning not to apply the patch]
> If you really can't wait, you should be able to run the SQL from the
> patch to do the character set change, or just patch this into your own
> Habari install, upgrade, and revert.
Actually, I think this is important, and I don't really understand the
ins and outs of it, so what I wanted to do was to test that svn up'ing
will fix things, for normal punter. Nobody reads my blog anyway, so
being a bit broken doesn't stress me, but I want the issue to keep
moving.
Geoffrey Sneddon
On 10 Mar 2008, at 04:21, Owen Winkler wrote:
> Also, it might be a better idea to rethink the override of the
> upgrade()
> function. If we're going to call from the parent down, then we should
> probably just rename DatabaseConnection's upgrade() to run_upgrade()
> make the child's upgrade() function do the programmatic upgrades.
We then can't run the updates in order (assuming we still call
upgrade() to run the upgrade), as we'd have to run all the SQL files
before the programmatic upgrades (or the other way around) — or we
could introduce a huge amount of code there, which would seemingly
like it look more complex to add more switches for the future.
> Or maybe, as Geoffrey originally suggested, we should use version-
> based
> function names, ala upgrade_1450().
My intent of that was just to cut out the need to call the
programmatic upgrades method for every possible revision between the
original revision and the new revision: nothing more.