Skip to content

9009 dxox: Transaction support (formerly flexion#10705)#6283

Draft
Mwindo wants to merge 47 commits intostagingfrom
10705-dxox
Draft

9009 dxox: Transaction support (formerly flexion#10705)#6283
Mwindo wants to merge 47 commits intostagingfrom
10705-dxox

Conversation

@Mwindo
Copy link
Contributor

@Mwindo Mwindo commented Jul 11, 2025

This PR has two fundamental goals:

  1. First a foremost, implement transaction support.
  2. Revise our database connection code so that a) things are named better (don't say we are returning a connection when we aren't!) and b) use our connection pool correctly so that things don't break if we increase the number of clients in the pool.

Details:

I think our current connection code has a few issues.

  1. We say, getConnection and establishConnection ... but these are not getting a connection or establishing a connection. They are returning a kysely wrapper object around a client pool. Behind the scenes, kysely is creating/acquiring/releasing/removing connections when we call, e.g., .execute.
  2. We have two connection pools, one for getConnection and one for getLockingConnection. For the latter especially, we abuse the pool: we say, "Make sure there is only ever one connection in your pool so that we don't accidentally create a lock on one connection and try to remove it with another, which won't work!" But this is not what a connection pool is for. Instead, we should have one connection pool and get an honest-to-goodness connection from the pool and use that throughout the lifetime of the lock. Nope. This is complicated.
  3. If we have more than one connection in our pool, we could run into problems. We built things with one connection in mind. This is unnecessarily fragile.

En route to transaction support, this PR addresses those issues as follows:

  1. Rename getConnection and establishConnection etc. If we think of these objects as connections, we will confuse things.
  2. We have one connection pool. The equivalent of getLockingConnection, acquireOneDbConnection, now simply gets a particular connection from the existing pool and uses it throughout the lifetime of the locking code. Nope.
  3. Pass in context--e.g., a db transaction--to children processes via AsyncLocalStorage as needed. This means that we can in principle have two asynchronous connections at once that each know which transaction they are a part of. It also means we don't have to try passing around connections manually, which would be a huge mess. In other words, we can extend the number of clients in our pool and everything should "just work" because we have isolated connections per node process-chain.

Implementation details:

  • New file: transactions.ts, which defines withTransaction and a couple of helpers inTransaction (answers the question "Am I currently executing as part of a transaction?") and onTransactionCommit (queues up work to be executed upon a successful transaction).
  • Revision: database.ts, which now uses the helpers inTransaction and onTransactionCommit. If inTransaction() is false in getDbWriter, we send OpenSearch index messages directly after writing to Postgres, as we did before. If it is true, we cache the messages via onTransactionCommit to send only once the transaction is committed.
  • Revision: getConnection.ts, renamed to databaseConnection.ts, which now defines context we can send to child processes. getDb, the rough equivalent to our previous getConnection, now first checks "Do I have a connection passed in from my parent process that I should be using?" If so, it returns that. This allows us to pass a transaction (or, in the future, other connection stuff) through an arbitrarily complex child callback chain.
  • Revision: Move the connection stuff into the postgres directory instead of having it free-floating in the root.

}

// Callbacks to run once the commit is successful
export function onTransactionCommit(cb: () => Promise<void>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not want to update OpenSearch when we make a query unless we commit the transaction. onTransactionCommit allows us to say, "Ok, do X--like update OpenSearch--once the transaction commits."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing to test: if we queue up a ton of OpenSearch updates, will we run into memory limits?

Copy link
Contributor Author

@Mwindo Mwindo Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other potential use cases for onTransactionCommit: do not generate these PDFs or send these emails until we've updated the DB.

Mwindo added 3 commits July 12, 2025 10:05
…ally, and make connections more robust so that we could increase our maximum pool size without breaking everything
let transactionStore: ConnectionInfo = {} as ConnectionInfo;

// Start the transaction; Kysely handles BEGIN/COMMIT/ROLLBACK.
const result = await db.transaction().execute(async trx => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that kysely.transaction 1) guarantees the same connection throughout the transaction lifecycle and 2) handles the mutex lock so that no two transactions will ever run on the same connection.

@jimlerza jimlerza changed the title 10705 dxox: Transaction support 9009 dxox: Transaction support (formerly flexion#10705) Sep 3, 2025
@codyseibert
Copy link
Contributor

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.

4 participants