I think if the migration can't be done, it would be better an error or exception than a warning. If the changes in the DB can't be migrated, it's better for the developer to know the problem and find an alternate way.
I'll try and send you a patch, and then we can decide if it's better to give a warning or an exception. If it's not possible to drop columns in Sqlite, what about creating a new temp table, inserting data from old table into the new one, removing the old table and renaming the temp table into the old table's name? That would allow this kind of migrations in Sqlite. I could try this too, and suggest a patch, if you agree this is s a correct approach. Greets. On 29 ago, 07:12, mdipierro <[email protected]> wrote: > I agree .table should not we rewritten all the time. If it does so it > is a bug, I should double check, and I will take a patch to fix it. > > I am not sure about dropping columns and SQLite. The problem only > appears when a user tries to re-define a column that was dropped. > Perhaps a warning instead of an exception? I would take a patch to > issue a logging.warn. > > On Aug 28, 12:32 pm, Álvaro J. Iradier <[email protected]> wrote: > > > > > Thanks Massimo, that answers my first question. > > > But what about the .tables being written on every request, with the > > old fields? > > > if query: > > ... > > if key in sql_fields: > > sql_fields_old[key] = sql_fields[key] > > else: > > del sql_fields_old[key] > > tfile = open(self._dbt, 'w') > > portalocker.lock(tfile, portalocker.LOCK_EX) > > cPickle.dump(sql_fields_old, tfile) > > portalocker.unlock(tfile) > > tfile.close() > > > For sqlite, query == None, so sql_fields_old is unchanged, and the > > original sql_fields_old is cpickled to the .table file. So, the change > > is detected on every request, and the .table file is rewritten all the > > time. > > > I think the correct fix should be, if sqlite doesn't support dropping > > a column, then an exception should be raised, so the developer knows > > the migration has to be done manually. What do you think? > > > Thanks very much. > > > On 27 ago, 23:31, mdipierro <[email protected]> wrote: > > > > > so, for SQL databases, if a column is removed or dropped, it's not > > > > migrated for sqlite. Is there a reason for this? > > > > sqlite does not support drop columns. > > > > On Aug 27, 2:03 pm, Álvaro J. Iradier <[email protected]> > > > wrote: > > > > > Hi everyone, this is a long one > > > > > today I noticed one of my xxxx_settings.table file was written > > > > everytime. The table definition was changed a long ago, from the old > > > > chema: > > > > > CREATE TABLE settings( > > > > id INTEGER PRIMARY KEY AUTOINCREMENT, > > > > key CHAR(512), > > > > value CHAR(512) > > > > ); > > > > > to the new: > > > > > CREATE TABLE settings( > > > > id INTEGER PRIMARY KEY AUTOINCREMENT, > > > > key CHAR(512) NOT NULL UNIQUE, > > > > value CHAR(512) > > > > ); > > > > > the change is, key column was made NOT NULL UNIQUE. > > > > > So first question I noticed. In the migrate procedure, I noticed the > > > > following code: > > > > for key in keys: > > > > if not key in sql_fields_old: > > > > query = ['ALTER TABLE %s ADD %s %s;' % \ > > > > (self._tablename, key, > > > > sql_fields_aux[key].replace(', ', new_add))] > > > > elif self._db._dbname == 'sqlite': > > > > query = None > > > > ... > > > > > so, for SQL databases, if a column is removed or dropped, it's not > > > > migrated for sqlite. Is there a reason for this? > > > > > Then I tried commenting the "if self._db._dbname == 'sqlite':" line, > > > > and I got the following SQL error: > > > > > OperationalError: Cannot add a UNIQUE column > > > > > which makes sense, it's not trivial to add a UNIQUE column to an > > > > existing database... > > > > > But what makes me worry is, at the end of the _migrate method, I find > > > > this: > > > > > if query: > > > > ... > > > > if key in sql_fields: > > > > sql_fields_old[key] = sql_fields[key] > > > > else: > > > > del sql_fields_old[key] > > > > tfile = open(self._dbt, 'w') > > > > portalocker.lock(tfile, portalocker.LOCK_EX) > > > > print "Here2:", self._dbt, sql_fields, sql_fields_old > > > > cPickle.dump(sql_fields_old, tfile) > > > > portalocker.unlock(tfile) > > > > tfile.close() > > > > > First, sql_fields_old is updated with the migrated value ONLY if query > > > > is not None. For sqlite it's None for changed columns. So, later, > > > > cPickle dumps the value of sql_fields_old, so the file > > > > xxxx_settings.table is written with the same old values, as this field > > > > was not migrated. > > > > > So, for every request, web2py detects a migration is needed, but it > > > > does nothing but writing the .table file with the old values. > > > > > I think the correct behaviour should be throwing an error if migration > > > > can't be done. > > > > > Can you suggest a fix for this? > > > > > Thanks very much. > > > > > -- > > > > Álvaro J. Iradier Muro > > > > Departamento de Desarrollo > > > > [email protected]
