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

feature: Parameterized queries and transaction API #39

Merged
merged 16 commits into from
Mar 25, 2024

Conversation

samwillis
Copy link
Collaborator

@samwillis samwillis commented Mar 11, 2024

Each PGlite instance (in that PR) has two methods to run a query:

  • .query(sql, params): execute a single statement, optionally with params. Returns single result object
  • .exec(sql): execute one or more statements, no params. Returns array of result objects, one for each statement.

I split this into two as it makes it easer to know if you are getting one or multiple result objects. Internally the first uses the extended protocol which only supports a single statement, the latter the simple protocol that can run multiple statements but with no params.

Node: this is a breaking change, what was .query(sql) is now .exec(sql).

It also has a .transaction((tx) => {}) api for interactive transactions. Each tx object also has the same .query and .exec methods, along with a .rollback().

Finally there is also a .execProtocol(msg: uInt8Array) method that allows users to execute raw pg-protocl message.

If anyone wants to try this, there is a build including this linked from: #48

Fixes #17, Fixes #31, Fixes #29, see also: electric-sql/postgres-wasm#3, electric-sql/postgres-wasm#4

@samwillis samwillis changed the title feature: Parameterized queries feature: Parameterized queries and transaction API Mar 14, 2024
@msfstef msfstef self-requested a review March 14, 2024 11:38
@samwillis samwillis marked this pull request as ready for review March 17, 2024 12:25
Copy link
Contributor

@msfstef msfstef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work! I think it's quite clean and easy to follow, I've left a few nits and comments mostly focusing on clarity.

I think the only design decision I'm not sure about is the transaction rollback API. It might be worth getting rid of it and just letting any error result in a rollback and needing the user to handle it. I think no transactions should aim to fail and be rolled back, so any rollbacks should be viewed as the internal preparation for the error handling logic left to the user.

packages/pglite/README.md Outdated Show resolved Hide resolved
packages/pglite/README.md Outdated Show resolved Hide resolved
packages/pglite/README.md Outdated Show resolved Hide resolved
packages/pglite/README.md Outdated Show resolved Hide resolved
packages/pglite/README.md Show resolved Hide resolved
packages/pglite/src/pglite.ts Outdated Show resolved Hide resolved
packages/pglite/src/pglite.ts Outdated Show resolved Hide resolved
packages/pglite/src/pglite.ts Outdated Show resolved Hide resolved
packages/pglite/src/pglite.ts Outdated Show resolved Hide resolved
packages/pglite/README.md Show resolved Hide resolved
@samwillis
Copy link
Collaborator Author

Thanks @msfstef!

I've addressed everything in your review.

For the transactions, .rollback() is now an async call that issues the rollback and then marks the transaction as closed so no further operations can be applied. The user can then continue with their own cleanup and return whatever they want from the transaction call. Could take a quick look at that and see if you think it works well?

@msfstef
Copy link
Contributor

msfstef commented Mar 20, 2024

@samwillis I like the idea of allowing the user to perform their cleanup within the transaction - though the cleanup will still hold the lock without strictly needing it. Might be still worth documenting that transactions get rolled back on any error though regardless of whether rollback() is called.

Looks good, ready to merge!

@AntonOfTheWoods
Copy link

Is there a build of this with the changes somewhere? It would be nice to take it out for a spin!

@samwillis
Copy link
Collaborator Author

Hey @AntonOfTheWoods (and anyone else),

There is a dev build of the latest changes from the tip of my stacked PRs linked on #48. You can download and try it out.

We are planning to do a release early next week after a few final checks.

@samwillis samwillis merged commit 101a41a into main Mar 25, 2024
@samwillis samwillis deleted the samwillis/query-params branch March 25, 2024 10:37
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 18, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 22, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 23, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 25, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 29, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 30, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 30, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
kevin-dp pushed a commit to electric-sql/electric that referenced this pull request Apr 30, 2024
PGlite driver for Electric

Builds on parameterized query support for PGlite:
electric-sql/pglite#39

---------

Co-authored-by: msfstef <msfstef@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants