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 batch query interface #38

Closed
wants to merge 1 commit into from
Closed

add batch query interface #38

wants to merge 1 commit into from

Conversation

skyzh
Copy link
Member

@skyzh skyzh commented Jul 25, 2023

This is a very early draft of non-interactive txn batch query interface and might need some refactor before merging.

neondatabase/neon#4703

TL;DR, we added a new interface:

  const { txn, stmt, sql } = neon(env.NEON_DB_URL);
  await txn([
    stmt`SELECT ${1}::int AS int`,
    stmt`SELECT ${"hello"} AS str`
  ], /* ...options */)

meanwhile, we are also compatible with the original way of handling a query:

  const sql = neon(env.NEON_DB_URL);
  await sql(`SELECT ${1}::int AS int`)

several challenges of the current architecture:

  • we don't have proper type definition for most of the HTTP query interface, and therefore we are using any a lot.
  • there does not seem to be a test framework or something like that where I can write an integration test? and therefore I just prepended my test cases to the latency benchmark.
  • will update README and docs after PR reviews as I feel we might need to discuss what would be the best way to expose this interface.

Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh requested review from jawj, kelvich and conradludgate July 25, 2023 18:31
@skyzh
Copy link
Member Author

skyzh commented Jul 25, 2023

I think we also need to support something like:

  const { txn, stmt, sql } = neon(env.NEON_DB_URL);
  await txn([
    stmt`SELECT ${1}::int AS int`,
    "SELECT 1"
  ], /* ...options */)

...?

@kelvich
Copy link
Contributor

kelvich commented Jul 25, 2023

wonder if it is possible to introduce only one new thing instead of two, e.g. make it possible to write:

  const { txn, sql } = neon(env.NEON_DB_URL);
  await txn([
    sql`SELECT ${1}::int AS int`,
    sql`SELECT ${"hello"} AS str`
  ], /* ...options */)

Right away I don't have any good idea how to implement that except accessing some global state to detect txn context (which would be a mess to recover from errors).

@skyzh
Copy link
Member Author

skyzh commented Jul 25, 2023

wonder if it is possible to introduce only one new thing instead of two, e.g. make it possible to write:

  const { txn, sql } = neon(env.NEON_DB_URL);
  await txn([
    sql`SELECT ${1}::int AS int`,
    sql`SELECT ${"hello"} AS str`
  ], /* ...options */)

Right away I don't have any good idea how to implement that except accessing some global state to detect txn context (which would be a mess to recover from errors).

Of course this is possible by attaching a property to the function object and then extract it. But I feel it would be too tricky for users to understand what's happening...?

@skyzh
Copy link
Member Author

skyzh commented Jul 25, 2023

the way to do that, sql function returns a promise that resolves to the single-flight query result, as well as some properties that can be extracted later by txn call. If await is not placed before sql, then the promise will not be resolved immediately.

@jawj
Copy link
Collaborator

jawj commented Jul 26, 2023

Agree with Stas that it would be good to be able to use the same sql function inside a transaction block as outside.

Also: we need a way to specify the transaction isolation level.

@kelvich
Copy link
Contributor

kelvich commented Jul 27, 2023

But I feel it would be too tricky for users to understand what's happening...?

For me less imports is less burden to understand what each thing does. And txn block around is explicit enough. But that is subjective, right.

the way to do that, sql function returns a promise that resolves to the single-flight query result, as well as some properties that can be extracted later by txn call. If await is not placed before sql, then the promise will not be resolved immediately.

ah, right. forgot that it is async function

Also: we need a way to specify the transaction isolation level.

good point. Also there is read only qualifier for transaction.

@skyzh
Copy link
Member Author

skyzh commented Jul 27, 2023

I just realized that JavaScript promise is different from Rust futures. That is to say, once the promise object is created, the runtime will execute it in the background. Therefore, attaching properties to the promise does not look like a good idea...


It seems that we can create a deferred promise as in https://masteringjs.io/tutorials/fundamentals/promise-create#:~:text=JavaScript%20promises%20are%20%22hot%22%20in,a%20new%20promise%20every%20time. but this will cause problems for error reporting.

@nicksrandall
Copy link
Contributor

nicksrandall commented Jul 27, 2023

I took a stab at this in #39

The api looks like:

const sql = neon(...);
const results = await sql.transaction([
  sql`SELECT ${1}::int AS int`,
  sql`SELECT ${"hello"} AS str`
], /* options */);

Which (I think) achieves all the goals stated above. It's not a breaking change because the single queries worked as before!

skyzh added a commit to neondatabase/neon that referenced this pull request Jul 31, 2023
We will retrieve `neon-batch-isolation-level` and `neon-batch-read-only`
from the http header, which sets the txn properties.
neondatabase/serverless#38 (comment)

---------

Signed-off-by: Alex Chi Z <chi@neon.tech>
@skyzh skyzh closed this Aug 11, 2023
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.

4 participants