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

create beginPrepared function #628

Merged
merged 9 commits into from
Jul 2, 2023

Conversation

shayan-shojaei
Copy link
Contributor

@shayan-shojaei shayan-shojaei commented Jul 2, 2023

Added a new function called beginPrepared to support prepared transactions.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

My idea was that .prepare() would be available on the sql transaction instance, so you would still use .begin()

Like this:

await sql.begin(async sql => {
  ...queries
  await sql.prepare(name)
})

@shayan-shojaei
Copy link
Contributor Author

This would have been a great option if running other queries after the prepare function was an option, but considering that it's not, I would imagine this would be a more elegant option.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

There's nothing preventing us from implementing it such that you can? sql.prepare(name) can simply be a function that caches the name and signals that instead of commit we should do prepare transaction name

@shayan-shojaei
Copy link
Contributor Author

So your idea is that sql.prepare(name) does not instantly prepare the transaction? Instead it signals that the transaction should end with a prepared transaction?

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

The cool thing is that you'll also be able to conditionally decide if you want to prepare or commit the transaction depending on the result of queries in the transaction.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

So your idea is that sql.prepare(name) does not instantly prepare the transaction? Instead it signals that the transaction should end with a prepared transaction?

Exactly.

@shayan-shojaei
Copy link
Contributor Author

Yes that would be nice, I'll implement it like so.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

Cool.. Mind adding tests and documentation for this new method?

@shayan-shojaei
Copy link
Contributor Author

Cool.. Mind adding tests and documentation for this new method?

done

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

Looks good so far. Perhaps the test should open another connection and try to commit the prepared transaction?

@shayan-shojaei
Copy link
Contributor Author

Looks good so far. Perhaps the test should open another connection and try to commit the prepared transaction?

Yeah I just thought of that. See if the current test would suffice.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

🎉 perfect.. Seems we need to enable prepared transactions in the bootstrap script for tests to work.

Also - very curious what scenario you're in - needing this feature 😋 sounds exotic 🏝️

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

Add this line to bootstrap.js
exec('psql', ['-c', 'alter system set max_prepared_transactions to 10'])

@shayan-shojaei
Copy link
Contributor Author

🎉 perfect.. Seems we need to enable prepared transactions in the bootstrap script for tests to work.

Also - very curious what scenario you're in - needing this feature yum sounds exotic desert_island

We're trying to implement distributed transactions using the 2PC method. =)

@shayan-shojaei
Copy link
Contributor Author

Add this line to bootstrap.js exec('psql', ['-c', 'alter system set max_prepared_transactions to 10'])

I don't think this has enabled them.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

No, seems it can only be set at start, so we need to edit postgresql.conf and restart 😐

@shayan-shojaei
Copy link
Contributor Author

shayan-shojaei commented Jul 2, 2023

No, seems it can only be set at start, so we need to edit postgresql.conf and restart neutral_face

I think that could be handled if a line is added to the test.yml file to add that line to the postgresql.conf file at start.

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

I'd like an idempotent solution in the bootstrap.js script since I run tests most locally (and that ought to fix github as well). I'm looking into a solution ;)

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

I remember why i hated this now 😮‍💨 Doing a cross platform way of restarting the server is quite annoying 😅 I think for now saying you have to do this locally yourself is ok, and the following line to replace your's in the .yml ought to do the trick for github actions.:

sudo sed -i 's/.*max_prepared_transactions.*/max_prepared_transactions = 100/' /etc/postgresql/${{ matrix.postgres }}/main/postgresql.conf

@shayan-shojaei
Copy link
Contributor Author

shayan-shojaei commented Jul 2, 2023

I remember why i hated this now 😮‍💨 Doing a cross platform way of restarting the server is quite annoying 😅 I think for now saying you have to do this locally yourself is ok, and the following line to replace your's in the .yml ought to do the trick for github actions.:

sudo sed -i 's/.*max_prepared_transactions.*/max_prepared_transactions = 100/' /etc/postgresql/${{ matrix.postgres }}/main/postgresql.conf

Yeah I can't think of a pretty way of doing that either. 😂

@porsager
Copy link
Owner

porsager commented Jul 2, 2023

Everything looks good now 👏

@porsager porsager merged commit 8f6f4e3 into porsager:master Jul 2, 2023
porsager pushed a commit that referenced this pull request Jul 2, 2023
* create beginPrepared function

* change implementation to new method

* add prepare method type to TransactionSql

* add documentations and test

* fix test

* enable prepared transactions in the bootstrap script

* enable prepared transactions in the github actions setup file

* fix github actions

* fix github actions yml file
porsager added a commit that referenced this pull request Jul 5, 2023
* Initial support for cloudflare

* Types here are not needed

* Include cloudflare in npm

* Allow crypto to be async to support WebCrypto polyfills

* Polyfill crypto with WebCrypto for cloudflare

* Use crypto polyfill for cloudflare

* Not ready for tests on CF yet

* build

* build cf

* build

* README.md - improve the "Multiple statements in one query" section

- add links for the official documentation
- escape the backtick character
- change the subtitle to "await sql``.simple()" instead of "await sql`select 1; select 2;`.simple()" (to be coherent with the other subtitles)
- add a small example below

* Ensure number options are coerced from string - fixes #622

* Add sql.reserve method

* build

* create beginPrepared function (#628)

* create beginPrepared function

* change implementation to new method

* add prepare method type to TransactionSql

* add documentations and test

* fix test

* enable prepared transactions in the bootstrap script

* enable prepared transactions in the github actions setup file

* fix github actions

* fix github actions yml file

* Please the linter

* build

* Fix for using compatibility_flags = [ "nodejs_compat" ] instead

* build

* please eslint

* draft: Cloudflare works ! 🎉  (#618)

* Reworked from source cloudflare branch
feat: reran transpile
fix linter
feat: final touches + test files

squashed 2 commits
fix: Polyfills bulk (to please linter)
fix: Removed MD5 + put back SHA in the digest()

squashed 5 commits
fix: cloudflare workers deployment
feat: fixed auth
fix: encrypt not found in worker :(
fix: postgres SASL
fix: linting

* fix: merge cleanup

---------

Co-authored-by: wackfx <[email protected]>

* Switch to performance.now

* Please the linter

* Don't collect polyfills (keep line numbers similar to src)

* Simplify manual test script

* build

---------

Co-authored-by: Paulo Vieira <[email protected]>
Co-authored-by: Shayan Shojaei <[email protected]>
Co-authored-by: Wack <[email protected]>
Co-authored-by: wackfx <[email protected]>
Copy link
Contributor

@Minigugus Minigugus left a comment

Choose a reason for hiding this comment

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

Sorry I'm a month late... 😓 Just to report some issues that might be worth to fix (it's still on master as of now)

types/index.d.ts Show resolved Hide resolved
src/index.js Show resolved Hide resolved
README.md Show resolved Hide resolved
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.

3 participants