Skip to content
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

Add comments to promise aware knex transaction commits #6061

Open
qolity opened this issue Apr 21, 2024 · 0 comments
Open

Add comments to promise aware knex transaction commits #6061

qolity opened this issue Apr 21, 2024 · 0 comments

Comments

@qolity
Copy link

qolity commented Apr 21, 2024

Feature discussion / request

We are currently using AWS Aurora PostgreSQL, with a knex based application. Our database service has some unexpected slowness that seems to be purely from the COMMIT statement in an interactive transaction block. Unhelpfully, there is no way to tell which transaction has issued that COMMIT (we have some fixes in development based on solid hypotheses)

What we would like is to add a comment to each separate transaction block, which will allow disambiguating the COMMITs in our existing analytics tools. So instead of pg_stat_statements just having a single COMMIT, it has multiple, such as /* transactionA */ COMMIT.

As far as I understand, this is completely safe to do. It seems this is a problem many other people face, as there are a lot of threads about people trying to figure out what slow COMMIT logs represent.

At first, I tried using a manual raw COMMIT using the transacting statement in a promise-aware transaction:

return trx
    .raw(`/* ${tag} */ COMMIT;`)
    .transacting(trx)
    .then(_ => result);

This did seem to work. However after looking in the knex source code, it seems this is not a drop-in replacement, as the promise-aware knex transaction will issue RELEASE SAVEPOINT ${this.txid}; instead of COMMIT. This should have no effect as I understand, but it's not a drop-in replacement which is the ideal.
Secondly, this approach relies on adding manual rollbacks around the promise-aware transaction blocks, and it would be easy to miss these and cause subtle and potentially explosive application bugs.

I tried some attempts of using the .comment() queryBuilder method, but it does not appear that the queryBuilder is involved in issuing the promise-aware automatic COMMIT.

As a second approach, I found this other knex issue where the Transaction prototype commit function was replaced: #1641

This approach combined with AsyncLocalStorage allows injecting a custom comment into the commit statement in a single place, in a way that preserves the exact semantics of vanilla knex. However it's somewhat gross and possibly risky to modify the prototype function, and the current AsyncLocalStorage has performance limitations.

/* eslint-disable @typescript-eslint/no-var-requires */
const Transaction = require('knex/lib/execution/transaction');

function monkeyPatchTaggedCommit(): void {
  Transaction.prototype.commit = function (conn: any, value: any) {
    const tag = asyncLocalStorage.getStore();

    const commitStatement = tag 
      ? `/* ${tag} */ COMMIT;`
      : 'COMMIT;';

    return this.query(conn, commitStatement, 1, value);
  };
}

Two options I can think of to do this cleaner:

  1. Allow comments to be attached to the next query statement execution. I have no idea how this would look as I didn't investigate this part of the knex source code, but something like flushing the comments of the queryBuilder into the automated COMMIT/ROLLBACK functions.
  2. Attach the trx and connection objects to the transactor context, such that it's easy to replace the instance commit method of the transactor object. This would not be a first party feature, but would allow easier patching of these functions.
  3. Something else that I missed?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant