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

Ignore already removed deallocated prepared statement headers #249

Merged
merged 1 commit into from
Oct 11, 2022

Conversation

mdesmet
Copy link
Contributor

@mdesmet mdesmet commented Oct 6, 2022

Description

Resolves #248

X-Trino-Deallocated-Prepare may appear on multiple responses. Query execution should not fail on prepared statement removal if the prepared statement has already been removed.

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
(x) Release notes are required, with the following suggested text:

* Fix failure on already removed deallocated prepared statement headers

@cla-bot cla-bot bot added the cla-signed label Oct 6, 2022
@mdesmet mdesmet requested a review from hashhar October 6, 2022 12:49
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

LGTM. @findepi is the server supposed to be doing this?

trino/client.py Outdated Show resolved Hide resolved
@findepi
Copy link
Member

findepi commented Oct 6, 2022

is the server supposed to be doing this?

i don't think it is.

from the issue

Steps To Reproduce
I wasn't able to reproduce this locally, only in CI. So I assume some type of race condition is happening.

this is concerning a bit. the client protocol is "single threaded", i.e. there is typically exactly one ongoing request.

@@ -607,7 +607,7 @@ def process(self, http_response) -> TrinoStatus:
for name in get_header_values(
http_response.headers, constants.HEADER_DEALLOCATED_PREPARE
):
self._client_session.prepared_statements.pop(name)
self._client_session.prepared_statements.pop(name, None)
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a debug log or a warnings.warn here since ideally this should not be happenning at all. We should try to collect when this happens and why and fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Useful information to log would be the statement we are trying to deallocate, the current state of the prepared_statements dict and the response headers we received.

@mdesmet
Copy link
Contributor Author

mdesmet commented Oct 9, 2022

@hashhar : An alternative solution would be to implement the deallocated prepared statements as a set instead and merge them when performing a request (as in the JDBC driver). The way it's done in JDBC makes me think that this behaviour might actually not be erroneous but I'm still investigating if this is the case or not.

@hashhar hashhar merged commit 196188e into trinodb:master Oct 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

KeyError raised on prepared statement deallocate
3 participants