#71 fromDatabase and delColumn for sqlite

closed-accepted
Oleg Broytman
None
5
2006-12-28
2006-08-12
Arlo Belshee
No

This patch implements both fromDatabase and delColumn
for sqlite. I added sqlite to the appropriate
existing tests; it passes them.

fromDatabase is done by parsing the table creation
sql from the sqlite_master table. This will work with
any sqlite 3, and I think dates back into sqlite 2,
but I'm not sure.

delColumn is implemented by renaming the table,
creating a new table without the one column, then
copying all the data over. This is obviously not as
efficient as on DBs that support drop column
directly, but it works.

This implementation of dropColumn does deal correctly
with triggers, foreign keys, and indices that
reference the table whose column is being dropped. It
does not deal correctly with triggers & indices
attached directly to the table being altered.
Attached items will be lost. Extending the
implementation to support re-creating those indices &
triggers should be straightforward, but is beyond my
needs. You are welcome to do so.

Finally, this patch slightly improves the
implementation of addColumn for sqlite. It performs a
vacuum operation after each column add. See the
sqlite documentation for why this is a good idea.

Discussion

  • Oleg Broytman
    Oleg Broytman
    2006-08-15

    Logged In: YES
    user_id=4799

    Thank you very much! recreateTableWithoutColumn() is an
    excellent solution!

    I have some questions about the patch. Why have you added
    semicolons? I think they are not needed.

    Why VACUUM after addColumn()? It is more logical to do
    VACUUM after delColumn(), I suppose. And, BTW, "This command
    will fail if there is an active transaction." (from the
    SQLite documentation).

    Your patch deletes .delColumn(). But I have a number of
    programs that relies on a presence of the method (even if
    it's empty). I think you could just call
    recreateTableWithoutColumn() from delColumn() or raise a
    warning or an exception.

     
  • Oleg Broytman
    Oleg Broytman
    2006-08-15

    • assigned_to: nobody --> phd
     
  • Arlo Belshee
    Arlo Belshee
    2006-08-27

    Logged In: YES
    user_id=1027257

    Why vacuum after addColumn:

    According to SQLite's docs, alter table add column doesn't
    add the column to the table's main location in the file,
    but rather creates it at the end of the file and the DB
    then takes care of pretending that the column adjoins all
    the other columns.

    Unfortunately, older versions of SQLite don't do this
    pretending, so can't read a table that has had a column
    added. Vacuum re-creates the table as a single table,
    meaning that all versions can then read it.

    Delete column, being not supported directly by SQLite,
    doesn't do anything funny. Since my implementation is just
    to create a new table without the column being dropped,
    all the tables remain together without any weird extra
    space, and a vacuum is not needed.

    Why I added semicolons:

    No, they are not needed, if you are using pysqlite.
    However, they are required in the interactive SQLite
    interpreter. I added the semicolons so that I could
    quickly paste generated SQL statements into the
    interactive console while I was trying to get things
    working.

    Since the semicolons have no effect with pysqlite, I just
    left them in for the next time I wan't to do this. Feel
    free to take them out if you'd like.

    On the name of delColumn():

    Feel free to name it delColumn. I gave the longer and
    clear name to the low-level implementation primarily to
    highlight the performance implications involved. The
    public-facing objects still expose delColumn; that is
    translated to a call to recreateTableWithoutColumn() if
    the sqliteconnection is being used.

    So, external programs use it as deleteColumn(), but my
    patch is easier to understand and the performance
    implications are clearer.

    Mostly, I was hoping that that would speed patch
    approval. ;) Feel free to name the function as you will.

    On transactions:

    Yeah, you can't vacuum in transactions. However, sqlite's
    support for transactions is already fairly problematic. I
    find I can't use transactions with SQLite on any real
    problems anyway, so I don't worry about it. When I need
    transactions, then I generally need a more full-featured
    DBMS than SQLite.

    If you have a different opinion on this, you could remove
    the call to vacuum in addColumn(). You are trading off
    read speed & the ability for older versions of sqlite to
    read such a modified table. Yer call.

     
  • Arlo Belshee
    Arlo Belshee
    2006-08-27

    Logged In: YES
    user_id=1027257

    I've slightly update my previous patch. Feel free to use
    whichever version you'd like - I just want to get these
    new functions into the main code line as quickly as
    possible. I need them in order to support migrations in
    TurboGears.

    This update does two things:
    1. svn:ignore *.pyc in a bunch of directories.
    2. Add sqlite to the -transactions list in the
    supportsMatrix.

    Why:

    1: I find it easier to see what needs to get added to svn
    if I don't have clutter on the various "modified files"
    lists. Thus, I make sure to always tell svn how to handle
    each file: ignore it or commit it. I find it reduces the
    chances of bad checkins.

    2: sqlite fails several tests that require transactions.
    It really doesn't handle them well. This may be dues to my
    adding vacuuming to addColumn, or it may be new failures
    because I added sqlite to the list of DBs that support
    fromDatabase() and delColumn. In any case, I found this to
    be the fastest reasonable way to express what SQLite can
    do. Feel free to modify if you want to go another way.

     
  • Oleg Broytman
    Oleg Broytman
    2006-09-01

    Logged In: YES
    user_id=4799

    The second patch is invalid because patch program doesn't
    understand svn ignore lists. This must be done manually if
    at all. I just run a script

    find . -name '*.py[co]' | xargs rm

    before commit.

    And what are the problems with SQLIte and transactions? All
    tests are now passed. Can you write a test script that
    demonstrates the problem(s)?

     
  • Oleg Broytman
    Oleg Broytman
    2006-12-28

    Logged In: YES
    user_id=4799
    Originator: NO

    A version extended by Glenn and me has been committed in the revision 2154. Thank you for the job!

     
  • Oleg Broytman
    Oleg Broytman
    2006-12-28

    • status: open --> closed-accepted