Retry failing for implicit pool queries

[ivank]

When using pool.query and Postgres throws an error, for example deadlock detected, code 40P01 slink attempts a retry. It is possible to have a deadlock even when executing a normal update query, not just a transaction.

Expected Behavior

We should retry the implicit query.

Current Behavior

The query mechanism seems to assume there are transactionQueries and we end up with transactionQueries is not iterable error. (In node_modules/slonik/dist/routines/executeQuery.js:32:38)

Steps to Reproduce

Execute several update queries that would deadlock, using implicit pool.query

Logs

    TypeError: transactionQueries is not iterable
        at retryTransaction (/Users/ivankerin/Projects/boost-statements-loss/node_modules/slonik/dist/routines/executeQuery.js:32:38)
        at process._tickCallback (internal/process/next_tick.js:68:7) }

If I set transactionRetryLimit: 0, I get:

   error:
    { error: deadlock detected
        at Connection.parseE (/Users/ivankerin/Projects/boost-statements-loss/node_modules/pg/lib/connection.js:614:13)
        at Connection.parseMessage (/Users/ivankerin/Projects/boost-statements-loss/node_modules/pg/lib/connection.js:413:19)
        at Socket.<anonymous> (/Users/ivankerin/Projects/boost-statements-loss/node_modules/pg/lib/connection.js:129:22)
        at Socket.emit (events.js:198:13)
        at Socket.EventEmitter.emit (domain.js:448:20)
        at addChunk (_stream_readable.js:287:12)
        at readableAddChunk (_stream_readable.js:268:11)
        at Socket.Readable.push (_stream_readable.js:223:10)
        at TCP.onStreamRead [as onread] (internal/stream_base_commons.js:94:17)
      name: 'error',
      length: 323,
      severity: 'ERROR',
      code: '40P01',
      detail:
       'Process 383 waits for ShareLock on transaction 5191; blocked by process 378.\nProcess 378 waits for ShareLock on transaction 5190; blocked by process 383.',
      hint: 'See server log for query details.',
      position: undefined,
      internalPosition: undefined,
      internalQuery: undefined,
      where: 'while updating tuple (59565,14) in relation "supply"',
      schema: undefined,
      table: undefined,
      column: undefined,
      dataType: undefined,
      constraint: undefined,
      file: 'deadlock.c',
      line: '1146',
      routine: 'DeadLockReport' } }

[gajus]

Sounds like a simple fix. Are you available to raise a PR?

[ivank]

Sorry, not used to dealing with flow. Would gladly help if it was plain js / TypeScript.

[gajus]

There is nothing about this fix that requires knowing Flow.

[oh-wo]

Hi @gajus ,

I'm interested to have a go at this one but need some guidance as I am fresh in this codebase. Like ivank I am not used to some of the conventions/structure of the codebase so please "go easy on me" if I am a bit "off".

The problem
I've had a look around and think I understand the problem:

• retry of failed transactions works now. It works by invoking retryTransaction in which it repeatedly retries the transaction until remainingRetries is 0, at which point it exits.
• remainingRetries expects transactionQueries to be an iterable and fails if it is not, in the loop within retryTransaction that applies each transactionQuery
• transactionQueries is an array for transactions (set in transaction), but is undefined for other queries

How to fix
I think the best fix is to modify lines 152/159 of executeQuery.js to always set transactionQueries.

if (!connection.connection.slonik.transactionQueries) {
   connection.connection.slonik.transactionQueries = []
}

connection.connection.slonik.transactionQueries.push({
  executionContext,
  executionRoutine,
  sql: actualQuery.sql,
  values: actualQuery.values,
})

• are there other side effects elsewhere if I do this? I don't quite understand the role of transactionQueries vs actualQuery and why they're split:
• Alternatively, connection.connection.slonik.transactionQueries = []; could be defined elsewhere, like in mapTaggedTemplateLiteralInvocation (and the other methods in bindPoolConnection) so it is always defined for each query.
• would that work or is there more to it than that? Is there an alternative solution you prefer where we don't use transactionQueries?
• Looking at transaction, connection.connection.slonik.transactionQueries = []; is set when the transactionId is defined. Therefore it would be nice to do the same within targetMethod (e.g. manyFirst etc). Is that best?

If you have a few minutes, could you please provide any feedback? Thanks for your time.

[gajus]

@oh-wo if you can add a failing test case to integration, I will make the necessary changes to make it work.

[oh-wo]

Thanks; I will aim to open a PR in the next 1-2 weeks.

[oh-wo]

Hey Gajus,

Just to follow up and apologise for the delay; I am not sure when I will get to this, despite it being on my mind.

[nponiros]

I'm trying to fix this before fixing #163 but I'm a bit unsure what the best way to fix it is.

My initial idea was to only retry the query that failed but I don't see a way to tell if the query is part of a transaction that was started by executing a 'begin' query without using the transaction method. If it is part of a transaction then retrying the query will not work.

I also don't think that using connection.connection.slonik.transactionQueries as suggested above will work because, as far as I can tell, there is no guarantee that only one query will be executed on any given connection. If a connection is used for multiple queries and we add the transactionQueries array to it in any method we will have the bug described in #163. Also if the user didn't explicitly start a transaction it is probably a bad idea to retry all queries.

The last possible solution I can think of is to fix the code so that it doesn't retry if we are not within a transaction started by the transaction method. This way the code will not throw the transactionQueries is not iterable error. With this users will see the deadlock error and would have to handle it themselves.

@gajus unless you have something against my first idea I will just implement that and trust that the users will not start transactions without using the transaction method.

[gajus]

@gajus unless you have something against my first idea I will just implement that and trust that the users will not start transactions without using the transaction method.

That's a reasonable assumption.

[gajus]

🎉 This issue has been resolved in version 25.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