extragear/multimedia/amarok/src rev 570617

[Andrew Turner]

Jeff,

Read my comment:
//No tables seem to exist (as doing a count(url) didn't even return
any number, even 0)

The actual test in isValid is for the return value of the queries
(which do a count(url)) to be empty. If the tables were empty, the
return value should be a QStringList of {"0"}, as far as I understand
SQL. "0" is not an empty string. So, even if all the tables are empty,
isValid should still be true. The only way for isValid to return false
is for every table to not exist, hence generating an empty QStringList
for every query.

So, I think my change is correct. But, feel free to respond if you
disagree with my reasoning.

Andrew



On 07/08/06, Jeff Mitchell <kde-dev at emailgoeshere.com> wrote:
> Hmm...
>
> I didn't make this change myself because I'm not sure it's entirely correct.
> This function will run if isValid() is not true.  isValid checks for any url
> in tags, statistics, podcastchannels and podcastepisodes, and any id from
> devices...if no value exists for any of those, it assumes the tables don't
> exist.
>
> However, could those tables exist, but for some reason be empty (nothing
> played yet so nothing in stats, no podcasts, no folders selected so tags
> empty)...I suppose there should always be a device, but is that true?
>
> Anyways, what I'm getting at is that this could maybe make us skip the upgrade
> code when it is needed, if a person with an older revision has empty tables
> for whatever reason.  The safer thing in this sense would be to drop the
> tables and then recreate them.
>
> Of course, that behavior led to the infamous "suddenly all my stats are gone
> and I have MySQL!" bug...except the fix for that really was in the MySQL
> initialization code, not here.  See the log message and diff for revision
> 559197.  In it I remarked that it didn't make sense to drop tables that
> seemed to be empty and then recreate them, leaving them empty; however if I
> am right about this and we can end up getting into a state where the tables
> skip the upgrade because they're empty, and we then write the new rev
> numbers, could we end up with worse behavior?
>
> I guess I'm suggesting that we do in fact drop the tables in this case (which
> shouldn't cause the stats bug again), and then recreate them (and then set
> the correct version numbers).  Or maybe do that for everything but stats, but
> always run the stats upgrade code?
>
> --Jeff
>
> On Monday 07 August 2006 08:47, Andrew Turner wrote:
> > SVN commit 570617 by aturner:
> >
> > After creating tables, set the version numbers. This prevents the
> > refactored DB code from wanting to update the tables. (This was due to the
> > DB update code being moved into the checkDatabase function, whereas the old
> > code was in the initialize function and would not run if the tables were
> > being created for the first time.)
> >
> > Well done to jefferai for spotting this.
> >
> >
> >  M  +16 -1     collectiondb.cpp
> >
> >
> > --- trunk/extragear/multimedia/amarok/src/collectiondb.cpp #570616:570617
> > @@ -4987,13 +4987,28 @@
> >      }
> >      else if ( !isValid() )
> >      {
> > -        warning() << "Tables seem to be empty; maybe they don't exist at
> > all." << endl; +        //No tables seem to exist (as doing a count(url)
> > didn't even return any number, even 0). +        warning() << "Tables seem
> > to not exist." << endl;
> >          warning() << "Attempting to create tables (this should be safe;
> > ignore any errors)..." << endl; createTables(false);
> >          createPersistentTables();
> >          createPodcastTables();
> >          createStatsTable();
> >          warning() << "Tables should now definitely exist. (Stop ignoring
> > errors)" << endl; +
> > +        //Since we have created the tables, we need to make sure the
> > version numbers are +        //set to the correct values. If this is not
> > done now, the database update code may +        //run, which could corrupt
> > things.
> > +        amaroK::config( "Collection Browser" )->writeEntry( "Database
> > Version", DATABASE_VERSION ); +        amaroK::config( "Collection Browser"
> > )->writeEntry( "Database Stats Version", DATABASE_STATS_VERSION ); +
> > amaroK::config( "Collection Browser" )->writeEntry( "Database Persistent
> > Tables Version", DATABASE_PERSISTENT_TABLES_VERSION ); +
> > amaroK::config( "Collection Browser" )->writeEntry( "Database Podcast
> > Tables Version", DATABASE_PODCAST_TABLES_VERSION );    amaroK::config(
> > "Collection Browser" )->writeEntry( "Database ATF Version",
> > DATABASE_ATF_VERSION ); +
> > +        setAdminValue( "Database Version", QString::number(
> > DATABASE_VERSION ) ); +        setAdminValue( "Database Stats Version",
> > QString::number( DATABASE_STATS_VERSION ) ); +        setAdminValue(
> > "Database Persistent Tables Version", QString::number(
> > DATABASE_PERSISTENT_TABLES_VERSION ) ); +        setAdminValue( "Database
> > Podcast Tables Version", QString::number( DATABASE_PODCAST_TABLES_VERSION )
> > ); +        setAdminValue( "Database ATF Version", QString::number(
> > DATABASE_ATF_VERSION ) ); }
> >      else {
> >          //updates for the Devices table go here
> _______________________________________________
> Amarok mailing list
> Amarok at kde.org
> https://mail.kde.org/mailman/listinfo/amarok
>