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

Implement Batch query package #2942

Draft
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

iamkhush
Copy link

Attempts to fix #2257


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 👍

@brianc
Copy link
Owner

brianc commented Mar 30, 2023

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

@brianc
Copy link
Owner

brianc commented Mar 30, 2023

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

Copy link
Owner

@brianc brianc 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 😄 )

packages/pg-batch-query/bench.ts Outdated Show resolved Hide resolved
packages/pg-batch-query/src/index.ts Outdated Show resolved Hide resolved
}
catch(err) {
process.nextTick(() => {
throw err
Copy link
Owner

@brianc brianc Mar 31, 2023

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?

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

Copy link
Author

Choose a reason for hiding this comment

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

added test-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 commented May 2, 2023

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 commented May 16, 2023

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

packages/pg-batch-query/package.json Outdated Show resolved Hide resolved
Copy link
Contributor

@abenhamdine abenhamdine May 16, 2023

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 ?

@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 ?

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

iamkhush commented Jun 6, 2023

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

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

@iamkhush
Copy link
Author

iamkhush commented Jun 6, 2023

Moving it to draft to add typescript definitions

@hillac
Copy link

hillac commented Jul 25, 2024

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 was asked above

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.

Extended Query: Support Batch Execution
5 participants