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 additional context specific sql interfaces #768

Merged
merged 4 commits into from Dec 2, 2019

Conversation

kylejbrock
Copy link
Contributor

No description provided.

@basvanbeek
Copy link

Currently context methods on Conn are provided in the conn_go18.go file. I think it's best if you move these stmt context methods there too.

A next PR could potentially merge the conn*.go files as it seems minimum required version for lib/pq is Go 1.9.

@kylejbrock
Copy link
Contributor Author

@basvanbeek Are there any additional changes you'd like to see?

@twpayne
Copy link

twpayne commented Mar 29, 2019

Any update on this? I tried to use a prepared COPY IN statement inside a context-aware transaction. When the context is canceled, github.com/lib/pq deadlocks (there's a race condition found by the Go race detector). The underlying reason seems to be that github.com/lib/pq doesn't have this PR merged :)

Code to reproduce the race condition is here.

@prksu
Copy link

prksu commented Sep 7, 2019

any update? i hope this PR to be merged

@jalan
Copy link

jalan commented Nov 27, 2019

Is there anything blocking this change?

It is surprising behavior that contexts are supported properly when using DB.ExecContext but not when using Stmt.ExecContext.

@kylebrock
Copy link

🤷

@KillerX
Copy link

KillerX commented Nov 28, 2019

I mean, I understand that the maintainers do not owe us anything, but it would sure be nice to hear back if we can do anything to get this merged!

It appears that there are several "forks" popping up mostly for the purpose of merging this patch, so it's somewhat splitting up the users, which seems like a bad idea...

@maddyblue maddyblue merged commit a2bfbdf into lib:master Dec 2, 2019
@maddyblue
Copy link
Collaborator

I didn't really read it over in detail. Someone tell me if it breaks anything.

@kjbass
Copy link

kjbass commented Dec 2, 2019

This breaks things

# github.com/lib/pq
../../lib/pq/conn_go18.go:213:21: not enough arguments in call to st.cn.cancel
	have ()
	want (context.Context)

@kjbass
Copy link

kjbass commented Dec 2, 2019

Above snippet is the output of executing go get github.com/lib/pq

@maddyblue
Copy link
Collaborator

Thanks. Reverted.

@muhlemmer muhlemmer mentioned this pull request Dec 2, 2019
@kylejbrock
Copy link
Contributor Author

This PR was from 1.5 yrs ago. Give me a second and I'll updated it.

@maddyblue
Copy link
Collaborator

I didn't realize I hadn't re-run the CI tests which have changed a lot since then. Thanks for getting another one together.

@kylejbrock
Copy link
Contributor Author

@mjibson It's been a really long time since I've thought about any of this.

But, here is a PR that compiles: #921

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.

None yet

9 participants