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

Implicit transaction in DatabasePool.write and DatabaseQueue.write #332

Merged
merged 17 commits into from
Apr 15, 2018

Conversation

groue
Copy link
Owner

@groue groue commented Apr 11, 2018

This PR fixes an ergonomic issue with database pools and queues, and has them wrap database accesses in a transaction by default.

The problem is clear with database pools: unless writes are wrapped in a transaction, concurrent reads see partial changes. This is positively unsafe:

// Currently unsafe
try dbPool.write { db
    try Credit(destinationAccout, amount).insert(db)
    // <- Concurrent dbPool.read sees a partial db update here
    try Debit(sourceAccount, amount).insert(db)
}

// Current way to make it safe:
try dbPool.writeInTransaction { db
    try Credit(destinationAccout, amount).insert(db)
    try Debit(sourceAccount, amount).insert(db)
    return .commit
}

This subtlety in the concurrent behavior of database pools was abundantly documented. But personal experience with coworkers and frequent feedback from GRDB users have revealed that it was too high a burden to force users to remember to use transactions in nearly all their database pool writes.

The problem is less pressing with database queues, since serialization of database accesses prevents concurrent reads from fetching inconsistent database values. Yet, personal experience again reveals that users perform a constant and conscious effort in order to choose whether to use a transaction or not, or wonder if a transaction-less access hides a potential bug or not.

On top of that, the GRDB 2.0 API makes it painful to both run a transaction and extract values, leading to more opportunities to hesitate between transactions and convenience:

// Typical GRDB 2.0 code: meh
var count: Int! = nil
dbQueue.inTransaction { db in
    try ...
    count = try Player.fetchCount(db)
    return .commit
}
print(count)

Since GRDB is supposed to take care of concurrency difficulties so that developers can profit from the safest behavior without much thinking about it, something has to be done.


After this PR is merged:

  • dbQueue.write and dbPool.write run inside a transaction.
  • dbQueue.inTransaction and dbPool.writeInTransaction are still here, in order to support the various SQLite transaction types, and explicit rollbacks.
  • dbQueue.inDatabase, which does not open a transaction, is now documented as an advanced method.
  • A new method dbPool.writeWithoutTransaction is introduced, for the advanced use cases that need fine-grained transactions.
// After this PR is merged:

// BEGIN TRANSACTION
// INSERT INTO credits ...
// INSERT INTO debits ...
// COMMIT
try dbPool.write { db in  // or dbQueue.write
    try Credit(destinationAccout, amount).insert(db)
    try Debit(sourceAccount, amount).insert(db)
}

// BEGIN IMMEDIATE TRANSACTION
// INSERT INTO credits ...
// INSERT INTO debits ...
// COMMIT
try dbPool.writeInTransaction(.immediate) { db in  // or dbQueue.inTransaction
    try Credit(destinationAccout, amount).insert(db)
    try Debit(sourceAccount, amount).insert(db)
    return .commit
}

// INSERT INTO credits ...
// INSERT INTO debits ...
try dbPool.writeWithoutTransaction { db in // or dbQueue.inDatabase
    try Credit(destinationAccout, amount).insert(db)
    // <- Concurrent dbPool.read sees a partial db update here
    try Debit(sourceAccount, amount).insert(db)
}

groue added 2 commits April 11, 2018 20:03
…don't see any changes until all changes are executed. DatabasePool.write thus wraps changes in a transaction.
@groue groue added this to the GRDB 3.0 milestone Apr 11, 2018
@groue
Copy link
Owner Author

groue commented Apr 11, 2018

Thanks @mayurdzk and @sobri909 for the inspiration!

@groue groue mentioned this pull request Apr 11, 2018
29 tasks
@groue groue changed the title Implicit transaction in DatabasePool.write Implicit transaction in DatabasePool.write and DatabaseQueue.write Apr 15, 2018
@groue groue merged commit 1c9c3d4 into GRDB3 Apr 15, 2018
@groue groue deleted the GRDB3-DatabaseWriterRobustness branch April 15, 2018 17:51
groue added a commit to hartbit/RxGRDB that referenced this pull request Apr 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant