Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DatabasePool can't read from existing non-WAL database #102

Closed
blkbam opened this issue Aug 16, 2016 · 19 comments
Closed

DatabasePool can't read from existing non-WAL database #102

blkbam opened this issue Aug 16, 2016 · 19 comments
Labels

Comments

@blkbam
Copy link

blkbam commented Aug 16, 2016

I ran into the same issue as #40 and did a little digging. I appears that a DatabasePool connection fails if no SerializedDatabase exists for the connection yet. I wrote a sample which illustrates this below.

`
import UIKit
import GRDB

class ViewController: UIViewController
{

internal static var connectionPool = [String:DatabasePool]()

override func viewDidLoad() {
    super.viewDidLoad()
    doStuff()
}

func doStuff()
{
    //a call to this db creates the .db-shm and .db-wal files
    createTable("WillNotFail")
    getAllTables("WillNotFail")

    //this call does not and throws an exception
    getAllTables("WillFail")
}

internal func getPooledConnection(name:String) -> DatabasePool?
{
    do
    {
        if ViewController.connectionPool[name] == nil
        {
            let rootLibDirectory = NSSearchPathForDirectoriesInDomains( .LibraryDirectory, .UserDomainMask, true ).first as String!
            print(rootLibDirectory)

            ViewController.connectionPool[name] = try DatabasePool( path: "\(rootLibDirectory)/\(name).db" )
        }
    }
    catch
    {
        print("\(error)")
        return nil
    }

    return ViewController.connectionPool[name]
}

internal func createTable(name: String)
{
    guard let pool = getPooledConnection(name)
        else
    {
        return
    }

    do
    {
        try pool.write
        { db in
            try db.execute("CREATE TABLE IF NOT EXISTS SampleTable (col1 varchar)")
        }
    }
    catch
    {
        print(error)
    }

    return
}

internal func getAllTables(name: String)
{
    guard let pool = getPooledConnection(name)
    else
    {
        return
    }

    pool.read
    { db in

        let results = Row.fetchAll( db, "SELECT name FROM sqlite_master WHERE type = 'table' ORDER BY name")

        for result in results
        {
            print( result.value(atIndex: 0) )
        }
    }

    return
}

}

`

@groue
Copy link
Owner

groue commented Aug 16, 2016

Hello @blkbam. If this is the same trouble than #40, then the answer is the same: don't create several instances of DatabasePool that connect to a given database file.

Review your application architecture. Don't create DatabasePool in temporary objects like UIViewController, since this implies that you can't guarantee that there is never more than one DatabasePool connected to a single database file.

@groue groue closed this as completed Aug 16, 2016
@groue groue added the support label Aug 16, 2016
@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

This is the same error however I'm not sure you are understanding the problem. There is only one connection per file.

In this sample the connection is stored in a dictionary by database name and is retrieved each time. Running through the sample you will see the issue. This happens the very first time the database is accessed. I used two connections to illustrate.

Running through the sample in #40 as well just one time you get the same issue. Opening a DatabasePool connection and reading from it simply doesn't work unless you have written to the database prior to the read. This isn't a concurrency problem.

@groue
Copy link
Owner

groue commented Aug 16, 2016

I admit that I don't happily jump in dozens of lines of pasted code when there is no clear declaration of any problem, and an explicit link to an already solved issue. Tagging answers with a 😕 does not help.

Besides, what do you want to read from a empty database? Isn't it a sure condition for failure? What do you expect?

@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

How can an application know that the database is empty unless it reads from it first? Every database has sqlite_master in it so even in this sample it should not fail.

With applications that dynamically create databases such as mine I can't blindly write to the database without ensuring tables exist.

@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

#40 was not solved, the previous poster used a different object however this is to say that an application cannot solely use a DatabasePool object for connections unless it either writes to the database first or uses a DatabaseQueue object instead.

Others may have read #40 and figured they were using the code incorrectly however in the two examples provided to you, it is clear that there is a bug in the object for newly created databases. Unless DatabasePool should not be used for new/empty database reads to system tables. If so, please either provide proper guidance in your documentation or update the code to not explicitly call try! so that the caller can catch and handle the exception.

@groue
Copy link
Owner

groue commented Aug 16, 2016

Listen, @blkbam. If you have found a real issue, I'm really happy, because I like this library to be as bug-free as possible.

Since you say that you are not facing a concurrency issue, then you are able to write a short and minimal sample code that reveals the problem. Please write a real bug report with steps to reproduce, expected output, actual output. Comments are not necessary. We'll then reopen this issue.

@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

Define real bug report? I've provided all that you asked in the original post. The only difference is that my class uses a ViewController which again done for demo purposes. Would you like me to attach a whole project?

@groue
Copy link
Owner

groue commented Aug 16, 2016

I was thinking that a sample code of a few lines would have been enough. But if your issue only reveals in a whole project, then it's even more crucial that you write a real bug report.

@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

This is a minimal sample while also following the intended use of your the pooling philosophy. A full project would just include all to other basic templated files.

@groue
Copy link
Owner

groue commented Aug 16, 2016

Have a nice day!

@blkbam
Copy link
Author

blkbam commented Aug 16, 2016

Thanks. You as well.

@groue groue added invalid or off-topic Not about GRDB, or does not contain any useful information and removed support labels Aug 16, 2016
@swiftlyfalling
Copy link
Collaborator

swiftlyfalling commented Aug 17, 2016

@groue:

I just encountered something similar, so allow me to include a reduced sample and a dissection of what seems to be happening:

