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

More fine-grained transaction API with savepoints? #257

Open
koskimas opened this issue Dec 26, 2022 · 17 comments · May be fixed by #962
Open

More fine-grained transaction API with savepoints? #257

koskimas opened this issue Dec 26, 2022 · 17 comments · May be fixed by #962
Labels
api Related to library's API enhancement New feature or request mysql Related to MySQL postgres Related to PostgreSQL sqlite Related to sqlite

Comments

@koskimas
Copy link
Member

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()
}
@koskimas koskimas added enhancement New feature or request api Related to library's API labels Dec 26, 2022
@igalklebanov igalklebanov added breaking change Includes breaking changes postgres Related to PostgreSQL mysql Related to MySQL sqlite Related to sqlite labels Dec 26, 2022
@igalklebanov
Copy link
Member

igalklebanov commented Dec 26, 2022

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
Copy link
Member Author

koskimas commented Dec 27, 2022

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 igalklebanov removed the breaking change Includes breaking changes label Dec 27, 2022
@koskimas koskimas changed the title More transparent transaction API with savepoints? More fine-grained transaction API with savepoints? Dec 28, 2022
@igalklebanov
Copy link
Member

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
Copy link
Member Author

koskimas commented Dec 29, 2022

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
Copy link
Member

igalklebanov commented Dec 30, 2022

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
Copy link
Member Author

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

@igalklebanov
Copy link
Member

igalklebanov commented Dec 30, 2022

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

@igalklebanov
Copy link
Member

igalklebanov commented Dec 30, 2022

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
Copy link
Member Author

koskimas commented Dec 31, 2022

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
Copy link
Member

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
Copy link
Contributor

nikeee commented Jan 29, 2023

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
Copy link
Member

igalklebanov commented Jan 29, 2023

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
Copy link

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
Copy link

rdgout commented Jul 24, 2023

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
Copy link
Member Author

koskimas commented Aug 12, 2023

@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
Copy link

@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
Copy link

subframe7536 commented Sep 9, 2023

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()
  }
})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request mysql Related to MySQL postgres Related to PostgreSQL sqlite Related to sqlite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants