Mailing List Archive: MythTV: Dev

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham

Index | Next | Previous | View Threaded

nigel at ind

Feb 11, 2008, 2:25 AM

Post #1 of 6 (638 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham

> Log: Add code to backup the database before any schema upgrade.

Just tried Mac frontend and was surprised by a
DBUtil ... mysqldump error (in fact. I'm amazed
that my Mac had mysqldump in its path :-)

2008-02-11 16:40:32.512 SG(Default) Error: FindNextDirMostFree: '/
myth/tv' does not exist!
2008-02-11 16:40:32.513 Backing up database to file: /myth/tv/
mythconverg-1208-20080211164032.sql.gz
sh: line 1: /myth/tv/mythconverg-1208-20080211164032.sql.gz: No such
file or directory
mysqldump: Got errno 32 on write
2008-02-11 16:40:32.720 DBUtil Error: Error backing up database:
mysqldump --defaults-extra-file='/tmp/mythtv_db_backup_conf_nkYIFL' --
host='149.135.128.77' --user='mythtv' --add-drop-table --add-locks --
allow-keywords --complete-insert --extended-insert --lock-tables --no-
create-db --quick 'mythconverg' | /usr/bin/gzip > '/myth/tv/
mythconverg-1208-20080211164032.sql.gz' 2>/dev/null (1)
2008-02-11 16:40:32.723 Unable to backup your database. If you have
not already created a backup, you may want to exit before the
database upgrade and backup your database.

1) GetBackupDirectory() should check directory exists?

2) Calling BackupDB() before PromptForSchemaUpgrade()
means backup is always attempted if the schemas differ,
but has the disadvantage of needlessly doing a backup
(e.g. user selects "Use Existing Schema" or Exit,
or DB schema is newer than the connecting client?)

It might be better to either:

A) Pass a flag into PromptForSchemaUpgrade()
if the caller can attempt a backup, and replace the
"You really should have a backup" buttons with a
"Backup, then upgrade" button and exit type, or

B) Pass a flag into PromptForSchemaUpgrade()
if a backup was done and was successful, so the
"You really should have a backup" buttons are not shown.

C) B) plus a check to disable BackupDB()
if the schema is newer than expected.

--
Nigel Pearson, nigel [at] ind|"People say I'm strange,
Telstra Net. Eng., Sydney, Australia | does it make me a stranger?
Office: 9202 3900 Fax: 9261 3912 | My best friend was born...
Mobile: 0408 664435 Home: 9792 6998 | in a manger" -DC Talk
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

cpinkham at bc2va

Feb 11, 2008, 8:41 AM

Post #2 of 6 (583 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham [In reply to]

* On Mon Feb 11, 2008 at 09:25:46PM +1100, Nigel Pearson wrote:
> > Log: Add code to backup the database before any schema upgrade.

> Just tried Mac frontend and was surprised by a
> DBUtil ... mysqldump error (in fact. I'm amazed
> that my Mac had mysqldump in its path :-)

I thought about Win32, but unfortunately didn't think about Mac until
an hour after I committed.

> 2008-02-11 16:40:32.512 SG(Default) Error: FindNextDirMostFree: '/
> myth/tv' does not exist!
> 2008-02-11 16:40:32.513 Backing up database to file: /myth/tv/
> mythconverg-1208-20080211164032.sql.gz
> sh: line 1: /myth/tv/mythconverg-1208-20080211164032.sql.gz: No such
> file or directory
> mysqldump: Got errno 32 on write
> 2008-02-11 16:40:32.720 DBUtil Error: Error backing up database:
> mysqldump --defaults-extra-file='/tmp/mythtv_db_backup_conf_nkYIFL' --
> host='149.135.128.77' --user='mythtv' --add-drop-table --add-locks --
> allow-keywords --complete-insert --extended-insert --lock-tables --no-
> create-db --quick 'mythconverg' | /usr/bin/gzip > '/myth/tv/
> mythconverg-1208-20080211164032.sql.gz' 2>/dev/null (1)
> 2008-02-11 16:40:32.723 Unable to backup your database. If you have
> not already created a backup, you may want to exit before the
> database upgrade and backup your database.
>
>
> 1) GetBackupDirectory() should check directory exists?

