More fine-grained transaction API with savepoints?

[koskimas]

try {
  const trx = await db.begin().isolationLevel('serializable').execute()
  await trx.insertInto(...).execute()

  await trx.savepoint('foobar')
  try {
    await trx.updateTable(...).execute()
    await trx.updateTable(...).execute()
  } catch (err) {
    await trx.rollbackToSavepoint('foobar')
  }

  await trx.commit()
} catch (err) {
  await trx.rollback()
}

[igalklebanov]

Great idea! 💡

• I personally prefer .startTransaction() over .begin(), clearer intent. The first is usually aliased by the latter in documentations.

• We should probably also support .releaseSavepoint(name).

• It's weird that some of the methods fire immediately without .execute(). Let's allow compiling and executing these with the usual methods?

• Does this replace current API, or adds another variant (lets say we rename .transaction() to .begin())? Could the following strategy be viable? current API is applicable when passing a callback as argument to .execute(...), while this newer more hands-on mode is applicable when passing no arguments at all...

[koskimas]

Excellent points! I didn't originally add the execute() calls since you can't really do anything but execute them, but that's not consistent.

So here's the revised API:

try {
  const trx = await db.startTransaction().setIsolationLevel('serializable').execute()
  await trx.insertInto(...).execute()

  await trx.savepoint('foobar').execute()
  try {
    await trx.updateTable(...).execute()
    await trx.updateTable(...).execute()
    await trx.releaseSavepoint('foobar').execute()
  } catch (err) {
    await trx.rollbackToSavepoint('foobar').execute()
  }

  await trx.commit().execute()
} catch (err) {
  await trx.rollback().execute()
}

Does this replace current API

We could still provide the old API for convenience. Most of the time people would probably still want to use that one.

[igalklebanov]

Another good thing about this, is that it allows compiling transactions, e.g. for display in the browser.
We had someone ask about this a while back on discord..

[koskimas]

Actually that's not the case 🤔 We have this API in the driver:

  /**
   * Begins a transaction.
   */
  beginTransaction(
    connection: DatabaseConnection,
    settings: TransactionSettings
  ): Promise<void>

  /**
   * Commits a transaction.
   */
  commitTransaction(connection: DatabaseConnection): Promise<void>

  /**
   * Rolls back a transaction.
   */
  rollbackTransaction(connection: DatabaseConnection): Promise<void>

Dialects have the ability to start transactions however they want. For example the kysely-data-api does a "start transaction" HTTP request instead of running a BEGIN SQL command. Same for commit and rollback.

We can't allow those to be compiled since some dialects in fact don't compile anything.

[igalklebanov]

I see, yep that wouldn't work for such dialects..

Can't we make the new API fully compilable, not requesting directly from these methods in the driver? So dialects that actually perform a sql query to begin a transaction can enjoy this..

[koskimas]

I don't see how? What would compile return for dialects that just call a method and don't execute any SQL?

[igalklebanov]

Not sure, maybe dummy response such as { sql: 'NOT_COMPILABLE', parameters: [], query: { kind: 'StartTransactionNode' }}?

[igalklebanov]

Could we make these driver methods optional?

• If they are not implemented (which should be the case for all 3 built-in dialects), beginning a transaction (the old API) defaults to creating a StartTransactionNode, compiling and executing.
• If they are implemented, compilation will just return some dummy response, or even throw..

If this makes sense, a rename that describes this "non-sql begin transaction method" situation would make sense.

[koskimas]

Optional driver methods could make sense. But on the other hand, jumping through too many hoops to allow the user to compile things that will always be the string commit or rollback doesn't seem too important.

Also: MySQL's beginTransaction runs two queries currently:

    if (settings.isolationLevel) {
      // On MySQL this sets the isolation level of the next transaction.
      await connection.executeQuery(
        CompiledQuery.raw(
          `set transaction isolation level ${settings.isolationLevel}`
        )
      )
    }

    await connection.executeQuery(CompiledQuery.raw('begin'))

unlike PostgreSQL, MySQL doesn't allow isolation level to be set in the same query as begin. BUT the set transaction isolation level query must be executed on the same connection BEFORE starting the transaction. Making things compilable, we'd need to implement an API like this for mysql:

