Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Implement Batch query package#2942

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Draft
iamkhush wants to merge20 commits intobrianc:master
base:master
Choose a base branch
Loading
fromiamkhush:batch-query-package

Conversation

iamkhush
Copy link

Attempts tofix#2257

proddata, abenhamdine, and cesco69 reacted with heart emoji

The MIT License (MIT)

Copyright (c) 2013-2020 Ankush Chadda
Copy link
Author

Choose a reason for hiding this comment

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

Is it okay to have my name in the copyright ? I am not aware if this is the correct thing or not

Copy link
Owner

Choose a reason for hiding this comment

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

yea absolutely okay. you can get boilerplate MIT license text from github or other places 👍

iamkhush reacted with thumbs up emoji
@brianc
Copy link
Owner

whoah this is really awesome! I'm excited to help get this over the finishline & release it.

@brianc
Copy link
Owner

@charmander any thoughts on this? I think it looks p great!

Copy link
Owner

@briancbrianc left a comment

Choose a reason for hiding this comment

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

Some initial comments on things we should change. And needs some tests around errors, error handling, and ensuring the client is still usable after an error (you typically need to flush after an error I believe? Could be wrong there...tests will uncover these things though 😄 )

}
catch(err) {
process.nextTick(() => {
throw err
Copy link
Owner

@briancbriancMar 31, 2023•
edited
Loading

Choose a reason for hiding this comment

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

why catch & throw on next tick? Doesn't this create an uncatchable error which will crash a node process?

iamkhush reacted with thumbs up emoji
assert.strictEqual(response.rowCount, 1)
}
})
})
Copy link
Owner

Choose a reason for hiding this comment

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

would be good to see some tests for errors / error handling / error recovery

iamkhush reacted with thumbs up emoji
Copy link
Author

Choose a reason for hiding this comment

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

addedtest-error-handling.ts

@charmander
Copy link
Collaborator

@charmander any thoughts on this? I think it looks p great!

If it’s going to be a separate package instead of an API change to pg’s default query type – why in this repo?

@iamkhush
Copy link
Author

@brianc@charmander please have a look at the PR and provide feedback

@iamkhush
Copy link
Author

iamkhush commentedMay 2, 2023•
edited
Loading

I had to make changes in the types/pg to get the build running again
Here is the PR waiting for the maintainers (@brianc,@charmander )
DefinitelyTyped/DefinitelyTyped#65348

@abenhamdine
Copy link
Contributor

abenhamdine commentedMay 16, 2023•
edited
Loading

waouh, so happy to see that !
thx for your work@iamkhush ! 🎉

iamkhush reacted with heart emoji

Copy link
Contributor

@abenhamdineabenhamdineMay 16, 2023•
edited
Loading

Choose a reason for hiding this comment

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

IMHO, it would be great to add explanations about :

  • the benefits of batched queries vs non-batched
  • the difference with pipelining (which is not implemented yet)
  • perhaps also some hints on the underlying mechanism (multiple BIND etc) for future contributors

I also wonder if any advice about the number of parameters is needed ? is there a hard limit ?

iamkhush reacted with thumbs up emoji
@abenhamdine
Copy link
Contributor

I think this important question remains unanswered

If it’s going to be a separate package instead of an API change to pg’s default query type – why in this repo

in other words : are maintainers of this repo willing to take responsability for this new package ?

@iamkhushiamkhush marked this pull request as draftJune 6, 2023 06:12
@iamkhush
Copy link
Author

In other words : are maintainers of this repo willing to take responsability for this new package ?

@abenhamdine I have followed what@brianc advised mehere.
But ofcourse it will be good to have confirmation again

@iamkhush
Copy link
Author

Moving it to draft to add typescript definitions

@hillac
Copy link

Is this still alive / any help needed to get this done? I would like to use this myself.

@iamkhush
Copy link
Author

Hey@hillac , happy to continue the work if maintainers would confirm on what wasasked above

hillac reacted with thumbs up emoji

@hillac
Copy link

hillac commentedMar 13, 2025•
edited
Loading

Given this is a fully separate package, I can just publish and use this on its own without having to maintain my own fork of node postgres right? I might do that.

Edit: tried it out on some micro benchmarks. For updates I saw no real advantage compared to building a single big parametrized query. And for more than 50 rows, the big parametrized query is significantly faster. For inserts I'm seeing worse results compared to a big parametrized query.

@iamkhush
Copy link
Author

@hillac could you show the code where it didnt work for you ? Im curious since the bench.ts file showed triple the qps I had seen otherwise.

@hillac
Copy link

hillac commentedMar 24, 2025•
edited
Loading

https://github.com/hillac/pg-batch-bench
Pulled the benchmark out into it's own repo. Example output in the readme.

I should change it to do the inserts and updates multiple times to get some percentiles and means, but you can pretty clearly see a trend that just building a large parameterized query is faster.

The batch is much nicer to use though. And might be a bit faster for single inserts, but that could just be a jit warmup thing. Once again, needs better statistics.

(Edit: Added some more iterations of inserting 1, 2 and 3 rows and batch is definitely slower, so likely just a jit warmup issue.)

I think your benchmark compared multiple round trip single inserts/ updates, which is slower. This benchmark does a single query with all the rows in it.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@briancbriancbrianc left review comments

@abenhamdineabenhamdineabenhamdine left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extended Query: Support Batch Execution
5 participants
@iamkhush@brianc@charmander@abenhamdine@hillac
[8]ページ先頭
©2009-2025 Movatter.jp