[PATCH 5/7] Re: Don't keep sqlite connections open

[Nicolas Sebrecht]

On Thu, May 05, 2011 at 03:59:27PM +0200, Sebastian Spaeth wrote:

<...>

>              if version < LocalStatusSQLiteFolder.cur_version:
>                  self.upgrade_db(version)
> -            self.connection.close()
> +
> +    def get_cursor(self):
> +        """Return a db (connection, cursor) that we can use
> +
> +        You are responsible for connection.commit() and
> +        connection.close() yourself. Connection close() happens
> +        automatically when the connection variable is destroyed
> +        though. According to sqlite docs, you need to commit() before
> +        the connection is closed or your changes will be lost!"""
> +        #get db connection which autocommits
> +        connection = sqlite.connect(self.filename, isolation_level=None)        
> +        cursor = connection.cursor()
> +        return connection, cursor
>  
>      def upgrade_db(self, from_ver):
>          """Upgrade the sqlite format from version 'from_ver' to current"""

This is a good improvement but...

> +                conn, cursor = self.get_cursor()
> +                with conn:

<...>

> +                        cursor.execute('INSERT INTO status (id,flags) VALUES (?,?)',
>                                          (uid,flags))
> +                    conn.commit()


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            cursor.execute('CREATE TABLE metadata (key VARCHAR(50) PRIMARY KEY, value VARCHAR(128))')
> +            cursor.execute("INSERT INTO metadata VALUES('db_version', '1')")
> +            cursor.execute('CREATE TABLE status (id INTEGER PRIMARY KEY, flags VARCHAR(50))')
> +            conn.commit()


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            cursor.execute('DELETE FROM status')
> +            conn.commit()


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            cursor.execute('SELECT id,flags from status')
> +            for row in cursor:
> +                flags = [x for x in row[1]]
> +                self.messagelist[row[0]] = {'uid': row[0], 'flags': flags}


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            while True:
> +                try:
> +                    cursor.execute('INSERT INTO status (id,flags) VALUES (?,?)',
> +                               (uid,flags))
> +                    conn.commit()
> +                except sqlite.OperationalError as e:
> +                    if e.args[0] != 'database is locked':
> +                        raise
> +                else: #success
> +                    break


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            while True:
> +                try:
> +                    cursor.execute('UPDATE status SET flags=? WHERE id=?',(flags,uid))
> +                    conn.commit()
> +                except sqlite.OperationalError as e:
> +                    if e.args[0] != 'database is locked':
> +                        raise
> +                else: #success
> +                    break


<...>

> +        conn, cursor = self.get_cursor()
> +        with conn:
> +            for uid in uidlist:
> +                del(self.messagelist[uid])
> +                while True:
> +                    try:
> +                        cursor.execute('DELETE FROM status WHERE id=?',
> +                               uid)
> +                        conn.commit()
> +                    except sqlite.OperationalError as e:
> +                        if e.args[0] != 'database is locked':
> +                            raise
> +                    else: #success
> +                        break

...we tend to have a lot of constructs like this

  conn, cursor = self.get_cursor()
  with conn:
    while True:
      try:

        <execute some SQL request and commit>

      except blablah:

        <raise on database lock>

      else:

        <cool, it worked>


it would be nice to factorize out. Not that this is wrong by design but
it will cause harder work to maintain with possible errors and bugs.

I think we need a wrapper (or more) proceeding the SQL requests with
generic data methods. Let's me try to categorize the different request
types:

  - read only: mostly SELECTs with no need to commit()

  - write (we need to commit()):

    * one statement: easy

    * multiple statements: easy using a list

    * statements including additional code to execute: pass both the
      request and a function to the wrapper

It would really help to delegate the entire read/write process to the
lower stage. Why? Think a bit more about the potential of the SQL
backend. With nice data drivers it would actually be SO easy to add
features like

  - statistics
  - advanced tracking system for logging IMAP/cache sessions
  - manage miscallenous exceptions
  - multiple data formats as backend (usefull for testing purpose ,-))
  - thread safe concurrent access by managing queues

I think the SQL topic is a very good opportunity for improving our data
driver design. In short, I would expect something like data.write(),
data.read(), (...).

-- 
Nicolas Sebrecht