Saw your commit, thanks.

> 2) Calling BackupDB() before PromptForSchemaUpgrade()
> means backup is always attempted if the schemas differ,
> but has the disadvantage of needlessly doing a backup
> (e.g. user selects "Use Existing Schema" or Exit,
> or DB schema is newer than the connecting client?)

I think that in these cases, we want to make sure there was a
backup. If the user is running a program that expects a newer or
older DB version than the current one, we have no guarantee what
corruption may or may not occur. Having a backup done before the
user does this sounds like a good idea. The backups don't take
that long to run, mine only took 19 seconds with a remote DB. I
think sphery said his were taking about 29. I can see adding a
hidden setting to disable DB backups for dev machines, but if
someone is running a different schema version than their binaries
expect, a backup should almost be a requirement.

With the other commit that I made after this backup one, by default
you can't run mythbackend, mythtv-setup, or mythfrontend on a DB
that has a newer schema than what the program expects. The only
way around this is by turning on the pre-existing hidden setting to
enable expert mode. Maybe the solution is to not run backups if expert
mode is enabled or perhaps do not run if expert is enabled and
the schema version is newer than expected.

We could just call PromptForSchemaUpgrade() first and check the
result, and then only run the backup if the user selected to
run anyway or to upgrade the schema.

int upgradeCheck =
gContext->PromptForSchemaUpgrade(dbver, currentDatabaseVersion);

if (((upgradeCheck == MYTH_SCHEMA_USE_EXISTING) ||
(upgradeCheck == MYTH_SCHEMA_UPGRADE)) &&
((gContext->GetNumSetting("DBSchemaAutoUpgrade") != -1) ||
(CompareTVDatabaseSchemaVersion() < 0)) &&
(gContext->GetNumSetting("DBSchemaBackupBeforeUpgrade", 1)))
{
if (!dbutil.BackupDB())
VERBOSE(VB_IMPORTANT, "Unable to backup your database. If you have "
"not already created a backup, you may want to exit before "
"the database upgrade and backup your database.");
}

switch (upgradeCheck)
{
case MYTH_SCHEMA_USE_EXISTING: return true; // Don't upgrade
case MYTH_SCHEMA_ERROR:
case MYTH_SCHEMA_EXIT: return false;
case MYTH_SCHEMA_UPGRADE: break;
}

That is untested, but should only backup the DB if (we are going to upgrade
it or we're running a different binary schema version than DB schema version)
AND (we're not in expert mode or we're running an older schema with newer
binary). You could also set DBSchemaBackupBeforeUpgrade to 0 to prevent
DB backups on a dev system.

Does that handle what you're looking for?

--
Chris
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

nigel at ind

Feb 11, 2008, 5:32 PM

Post #3 of 6 (598 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham [In reply to]

>> needlessly doing a backup
>> (e.g. user selects "Use Existing Schema" or Exit,
>> or DB schema is newer than the connecting client?)
>
> I think that in these cases, we want to make sure there was a
> backup. If the user is running a program that expects a newer or
> older DB version than the current one, we have no guarantee what
> corruption may or may not occur.

Very true. And I was amazed how fast a Mac OS X
remote backup actually was. Four seconds at home
(26MB dump from MySQL 4.0.22, 5.1MB gzipped),
less than one second on my work test rig!

Doing an un-needed backup if the user selects Exit
isn't really a problem - its just disk space.
It is more the confusing message in the 2nd popup.

I'm thinking that the caller of PromptForSchemaUpgrade()
should do:

1) PromptForSchemaUpgrade(v1, v2)
if the caller hasn't attempted a backup,
and the existing popup messages are used

2) PromptForSchemaUpgrade(v1, v2, "failed")
if the backup failed,
add "Unable to backup your database ..."
in the first popup, and remove the second

3) (optional)
PromptForSchemaUpgrade(v1, v2, backupPathname)
if the backup succeeded in that pathname,
which would change the second popup to

