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

Wait for schema changes on execute and batch #206

Merged
merged 46 commits into from
May 30, 2024
Merged

Conversation

giovannibenussi
Copy link
Contributor

@giovannibenussi giovannibenussi commented May 15, 2024

This PR adds an option to batch to allow to wait until all schema changes are done:

await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column text"], {
  wait: true,
});

Currently, batch supports a transaction mode parameter, so I keep support for it but also added a new object parameter that supports a transactionMode property. The code below is equivalent:

// Previous API, still supported
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], " write")

// New API
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], {
  transactionMode: "write"
});

// You can mix transaction mode and the new wait option
await schemaClient.batch(["ALTER TABLE users ADD COLUMN test_column_2 number"], {
  transactionMode: "write",
  wait: true,
});

You can test these changes locally with this command:

cd packages/libsql-core && npm run build && cd - && cd packages/libsql-client && npm run build && cd - && node test.ts

It builds and run a test using the new wait parameter while I add automatic tests.
Please note that the API returns frequent 500 errors which are being investigated internally.

If everything works as expected, you should see an output similar to the one below.

Waiting for migration jobs
json: {
  schema_version: 4,
  migrations: [
    { job_id: 2, status: 'RunSuccess' },
    { job_id: 1, status: 'RunSuccess' }
  ]
}
lastMigrationJob: { job_id: 2, status: 'RunSuccess' }
Finished waiting for migration jobs

@giovannibenussi
Copy link
Contributor Author

I decided to move (at least for now, we can still discuss this!) the code to wait for schema changes to the execute function as an optional object parameter, so if you want to run a schema migration, you can run a command like this:

await schemaClient.execute(
  "ALTER TABLE users ADD COLUMN test_column number;",
  { wait: true },
);

I think this is a better approach than adding the parameter to batch because the batch function wraps the whole SQL code in a transaction, which makes the schema SQL to fail in the server. This means that we need to remove the transaction from the SQL code if { wait: true }, which makes all these combinations equivalent:

// All these operations would be the same
batch('...', { transactionMode: 'write', wait: true })
batch('...', { transactionMode: 'read', wait: true })
batch('...', { transactionMode: 'deferred', wait: true })

This is not bad per se, but having a configuration object parameter with options that can't be mixed doesn't seem the correct way to implement this because it may cause confusion to users. For now, I moved the implementation to execute, since it seems a better place to implement this logic. However, it's quite easy to move it to any other function in case we want to. If there's anything wrong with this please let me know!

I also added some tests for the waitForLastMigrationJobToFinish using msw to simulate responses for the job-related endpoints and make sure that it works as expected.

I've been testing these changes with my own databases using the test file as a reference and it's working as expected, so if everything looks good, I just need to remove the test.ts file and the extra console.log calls!

@haaawk haaawk merged commit fc8d3db into main May 30, 2024
3 checks passed
@giovannibenussi
Copy link
Contributor Author

Update about the final implementation: we don't require users to manually provide a new parameter like { wait: true }.
We determine automatically if a database is a schema database or not by calling an API endpoint so users are not required to do manual changes in order to make this work!

@giovannibenussi giovannibenussi changed the title Add option to wait for schema changes to batch Wait for schema changes on execute and batch May 30, 2024
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

Successfully merging this pull request may close these issues.

2 participants