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

expose err attached to ResultReader #55

Closed
wants to merge 1 commit into from
Closed

expose err attached to ResultReader #55

wants to merge 1 commit into from

Conversation

ethanpailes
Copy link
Contributor

This is another change meant to support the fix for jackc/pgx#841

@ethanpailes
expose err attached to ResultReader
4c31450 
This is another change meant to support the fix for jackc/pgx#841
ethanpailes added a commit to ethanpailes/jackc-pgx that referenced this pull request Nov 2, 2020
@ethanpailes
fix prepared statement cache invalidation
4b88364 
This patch fixes jackc#841. To fix it we notice
when a statement failed because its cached plan is now
invalid and flush it from our cache. We don't try to get
fancy with retries or anything like that, just return the
error and allow the application to handle it.

The flushing of statements is deferred to the end of transactions
because all statements executed after an error in a transaction
will just result in errors. If we are not in a transaction, we
flush eagerly.

I had to make [some][1] [changes][2] 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 2f31e98, but I
would say it is ready for review (just review the most recent commit).

One question I have is whether anything else needs to be done around
batch SQL, so guidance there would be appreciated.

Fixes jackc#841

[1]: jackc/pgconn#54
[2]: jackc/pgconn#55
@ethanpailes ethanpailes mentioned this pull request Nov 2, 2020
Closed
@ethanpailes
Copy link
Contributor Author

Closing in favor of #56

@ethanpailes ethanpailes closed this Nov 9, 2020
@ethanpailes
Copy link
Contributor Author

Nevermind, I actually still need this guy. Reopening.

@ethanpailes ethanpailes reopened this Nov 9, 2020
ethanpailes added a commit to ethanpailes/jackc-pgx that referenced this pull request Nov 9, 2020
@ethanpailes
fix stmtcache invalidation
e24cfe4 
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, 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 jackc#841

[1]: jackc/pgconn#56
[2]: jackc/pgconn#55
This was referenced Nov 9, 2020
Merged
Closed
@jackc
Copy link
Owner

jackc commented Nov 11, 2020

If reasonably possible, I'd like to avoid this. I suppose I haven't documented it very well but there is an implicit requirement that nothing should be called on a ResultReader after it is closed. The reason is there is only one ResultReader per *PgConn -- it is reused. I think this exposure and its corresponding usage in pgx is safe ... but I'd rather avoid even the possibility of error.

ethanpailes added a commit to ethanpailes/jackc-pgx that referenced this pull request Nov 11, 2020
@ethanpailes
fix stmtcache invalidation
d4a5199 
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, 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 jackc#841

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

Closing, we didn't end up needing it

@eh-steve
Copy link

We actually need this change to allow access to the last ResultReader error encountered during NextRow().

At the moment, pgx is swallowing a reader error (ERROR: cached plan must not change result type (SQLSTATE 0A000)) during iteration on rows, since there's no mechanism for the rows.err to store the last reader error, and you can only hit this error once you've started iterating.

jackc/pgx#870 and #58 cover our required changes to get around this problem, though I'd be open to any other suggestions

@ethanpailes
Copy link
Contributor Author

@eh-steve have you tried checking rows.Err() after calling rows.Next()? This error is definitely showing up in the tests I've written.

ethanpailes added a commit to ethanpailes/jackc-pgx that referenced this pull request Nov 12, 2020
@ethanpailes
fix stmtcache invalidation
8347332 
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 added a commit to ethanpailes/jackc-pgx that referenced this pull request Nov 12, 2020
@ethanpailes
fix stmtcache invalidation
1df45d7 
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
@eh-steve
Copy link

@eh-steve have you tried checking rows.Err() after calling rows.Next()? This error is definitely showing up in the tests I've written.

Yeah, that works in jackc/pgx#865 but not before that. I'm happy to close both my PRs as soon as jackc/pgx#865 is merged as it addresses my problem

@eh-steve
Copy link

eh-steve commented Nov 12, 2020 •
edited
Loading

(By the way, it might be an idea to use the replace directive in the go.mod files to avoid PRs being bloated by importing a different package name, like in https://github.com/vivacitylabs/pgx/blob/expose-result-reader-err/go.mod#L22)

@ethanpailes
Copy link
Contributor Author

Yeah, that works in jackc/pgx#865 but not before that. I'm happy to close both my PRs as soon as jackc/pgx#865 is merged as it addresses my problem

I suspect that jackc is going to want to merge jackc/pgx#865 because he has expressed reservations about exposing the error from the result reader due to concerns about use-after-free (well not really because go has a GC, but it sounds like the allocation is cached and re-used).

(By the way, it might be an idea to use the replace directive in the go.mod files to avoid PRs being bloated by importing a different package name)

Yeah, jackc pointed that out to me as well. I didn't know about the technique before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ethanpailes @jackc @eh-steve