"If your system becomes unstable,
a database backup is in /..."

[OK]

P.S. I just realised that, if the database is empty
(i.e. dvber == ""), then a backup is also done.

--
Nigel Pearson, nigel [at] ind|"People say I'm strange,
Telstra Net. Eng., Sydney, Australia | does it make me a stranger?
Office: 9202 3900 Fax: 9261 3912 | My best friend was born...
Mobile: 0408 664435 Home: 9792 6998 | in a manger" -DC Talk
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

cpinkham at bc2va

Feb 11, 2008, 9:04 PM

Post #4 of 6 (587 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham [In reply to]

* On Tue Feb 12, 2008 at 12:32:59PM +1100, Nigel Pearson wrote:
> I'm thinking that the caller of PromptForSchemaUpgrade()
> should do:

I think I handled all your cases in my [15932] commit. Let me know if
you see anything I left out.

> P.S. I just realised that, if the database is empty
> (i.e. dvber == ""), then a backup is also done.

Took care of this also, , but had second thoughts and reverted it.
It's not that much space to backup an empty DB, so it's not a big issue that
we backup an empty DB once. I think it is better to leave this backup in
for the case where the settings table is modified or corrupt in some way.

--
Chris
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

nigel at ind

Feb 11, 2008, 9:42 PM

Post #5 of 6 (578 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham [In reply to]

>> I'm thinking that the caller of PromptForSchemaUpgrade()
>> should do:
>
> I think I handled all your cases in my [15932] commit.

Wow. That was fast. Cool!

(This is just like programming by remote control.
I hacked together a spec, went to the Coffee shop,
and the new code was checked in when I got back :-)

> Let me know if you see anything I left out.

I will test thoroughly tonight.

If I was being pedantic, maybe change
the first message when the backup fails:
(dbcheck.cpp, line 511)

VERBOSE(VB_IMPORTANT, "Unable to backup your database.

to something more tech-related. e.g.

VERBOSE(VB_FILE, QString("dbutil.BackupDB() to '%s' failed")
.arg(backupResult));

because the user-oriented/translated message
is now in PromptForSchemaUpgrade().

But that requires changes in DoBackup().

Should we apply to 0.21 branch? I'm thinking
it may save a few percent of user confusion.

--
Nigel Pearson, nigel [at] ind|"Look at this!
Telstra Net. Eng., Sydney, Australia | Do you think I put this in
Office: 9202 3900 Fax: 9261 3912 | to get better reception?"
Mobile: 0408 664435 Home: 9792 6998 | Batty - Fern Gully
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

cpinkham at bc2va

Feb 11, 2008, 10:55 PM

Post #6 of 6 (596 views)
Permalink

Re: mythtv commit: r15901 - in trunk/mythtv/libs by cpinkham [In reply to]

* On Tue Feb 12, 2008 at 04:42:27PM +1100, Nigel Pearson wrote:
> >> I'm thinking that the caller of PromptForSchemaUpgrade()
> >> should do:
> >
> > I think I handled all your cases in my [15932] commit.
>
> Wow. That was fast. Cool!
>
> (This is just like programming by remote control.
> I hacked together a spec, went to the Coffee shop,
> and the new code was checked in when I got back :-)

Seemed like hours on this side of the globe. :) It was a
race, I was trying to get it in before the branch.

> I will test thoroughly tonight.

Thanks.

> If I was being pedantic, maybe change
> the first message when the backup fails:
> (dbcheck.cpp, line 511)

> VERBOSE(VB_FILE, QString("dbutil.BackupDB() to '%s' failed")
> .arg(backupResult));

After looking at this, I totally ripped this VERBOSE out.
With the VERBOSE calls in DBUtil and PromptForSchemaUpgrade(),
we have enough logging already.

> Should we apply to 0.21 branch? I'm thinking
> it may save a few percent of user confusion.

All of these changes are in both trunk and 0.21-fixes.

--
Chris
_______________________________________________
mythtv-dev mailing list
mythtv-dev [at] mythtv
http://mythtv.org/cgi-bin/mailman/listinfo/mythtv-dev

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads