Add configurable query timeout#1760
Conversation
Correct test name
|
@brianc Any feedback on this? |
|
Nice! I have briefly tested my code with query timeout enabled in production and it seemed to work fine. Eventually I found that the issue was caused by misconfigured tcp proxy, and decided not to pursue this. Regardless, this could be very useful to recover stuck TCP sessions due to lossy network conditions @brianc. Also this is completely optional, and the lib will function as it did without the setting enabled. Please note that if PR is accepted, documentation needs to be updated here: https://node-postgres.com/api/client |
|
@juliusza Is that documentation in some other repository? |
Unfortunately, so far the docs are not available for contributions. |
I'm thinking of porting them to https://www.gitbook.com so it's easier to contribute. |
|
@brianc what about this PR? |
|
Have the flu in bed. Will check as soon as im back in my feet. Havenf
forgotten
…On Thu, Nov 8, 2018 at 5:09 AM André Cruz ***@***.***> wrote:
@brianc <https://github.com/brianc> what about this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1760 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADDoe1ZykszVdKkvPkTzZm1HZCefeRLks5utBDYgaJpZM4YL6SO>
.
|
Winter is coming 😷 |
|
@brianc how’s that flu coming along? :) |
|
The flu suuuuuuuuuuuuuuucked! Thanks for asking. 😄 It cleared up early last week & now I just have left-over bronchitis. Definitely going to get the flu shot from here on out. Now...lemme look at this PR! Sorry for the review (thanks for pinging me gently on it!) |
brianc
left a comment
brianc
left a comment
There was a problem hiding this comment.
This is great, thanks! I'll get this merged & push out a new minor version today. ❤️
That's great 👍 Could you also bump the version of pg-pool to 2.0.4, in order to be able to benefit from brianc/node-pg-pool@1871d0f ? thx ! |
|
@abenhamdine you bet! |
|
I'm looking forward to this functionality |
|
@brianc Hello! Any news on this? |
|
omg totally slipped my mind....releasing now |
|
@brianc Is pg-pool bump also happening? |
|
@kibertoad yah definitely, right after this published |
|
pg-pool is already published at |
|
@brianc It's listed in package.json as |
|
ohhh duh yeah i can fix that no problem, sorry
…On Thu, Nov 29, 2018 at 9:22 AM Igor Savin ***@***.***> wrote:
@brianc <https://github.com/brianc> It's listed in package.json as "pg-pool":
"~2.0.3" which means that projects with package-lock.json are not going
to pick it up unless you regenerate entire lock file.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1760 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADDoUsHCPN2w2GUaCaZuTd-d5Q8NfS1ks5uz_usgaJpZM4YL6SO>
.
|
|
should I just change it to |
|
hmmm wait the |
|
K published |
|
great, thx @brianc ! |
|
Thank you! |
|
Does this actually cancel the query in the server once it timedout in the client side? isn't clear from the diff. Thank you! |
|
Hey @jfromaniello, this does not cancel query on the server. It would be reasonable to configure both query_timeout and statement_timeout, making the statement_timeout slightly longer so that it times out right after the client stopped waiting for response. |
|
@juliusza thank you very much! |
|
How is the documentation for this feature coming? The |
|
I recently open-sourced the docs!
https://github.com/brianc/node-postgres-docs . so feel free to add any
documentation you want over there!
…On Tue, Jul 16, 2019 at 12:48 PM johan13 ***@***.***> wrote:
How is the documentation for this feature coming?
The query_timeout property is missing from the typescript typings, and
the typings seem to be based on the docs, so I thought I'd start here
before I put in a merge request to DefinitelyTyped
<https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/pg>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1760?email_source=notifications&email_token=AAAMHIIZLQX7KIGNWLMYVI3P7YCXLA5CNFSM4GBPUSHKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2BUB7Y#issuecomment-511918335>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAMHIPGUNBTDWPG75CMTNLP7YCXLANCNFSM4GBPUSHA>
.
|
I am quite sure that it should be the other way around: the ` |
|
Yes @tmtron, make sense to have
I found that sometimes I get this error on production. Because pg lib is event based, there's no proper stack trace to work with. Obviously we haven't thought of a certain edge case here, that causes above error to crash node process. I no longer need the query-timeout setting, so I don't think I'll want to spend time working on a fix. |
This option has existed in pg since v7.7 - see discussion at brianc/node-postgres#1760 Adding to properties to be passed to new connection config so it can be used with sequelize
This option has existed in pg since v7.7 - see discussion at brianc/node-postgres#1760 Adding to properties to be passed to new connection config so it can be used with sequelize
10 participants
Addresses #1713
Based on work from @juliusza