import Foundation
import GRDB
func testDatabasePool() {

    let databaseName: String = NSUUID().UUIDString // a fresh/new database file is used
    let rootLibDirectory = NSSearchPathForDirectoriesInDomains( .LibraryDirectory, .UserDomainMask, true ).first as String!

    do {
        print("Opening database with name: \(databaseName).db")
        let dbPool = try DatabasePool(path: "\(rootLibDirectory)/\(databaseName).db")

        dbPool.read { db in
            print("Never reaches here")
            // execution never reaches here - exception occurs first
        }
    }
    catch {
        print ("Encountered Error: \(error)")
    }
    print ("Finished: testDatabasePool") // never reaches here
}
testDatabasePool()

What seems to be happening:

(Note: In this example, a unique database filename is generated each time it runs to ensure that the database file does not yet exist.)

In DatabasePool.init:

  1. The writer (a SerializedDatabase) is initialized. (This creates the .db file.)
  2. "PRAGMA journal_mode = WAL" is successfully set.
  3. "PRAGMA synchronous = NORMAL" is successfully set.
  4. DatabasePool.init succeeds.
    When dbPool.read triggers readerPool.makeElement:
  5. The attempt to open a read-only connection to the database (a SerializedDatabase) fails with SQLITE_ERROR = 14.
    At this point, the expected -wal and -shm files created by WAL mode are not present.

This seems to be a quirk in the behavior of SQLite.

On a database not already in WAL mode (in this example, a fresh database file), "PRAGMA journal_mode = WAL" succeeds, but does not actually create the -wal and -shm files. If an attempt is made to open a read-only connection to the database before then (in this case, when DatabasePool attempts to open a read-only SerializedDatabase connection), it fails with SQLITE_ERROR = 14 (presumably because, as a read-only connection, it cannot create the -wal and -shm files).

If the database file already exists and is already in WAL mode, then the initial attempt to create the writer in DatabasePool results in SQLite creating the -wal and -shm files (if not present), and thus the following attempt to open a read-only SerializedDatabase connection succeeds.

Or in other words:

If a database is not already in WAL-mode when opened,
then setting "PRAGMA journal_mode = WAL" is not sufficient to allow other read-only database connections because the -wal and -shm files do not yet exist, are not created by setting the journal_mode, cannot be created by read-only database connections, and are required for the read-only database connections to succeed.

Either:

  1. A write to the database must occur, OR
  2. A non-read-only database connection must be opened.

For SQLite to create the -wal and -shm files, which then permits future read-only database connections.


Since this can be triggered by at least two simple scenarios, I think it may be worth coding a workaround into DatabasePool for this odd SQLite behavior (i.e. to force SQLite to generate the -wal and -shm files after WAL mode is set).

Trigger 1: Using DatabasePool to open and read from a database that doesn't yet exist (prior to any writes).
Trigger 2: Using DatabasePool to open and read from an existing modifiable SQLite database that is not yet in WAL mode (prior to any writes).

@swiftlyfalling
Copy link
Collaborator

swiftlyfalling commented Aug 17, 2016

I believe that #99 may actually be due to this as well. (Trigger 2, above)

@groue
Copy link
Owner

groue commented Aug 17, 2016

Thanks for the investigation @swiftlyfalling!

But the sample code you provide has a logic error, since it reads from an empty database. There's nothing to read.

The error is thrown from the SELECT * FROM sqlite_master LIMIT 1 query that is run each time a connection opens, and aims at throwing an early error whenever the user opens a file which is not a database, or provides the wrong encryption key.

Well, the database is empty, so it has no schema, and I'm not surprised that there is an error.

I admit that error 14 (SQLITE_CANTOPEN "Unable to open the database file") is unexpected. But I don't see any quirk in SQLite, nor any bug in GRDB here.

Just application code that needs to be fixed.

If a database is not already in WAL-mode when opened,
then setting "PRAGMA journal_mode = WAL" is not sufficient to allow other read-only database connections because the -wal and -shm files do not yet exist, are not created by setting the journal_mode, cannot be created by read-only database connections, and are required for the read-only database connections to succeed.

This looks like another piece of information. Do you mean that this error also happens on a non-empty database?

@groue
Copy link
Owner

groue commented Aug 17, 2016

Yes, this is what you mean by "trigger 2":

Using DatabasePool to open and read from an existing modifiable SQLite database that is not yet in WAL mode (prior to any writes).

OK we have a bug, and I could reproduce it.

The trigger 1 reveals an application bug, and does not require specific handling.

@groue groue reopened this Aug 17, 2016
@groue groue added bug and removed invalid or off-topic Not about GRDB, or does not contain any useful information labels Aug 17, 2016
groue added a commit that referenced this issue Aug 17, 2016
@groue groue changed the title DatabasePool connection crashes if no write is performed first DatabasePool can't read from existing non-WAL database Aug 17, 2016
@groue groue closed this as completed in 5a296b1 Aug 17, 2016
@groue
Copy link
Owner

groue commented Aug 17, 2016

@swiftlyfalling @blkbam The fix has been released in v0.79.4.

This was referenced Aug 17, 2016
@blkbam
Copy link
Author

blkbam commented Aug 17, 2016

Thanks for the fix! I can confirm it is working as expected.

@groue
Copy link
Owner

groue commented Aug 17, 2016

You're welcome!

@swiftlyfalling
Copy link
Collaborator

swiftlyfalling commented Aug 17, 2016

Yup, fix looks good on my end too. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants