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

Retry fails for concurrent read-write SQLite transactions #10262

Open
rahuljayaraman opened this Issue Dec 13, 2018 · 6 comments

Comments

2 participants
@rahuljayaraman
Copy link

rahuljayaraman commented Dec 13, 2018

What are you doing?

I tried running 2 read-write transactions concurrently. Tried configuring SQLite to run in default Rollback Journal & WAL modes.

const Sequelize = require('sequelize')
const sequelize = new Sequelize({
  dialect: 'sqlite',
  storage: 'db.sqlite',
  retry: {
    max: 5
  }
})

// Optional: Comment out to run in default Rollback journal
sequelize.query('PRAGMA journal_mode=WAL;')

const table = sequelize.define('table', {
  data: Sequelize.STRING
})

const ins = data => table.findOrCreate({ where: { data } })

sequelize
  .sync()
  .then(() => Promise.all([1, 2].map(idx => ins(idx))))
  .catch(e => console.log('ERROR', e))

What do you expect to happen?

I expect retry to eventually succeed.

What is actually happening?

In WAL mode, the first transaction succeeds on COMMIT while the other one fails. The failed INSERT is retried 5 times (as configured) & eventually fails with SQLITE_BUSY: database is locked error.

In rollback journal mode, COMMIT on the first transaction waits silently for other transactions to complete (as second transaction's read query has acquired a shared lock). The second transaction retries INSERT & eventually fails.

Output, either JSON or SQL

Executing (default): PRAGMA journal_mode=WAL;
Executing (default): CREATE TABLE IF NOT EXISTS `tables` (`id` INTEGER PRIMARY KEY AUTOINCREMENT, `data` VARCHAR(255), `createdAt` DATETIME NOT NULL, `updatedAt` DATETIME NOT NULL);
Executing (default): PRAGMA INDEX_LIST(`tables`)
Executing (62e1e844-fdb0-41c4-83f1-e886e2860a6b): BEGIN DEFERRED TRANSACTION;
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): BEGIN DEFERRED TRANSACTION;
Executing (62e1e844-fdb0-41c4-83f1-e886e2860a6b): SELECT `id`, `data`, `createdAt`, `updatedAt` FROM `tables` AS `table` WHERE `table`.`data` = 1 LIMIT 1;
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): SELECT `id`, `data`, `createdAt`, `updatedAt` FROM `tables` AS `table` WHERE `table`.`data` = 2 LIMIT 1;
Executing (62e1e844-fdb0-41c4-83f1-e886e2860a6b): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,1,'2018-12-13 10:33:48.775 +00:00','2018-12-13 10:33:48.775 +00:00');
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,'2018-12-13 10:33:48.784 +00:00','2018-12-13 10:33:48.784 +00:00');
Executing (62e1e844-fdb0-41c4-83f1-e886e2860a6b): COMMIT;
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,'2018-12-13 10:33:48.784 +00:00','2018-12-13 10:33:48.784 +00:00');
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,'2018-12-13 10:33:48.784 +00:00','2018-12-13 10:33:48.784 +00:00');
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,'2018-12-13 10:33:48.784 +00:00','2018-12-13 10:33:48.784 +00:00');
Executing (05577241-c2b5-462e-8cf5-9fef9e6b43b0): INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,'2018-12-13 10:33:48.784 +00:00','2018-12-13 10:33:48.784 +00:00');

...... this continues for no. of retries and then

ERROR { SequelizeTimeoutError: SQLITE_BUSY: database is locked
    at Query.formatError (/node_modules/sequelize/lib/dialects/sqlite/query.js:420:16)
    at Statement.afterExecute (/node_modules/sequelize/lib/dialects/sqlite/query.js:119:32)
    at Statement.replacement (/node_modules/sqlite3/lib/trace.js:19:31)
  name: 'SequelizeTimeoutError',
  parent:
   { Error: SQLITE_BUSY: database is locked
     errno: 5,
     code: 'SQLITE_BUSY',
     sql:
      'INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,\'2018-12-13 10:33:00.342 +00:00\',\'2018-12-13 10:33:00.342 +00:00\');' },
  original:
   { Error: SQLITE_BUSY: database is locked
     errno: 5,
     code: 'SQLITE_BUSY',
     sql:
      'INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,\'2018-12-13 10:33:00.342 +00:00\',\'2018-12-13 10:33:00.342 +00:00\');' },
  sql:
   'INSERT INTO `tables` (`id`,`data`,`createdAt`,`updatedAt`) VALUES (NULL,2,\'2018-12-13 10:33:00.342 +00:00\',\'2018-12-13 10:33:00.342 +00:00\');' }

Dialect: sqlite
Dialect version: 4.0.4
Database version: 3
Sequelize version: 4.41.2
Tested with latest release: No

