feat: evict connections with open transactions on pool release

[panga]

Summary

• Add _txStatus tracking to pg Client, populated from PostgreSQL's ReadyForQuery message
• Add a new client.getTransactionStatus() method to return the _txStatus value
• Add a new pool option evictOnOpenTransaction (disabled by default for backwards compatiblity)
• Check client.getTransactionStatus() in pg-pool's release() method to evict connections not in idle (I) state instead of returning them to the pool
• Add a new log indicating that a connection was evicted from the pool due to open transaction
• Add tests covering the new methods and eviction behavior

Behavior Changes

• When evictOnOpenTransaction: true, a connection is closed and removed from the pool if it is released with an active transaction (BEGIN without COMMIT/ROLLBACK).

Problem

When a client with an active transaction (BEGIN without COMMIT/ROLLBACK) is released back to the pool, the connection is returned in a non-idle transaction state. The next consumer that checks out this connection inherits the open transaction, which can cause:

• Row-level lock retention: Rows locked by SELECT ... FOR UPDATE or UPDATE inside the uncommitted transaction remain locked, blocking other connections that try to access the same rows.
• Connection pool exhaustion: In a pool with max: N connections, poisoned connections that hold locks can cause other connections to block waiting on those locks, eventually starving the pool.
• Silent data visibility issues: The next consumer operates inside someone else's transaction with a stale MVCC snapshot, potentially reading stale data or having writes silently rolled back.

This is a known as "leaked/poison connection" problem in connection pools. This feature was recently added in Go pgx jackc/pgx#2481

Notes

• This change was also tested with PgBouncer in transaction mode to verify it is working as expected.

[charmander]

Related: #391, #724

How about throwing an error when attempting to return a client with an open transaction to the pool, since that indicates an app bug in current pg, and pg doesn’t have the information to make the optimal choice by itself?

[panga]

@charmander thanks for the review. I added a log to align with the behavior in other cases. I couldn’t find a clean way to surface an error at that point since release returns void and usually called on final. Given the trade-offs around pool correctness, I think it’s preferable to prevent an invalid state rather than silently ignoring it.

Do you think this behavior change should be behind a pool option and disabled by default?

[brianc]

Do you think this behavior change should be behind a pool option and disabled by default?

Though I think how the pool behaves now (doesn't have any concept of the client being in a transaction and will happily rent it out to another caller later) is almost always wrong, I think it risks weird behavioral breaking changes if we change the default behavior in the 8.x branch....so putting it behind a config where the default value currently is to do what it does now (which is incorrect) is there, but one can opt into auto-aborting & closing clients which are returned to the pool while a transaction is still pending.

So for pg@8.x it would be

// not sure what a good config property name is...but something like this:
const pool = new Pool({ evictOnOpenTransaction: true })

To opt into the behavior. Then in 9.x we can make this behavior the default & actually remove the feature flag entirely since I don't see any reason to allow the old, invalid behavior long term.

How about throwing an error when attempting to return a client with an open transaction to the pool, since that indicates an app bug in current pg, and pg doesn’t have the information to make the optimal choice by itself?

I agree w/ @panga that .release() seems like a disruptive place to throw an error as its usually in a cleanup phase & historically doesn't throw. I think one nice thing though would be to actually put .transactionStatus() as an actuall method (or getter property) on the client itself and make it public. That would allow libraries or users to perform the check if they wanted to and throw on their side. What do you think about that?

[charmander]

I couldn’t find a clean way to surface an error at that point since release returns void and usually called on final.

My suggestion was to throw an error synchronously from release. Yes, it’s disruptive if it happens, but that’s the point, since it indicates a serious bug (that would not necessarily even be fixed by discarding the client from the pool). I can’t think of a legitimate reason to return a connection with an open transaction to a pool. But for strict backward compatibility, it could be a deprecation warning in pg 8, with a possible option to opt-in to pg 9 behaviour early.

Exposing the status as a public property is a good idea for other reasons, though.

[brianc]

My suggestion was to throw an error synchronously from release. Yes, it’s disruptive if it happens, but that’s the point, since it indicates a serious bug (that would not necessarily even be fixed by discarding the client from the pool). I can’t think of a legitimate reason to return a connection with an open transaction to a pool. But for strict backward compatibility, it could be a deprecation warning in pg 8, with a possible option to opt-in to pg 9 behaviour early.

Yeah that makes sense too. I'll work on a PR soon for the 9.x branch. I agree it should be considered an error when you return a client that's in a transaction...and closing the users connection is kinda more of a surprise and "silent error" than just throwing as its almost certainly a mistake in the code w/ a dangling transaction.

[panga]

I implemented the recommended changes for the new pool option evictOnOpenTransaction and for client.getTransactionStatus(), including the corresponding documentation updates.

I'm concerned that throwing errors from release() may be overly disruptive for many Node.js applications, because in a number of codebases this can surface as an uncaught exception and terminate the process. For example, a common implementation pattern I see looks like this:

const client = await pool.connect();
try {
    return await client.query(...);
} catch (err) {
    client.release(err);
} finally {
    client.release();
}

Edit: The above example is not correct, it need to catch the err to avoid double release.
e.g:

try {
...
} catch (err) {
  clientErr = err
} finally {
  client.release(clientErr)
}

[charmander]

I'm concerned that throwing errors from release() may be overly disruptive for many Node.js applications, because in a number of codebases this can surface as an uncaught exception and terminate the process.

Yes, that’s normal JavaScript behaviour for error conditions. Note that this would not be an uncatchable exception, and would typically not terminate the process in applications that care about surviving unexpected error conditions.

For example, a common implementation pattern I see looks like this: […]

That pattern is broken and should not be used, but it’s not incompatible with the general approach of throwing an exception as long as the second release is a no-op.

[panga]

@charmander you’re right. The simplified example I shared above is not correct because it can result in a double release. In practice, the implementation should explicitly handle the error path and ensure the resource is released exactly once, typically by passing the error object to release(err).

If @brianc is aligned, I can add a throw in this case since it’s behind a feature flag, which should avoid introducing breaking changes for the current version.

[panga]

The PR is ready for another review. Could you share what the expectations are, and whether it can go into a minor release now that it’s behind a config flag? I’m a first-time contributor, so I’m just trying to understand the process.

[panga]

@charmander I’ve added native support in b10746f. It depends on brianc/node-libpq#123

[brianc]

@panga thanks for doing the work here! and for doing the work on the native side too that's top notch! I hate to be a monster about this...but do you think we could separate this out into 1 PR that just adds the transaction status but doesn't add the optional stuff to the pool and then a different one to add the pool behavior? I'm kinda thinking on the pool behavior one what if instead of having a config option that evicts it we add an onRelease callback (I just added onConnect here) and it might be more generic to have a callback that's passed the client. By default the callback is undefined but then the user could implement whatever they wanted:

onRelease: async (client) => {
  if (client.getTransactionStatus() !== 'I') {
  await client.query('ROLLBACK')
  console.error("YO YOUR CODE IS DOING SOMETHING BAD. TRANSACTION AUTOMATICALLY ROLLED BACK")
  client.release()
  }
}

or

onRelease: (client) => {
  console.error('transaction pending client released. destroying the universe')
   process.exit(-1)
}

etc...

Thoughts? I know its more work...I just want to keep opinions out kinda and extensibility as the focus. Personally I'd probably throw in the release function (and that throw should make the pool.release or client.release function throw. But everyone might have diff opinions. That's totally backwards compatible too so don't need a major bump or anything (your change didn't either but i feel like this might be more future-proof?)