feat: evict connections with open transactions on pool release

[panga]

Summary

• Add _txStatus tracking to pg Client, populated from PostgreSQL's ReadyForQuery message
• Check _txStatus 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 eviction behavior

Behavior Changes

• 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 new behavior is not implemented in pg-native because it works at protocol level which we don't have access in libpq.
• This change was also tested with PgBouncer in transaction mode to verify it is working as expected.

[charmander]

Is this check just for tests and pg-native? Both should probably be updated to consistently report a status in their readyForQuery events.

[panga]

Added integration tests and updated PR description. However pg-native is not supported because this is a protocol level feature.

[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.