Continuously retrying a single failed query inside a transaction might not always work.
In WAL mode, for example, SQLite transactions may fail with a BUSY_SNAPSHOT error. In such cases, the whole transaction needs to be re-tried instead of only INSERT query.

I can make the above code snippet work by setting transactionType to IMMEDIATE which makes BEGIN fail & forces the whole transaction to be retried.

const sequelize = new Sequelize({
...
  transactionType: 'IMMEDIATE',
...
})
@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Dec 15, 2018

Sequelize implements query level retrying, what you need here is transaction level retrying. I suggest you should implement this logic at application level, probably using retry-as-promised.

Sequelize can't reliably tell if retrying those same transaction instruction is correct logic as it could have changed based on other factors, that old set of SQL statements may be invalid.

Also if your application is going to have high level of parallel writes then I suggest using Postgres

@rahuljayaraman

This comment has been minimized.

Copy link

rahuljayaraman commented Dec 16, 2018

Thanks for your response.

Sequelize implements query level retrying, what you need here is transaction level retrying. I suggest you should implement this logic at application level, probably using retry-as-promised.

Please have a look at the last section again. I realize that sequelize implements query level retry. For SQLite, It can be converted to transaction level using BEGIN IMMEDIATE.

Sequelize can't reliably tell if retrying those same transaction instruction is correct logic as it could have changed based on other factors, that old set of SQL statements may be invalid.

Not sure if I understand your concerns correctly. IMO, anyone using retry would expect it to eventually succeed. If out of order transactions is a concern, maybe it should be handled at the application level. Frankly, the retry deadlock was a surprising default behavior for me and took time to debug.

Also if your application is going to have high level of parallel writes then I suggest using Postgres

Don't think a few concurrent txns should be a problem for SQLite with retry implemented correctly. My example had only 2.

Suggest documenting current retry behaviour & side effects. Also, for SQLite, the fact that if it's eventually expected to succeed, transactionType should be set to IMMEDIATE.

@sushantdhiman

This comment has been minimized.

Copy link
Member

sushantdhiman commented Dec 16, 2018

* @param {Object} [options.retry] Set of flags that control when a query is automatically retried.

Documentation already says query is retried, there is no mention of transaction. As I suggested earlier you should try your business logic using retry-as-promised.

Also, for SQLite, the fact that if it's eventually expected to succeed, transactionType should be set to IMMEDIATE.

I don't think we should point out these things as these are database specific concerns, anyone using SQLite and transaction in production should understand what are the effects of transactions types. if they don't care about that I highly doubt they will even read documentation, they will simply open a new issue here.

If you want you can add more docs here, note that we already point out "SQLite does not support more than one transaction at the same time."

@rahuljayaraman

This comment has been minimized.

Copy link

rahuljayaraman commented Dec 16, 2018

"SQLite does not support more than one transaction at the same time."

Don't think that's true. DEFERRED mode will interleave ops between transactions for example.

Documentation already says query is retried, there is no mention of transaction. As I suggested earlier you should try your business logic using retry-as-promised.

What I meant is that the side effects of retrying only a query & not transaction should be documented since it is the default behavior.

I don't think we should point out these things as these are database specific concerns, anyone using SQLite and transaction in production should understand what are the effects of transactions types. if they don't care about that I highly doubt they will even read documentation, they will simply open a new issue here.

If it were up to me, I would just default to BEGIN IMMEDIATE for SQLite, to make the API behave more reliably.

@rahuljayaraman

This comment has been minimized.

Copy link

rahuljayaraman commented Dec 16, 2018

If you want you can add more docs here, note that we already point out "SQLite does not support more than one transaction at the same time."

Will try to update docs & send a PR.

@rahuljayaraman

This comment has been minimized.

Copy link

rahuljayaraman commented Dec 17, 2018

Btw, this is just FYI. SQLite already has a busy_handler which works pretty well for query level retries. It detects deadlocks before attempting a retry and returns SQLITE_BUSY. From the docs

The presence of a busy handler does not guarantee that it will be invoked when there is lock contention. If SQLite determines that invoking the busy handler could result in a deadlock, it will go ahead and return SQLITE_BUSY to the application instead of invoking the busy handler. Consider a scenario where one process is holding a read lock that it is trying to promote to a reserved lock and a second process is holding a reserved lock that it is trying to promote to an exclusive lock. The first process cannot proceed because it is blocked by the second and the second process cannot proceed because it is blocked by the first. If both processes invoke the busy handlers, neither will make any progress. Therefore, SQLite returns SQLITE_BUSY for the first process, hoping that this will induce the first process to release its read lock and allow the second process to proceed.

People may want to wrap this with a transaction level retry at application level if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment