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.

Sign up for GitHub

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

Jump to bottom

implement ConnPrepareContext/StmtQueryContext/StmtExecContext interfaces #1047

Merged
merged 1 commit into from
Sep 2, 2021
Merged

implement ConnPrepareContext/StmtQueryContext/StmtExecContext interfaces #1047

merged 1 commit into from
Sep 2, 2021

Conversation

michaelshobbs
Copy link
Contributor

@michaelshobbs michaelshobbs commented Jun 11, 2021 •
edited
Loading

See #1046 for more context. (pun intended)

This is almost a line for line copy of #921 with test cases (thanks @kylejbrock). Please let me know what else needs to be done to get this merged.

Also dropped ci testing of unsupported versions (9.5 and 9.4) per pg docs here and added ci testing of versions 11, 12, and 13.

closes #921
closes #1046

@michaelshobbs michaelshobbs mentioned this pull request Jun 11, 2021
Closed
@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch from 9f33411 to 8cb9b67 Compare June 11, 2021 23:15
@michaelshobbs
Copy link
Contributor Author

@otan @mjibson please let me know if i can be of any more assistance in getting this merged

@michaelshobbs
Copy link
Contributor Author

@otan @mjibson
i know pgx is the preferred path forward. however, i also know there are many folks still using pq and this addition would certainly be a value add for those users. please let me know how i can help move this forward.

thank you 🙏

otan
otan reviewed Aug 18, 2021
View reviewed changes
Copy link
Collaborator

@otan otan left a comment

Choose a reason for hiding this comment

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

My gut with this is the same as #921 (comment) -- there are too many goroutines and channels for me to feel comfortable. But given that we already do this for the conn implementation, maybe we're ok.

Some testing changes needed.

conn_go18.go Outdated Show resolved Hide resolved
conn_test.go Outdated Show resolved Hide resolved
conn_test.go Outdated Show resolved Hide resolved
conn_test.go Outdated Show resolved Hide resolved
@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch 2 times, most recently from 55727bb to b5ce7c4 Compare August 18, 2021 22:46
@otan
Copy link
Collaborator

otan commented Aug 19, 2021

well looks like CI no longer works :\

@michaelshobbs
Copy link
Contributor Author

Yea looks like they deprecated travis-ci.org in favor of travis-ci-com. not sure what needs to happen to move this project over. Let me know if I can help.

@michaelshobbs
Copy link
Contributor Author

Looks like all you have to do is sign up on the .com?

https://blog.travis-ci.com/2021-05-07-orgshutdown

@otan
Copy link
Collaborator

otan commented Aug 19, 2021 •
edited
Loading

i've tried that and resent the webhooks but no bueno, perhaps try git commit --amend --reset-author -a and git push --force your commit to try retrigger it.

i don't have admin permissions to debug this further unfortunately (travis-ci.com doesn't show this repo when i try configure it there), may have to move to a new CI system. /shrug

@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch from 94c98c3 to 200830e Compare August 19, 2021 03:17
@michaelshobbs
Copy link
Contributor Author

I was able to add my fork here: https://travis-ci.com/github/michaelshobbs/pq/builds/235721359

I assume that's because I own it. Who is still an admin on this repo? 😬

@michaelshobbs
Copy link
Contributor Author

@otan where did we end up and is there anything i can do to help?

@otan
Copy link
Collaborator

otan commented Aug 26, 2021

You'd need to find who the admin is :) Idk who it is. Maybe @mjibson knows.

@maddyblue
Copy link
Collaborator

I do not!

@otan
Copy link
Collaborator

otan commented Aug 26, 2021

Well I think the answer to get CI working (and for me to have confidence in merging) we'd need to migrate to Github Actions. I don't have the time to do this, such is maintenance life :\

@maddyblue
Copy link
Collaborator

FYI I previously tried really hard to make github actions work at 8cec546. I was never able to get the pg initial configuration files correctly installed before the pg server started running. But that commit is like almost all there.

@cbandy
Copy link
Contributor

cbandy commented Aug 27, 2021

I was never able to get the pg initial configuration files correctly installed before the pg server started running.

It looks like the scripts should be mounted at /docker-entrypoint-initdb.d. Those scripts can copy the certs, write conf, createdb, etc. It looks like you can rely on $PGDATA rather than hard-coding /var/lib/postgresql/data, too.

@michaelshobbs
Copy link
Contributor Author

@otan @mjibson i'll give this a look this week

@michaelshobbs
Copy link
Contributor Author

@otan @mjibson can one of you approve me for running workflows?

@otan
Copy link
Collaborator

otan commented Aug 31, 2021

gotcha

@michaelshobbs
Copy link
Contributor Author

Sweet! I'll start working on this in the morning

@DoubleDi
Copy link

Hi!
Thank you very much for this PR. Just found out our code has the same problems with cancelling requests from prepared statements. Would be awesome to get this into master ASAP.

@DoubleDi
Copy link

DoubleDi commented Aug 31, 2021 •
edited
Loading

Quick update @michaelshobbs. We have tested your patch (just replaced the go.mod to your fork) and it seems still not to work on prepared statements.

Our usecase: add 10sec timeout to context from and send a db query on a prepared statement which last longer, than 1 minute.

@michaelshobbs
Copy link
Contributor Author

@DoubleDi can you provide some code that demonstrates the incorrect behavior? this PR includes canceling statement due to user request test assertions on stmt.QueryContext(), and stmt.ExecContext(). i'd like to better understand where we might be missing something here. thanks 🙏

@DoubleDi
Copy link

@michaelshobbs I am not exactly sure, but in some cases, this works. I can see the canceling statement due to user request error in the logs. But for some reason, we still have long queries, maybe it's an internal problem. I will let you know if I find out anything.

@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch 3 times, most recently from 200830e to 151165d Compare September 1, 2021 23:09
@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch from 151165d to 0e61f02 Compare September 1, 2021 23:16
@michaelshobbs michaelshobbs mentioned this pull request Sep 1, 2021
Merged
@michaelshobbs
Copy link
Contributor Author

@otan @mjibson #1054

@michaelshobbs michaelshobbs force-pushed the feature/context-interfaces branch from 0e61f02 to 9fa33e2 Compare September 2, 2021 13:28
@michaelshobbs
Copy link
Contributor Author

@otan @mjibson rebased master. this should be good to go now

@otan otan merged commit 8667c6b into lib:master Sep 2, 2021
@DoubleDi
Copy link

DoubleDi commented Sep 2, 2021

@otan thanks! Please also tag a new release

@otan
Copy link
Collaborator

otan commented Sep 2, 2021

v1.10.3, you have to be a tad more patient than 90s :P

@DoubleDi
Copy link

DoubleDi commented Sep 2, 2021

thank you so much!

@jybp jybp mentioned this pull request Mar 9, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otan otan otan 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.

stmt.QueryRowContext doesn't respect canceled context
5 participants
@michaelshobbs @otan @maddyblue @cbandy @DoubleDi