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

fix stmtcache invalidation #865

Merged
merged 1 commit into from
Nov 12, 2020

Conversation

ethanpailes
Copy link
Contributor

This patch fixes #841. The meat of the fix lives
in a PR to the pgconn repo. This change just checks
for errors after executing a prepared statement and informs
the underlying stmtcache about them so that it can properly
clean up. We don't try to get fancy with retries or anything
like that, just return the error and allow the application to handle it.

I had to make some changes to to the jackc/pgconn package as well
as this package, so I've retargeted this this repo on
ethanpailes/pgconn for the time being. This PR is not really in a
mergable state until those PRs merge and I rip out the retargeting
commit, but I would say it is ready for review (just review the most recent commit).

Fixes #841

@ethanpailes ethanpailes changed the title fix stmtcache invalidation [READY FOR REVIEW] [DO NOT MERGE] fix stmtcache invalidation Nov 9, 2020
@jackc
Copy link
Owner

jackc commented Nov 11, 2020

I think it would be possible to call StatementErrored in rows.Close. That would avoid needing to expose ResultReader.Err as I mentioned in the other PR.

Also, I think you could use a replace directive in go.mod to avoid having to rewrite all the import paths while you're working in development.

@ethanpailes ethanpailes force-pushed the ep/fix-stmtcache-invalidation branch from e24cfe4 to d4a5199 Compare November 11, 2020 16:41
@ethanpailes
Copy link
Contributor Author

ethanpailes commented Nov 11, 2020

I think it would be possible to call StatementErrored in rows.Close. That would avoid needing to expose ResultReader.Err as I mentioned in the other PR.

Good idea, I've done that and it works great. (I've updated the most recent patch so you can still review it all as one diff and ignore my big re-targeting diff). I did have to add a backreference to the connection the rows was spawned from though.

Also, I think you could use a replace directive in go.mod to avoid having to rewrite all the import paths while you're working in development.

That's a good idea. I'll keep it in mind for next time!

@ethanpailes
Copy link
Contributor Author

Just noticed that rows.Next calls rows.Close on error, so we should be covered there as well!

@jackc
Copy link
Owner

jackc commented Nov 11, 2020

This should be ready once it is pointed back at jackc/pgconn (point it at commit cba610c) and the StatementErrored call signature is changed.

@ethanpailes ethanpailes force-pushed the ep/fix-stmtcache-invalidation branch from d4a5199 to 8347332 Compare November 12, 2020 13:13
@ethanpailes ethanpailes changed the title [READY FOR REVIEW] [DO NOT MERGE] fix stmtcache invalidation fix stmtcache invalidation Nov 12, 2020
This patch fixes jackc#841. The meat of the fix lives
in [a PR to the pgconn repo][1]. This change just checks
for errors after executing a prepared statement and informs
the underlying stmtcache about them so that it can properly
clean up. We don't try to get fancy with retries or anything
like that, just return the error and allow the application to handle it.

I had to make [some][1] [changes][2] to to the jackc/pgconn package as well
as this package.

Fixes jackc#841

[1]: jackc/pgconn#56
[2]: jackc/pgconn#55
@ethanpailes
Copy link
Contributor Author

@jackc I think this should be good to go now.

@jackc jackc merged commit 1df45d7 into jackc:master Nov 12, 2020
@jackc
Copy link
Owner

jackc commented Nov 12, 2020

Thanks!

@eh-steve
Copy link

🎉

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.

connection local statement cache does not correctly invalidate
3 participants