const cn = await db.getConnection();
await cn.setTransactionIsolationLevel('serializable').execute()
const ctx = await cn.startTransction().execute()
...
await cn.release();

which is a little bit too detailed?

[igalklebanov]

Yeah this is not really worth it, for just 1-2 sql statements that are very predictable. Our consumers could simply prepend them manually if they want to display them. 👍

[nikeee]

JS is set to get explicit resource management:
https://github.com/tc39/proposal-explicit-resource-management (reached stage 3).

There is also an async version in planning:
https://github.com/tc39/proposal-async-explicit-resource-management

I think transactions could really benefit from this. They are also listed as a use case (example taken from the proposal, our implementation may be different):

async function transfer(account1, account2) {
  using await tx = transactionManager.startTransaction(account1, account2);
  await account1.debit(amount);
  await account2.credit(amount);

  // mark transaction success if we reach this point
  tx.succeeded = true;
} // await transaction commit or rollback

Maybe this is something what would be interesting for the design of a transaction API in the future. Since the sync version of the proposal was accepted, chances are that we'll also get an async version. :)

[igalklebanov]

JS is set to get explicit resource management: tc39/proposal-explicit-resource-management (reached stage 3).

There is also an async version in planning: tc39/proposal-async-explicit-resource-management

I think transactions could really benefit from this. They are also listed as a use case:

async function transfer(account1, account2) {
  using await tx = transactionManager.startTransaction(account1, account2);
  await account1.debit(amount);
  await account2.credit(amount);

  // mark transaction success if we reach this point
  tx.succeeded = true;
} // await transaction commit or rollback

Maybe this is something what would be interesting for the design of a transaction API in the future. Since the sync version of the proposal was accepted, chances are that we'll also get an async version. :)

This feature is great for javascript, but redundant in Kysely, since the current transaction API already abstracts away commit & rollback so consumers can focus on content.

This issue's purpose is to provide consumers with a more fine-grained control over transactions, to live alongside the current API (not replace it). The demographic for this feature wants to call .commit() and .rollback() themselves.

  tx.succeeded = true;

This is such a footgun. Hate it.

[Alex-Yakubovsky]

This would also be handy for parallelizing automated tests (each in their own transaction) that talk to the database without having them commit the data. Currently, I truncate tables so the tests have to run single order. With finer grained controls over rollbacking back I can rollback after each test.

[rdgout]

Is there any update regarding this?
In our application we would like to use transactions but we would like to perform every query as they come into our streaming system.
The current API would only allow us to batch a list of queries and then perform them all at once by way of

kyselyClient.transaction()
    .execute(async (trx) => {
        queries.map((query) => trx.execute(query))
    });

However, we would prefer to be able to use the connection for the transaction to perform each query as it comes in.
This way we can validate the query is not invalid, by trying to execute it on the connection, and only need to perform the commit when we reach the threshold or a predefined time.

Some pseudocode would be:

class TransactionBatcher {
    addQuery(query: Query) {
        if (!this.transactionHasBegun) {
            this.kyselyClient.beginTransaction();
            this.transactionHasBegun = true;
        }

        try {
             this.kyselyClient.execute(query);
        } catch (e) {
             logger(`Found an error in query: ${query}`);
        }
    }

    commitQueries() {
         try {
             this.kyselyClient.commitTransaction();
         } catch (e) {
             logger('Could not commit query');
             this.kyselyClient.rollbackTransaction();
         }
    }
}

[koskimas]

@rdgout We'll update this issue when we have updates. You can achieve the same thing using the current API with a small helper function:

async function begin(db: Kysely<DB>) {
  const connection = new Deferred<Transaction<DB>>()
  const result = new Deferred<any>()

  // Do NOT await this line.
  db.transaction().execute(trx => {
    connection.resolve(trx)
    return result.promise;
  }).catch(err => {
    // Don't do anything here. Just swallow the exception.
  })

  const trx = await connection.promise;
  
  return {
    trx,
    commit() {
      result.resolve(null)
    },
    rollback() {
      result.reject(new Error('rollback'))
    }
  }
}

export class Deferred<T> {
  readonly #promise: Promise<T>

  #resolve?: (value: T | PromiseLike<T>) => void
  #reject?: (reason?: any) => void

  constructor() {
    this.#promise = new Promise<T>((resolve, reject) => {
      this.#reject = reject
      this.#resolve = resolve
    })
  }

  get promise(): Promise<T> {
    return this.#promise
  }

  resolve = (value: T | PromiseLike<T>): void => {
    if (this.#resolve) {
      this.#resolve(value)
    }
  }

  reject = (reason?: any): void => {
    if (this.#reject) {
      this.#reject(reason)
    }
  }
}

[codepunkt]

@koskimas Is there a way to achieve individual test cases running in transactions? This would have to support nested transactions in case the business logic that's being tested has transactions itself, probably in the form of savepoints.

[subframe7536]

crude but works in sqlite

export function createSavePoint(db: Kysely<any> | Transaction<any>, name?: string) {
  const _name = name || `sp_${new Date().getTime()}`
  return {
    save: async () => {
      await sql`savepoint ${sql.raw(_name)}`.execute(db)
    },
    release: async () => {
      await sql`release savepoint ${sql.raw(_name)}`.execute(db)
    },
    rollback: async () => {
      await sql`rollback to savepoint ${sql.raw(_name)}`.execute(db)
    },
  }
}

await db.transaction().execute(async (trx) => {
  const s1 = createSavePoint(trx)

  await s1.save()
  await trx.insertInto('test').values({ person: `test ${Date.now()}` }).execute()
  
  const s2 = createSavePoint(trx)

  try {
    await s2.save()
    await trx.insertInto('test').values({ person: `test ${Date.now()}` }).execute()
    await s2.rollback()
    // throw new Error('test error')
    s1.release()
  } catch (error) {
    s1.rollback()
  }
})

[shirakawayohane]

@igalklebanov

Is there a way to achieve individual test cases running in transactions? This would have to support nested transactions in case the business logic that's being tested has transactions itself, probably in the form of savepoints.

I think this is a very important perspective. When testing functions that use transactions by utilizing transaction rollback, it is ideal to support nested transactions so as not to interfere with the internal transactions of the function. Therefore, supporting nested transactions, such as wrapping savepoint api, seems ideal to me.

function doSomethingUsingTransaction() {
  db.transaction().execute(tx => {
    ...
  })
}

// test code
export const testTrx = async <T>(fn: (tx: Transaction<DB>) => Promise<T>) => {
  let result: T;
  try {
    await db.transaction().execute(async (tx) => {
      result = await fn(tx);
      throw new TestTrxRollback();
    });
  } catch (e) {
    if (!(e instanceof TestTrxRollback)) throw e;
  }
  return result!;
};

describe('check something', () => {
  it('should rollback', () => {
    testTrx(async (tx) => {
      doSomethingUsingTransaction(tx);
    });
  });
});

[shirakawayohane]

@igalklebanov @koskimas
Do you have any updates or feedback?

[subframe7536]

@shirakawayohane #962

[jeffreypeoples]

@shirakawayohane #962

Curious: are nested transactions going to be possible with that pr? Specifically for my use case I have been testing a server that has itself transactions, but passing in a transaction into my createServer function to act as the parent db connection doesn't work since nested transactions are not allowed. I get "calling the transaction method for a Transaction is not supported." I want to do this so that I can just rollback any changes done while working with integration tests, both mutations performed by the test itself and mutations performed by the server being tested, some of which are themselves transactions. I'd like to just rollback everything after each test, or if the test fails. Is that something this PR will enable? Right now Im manually clearing whatever data is mutated by the tested server. Thx.

[igalklebanov]

@jeffreypeoples

Yes, in the form of savepoints.

[shirakawayohane]

@igalklebanov
While it is true that with Savepoint, we can use a transaction during testing, but roll back all the data under test at the end, savepoint perceives that it is in a transaction, which makes it difficult to abstract the process.

Do you have any answers or initiatives regarding this problem awareness?

[aq1018]

I see #962 is merged.

Can we please have a new release?

[BurningEnlightenment]

Can the new API be extended to support vendor extensions like sqlite3's IMMEDIATE?

[igalklebanov]

Can the new API be extended to support vendor extensions like sqlite3's IMMEDIATE?

Different issue. Probably.

[romanstetsyk]

I'm looking forward to a new release now that #962 is merged.