-
Notifications
You must be signed in to change notification settings - Fork 155
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
Use EXECUTE IMMEDIATE
for parameterized statement executions
#375
Conversation
Since client knows server version can we do this automatically instead of user having to opt-into it? We should only have the toggle so that if someone wants to manually revert to old PREPARE + EXECUTE + DEALLOCATE they can do so. cc: @findepi is this reasonable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we have existing tests with query text which needs escaping? Since EXECUTE IMMEDIATE accepts query text as a string literal and hence quotes need to be escaped.
How client knows server version? I assume client need to execute |
|
7b5b6e1
to
67563ba
Compare
Some checks are failing, please run |
1c75a46
to
f404520
Compare
I agree that -- if this feature is important -- it should auto-configure. Alternatively, we enable it by default after some time, once we believe only compliant server versions are still in use.
This would work. Of course, server can return any version string, so we need to guess whether this looks like an official Trino number and not fail if it doesn't. Also note that it would be good to avoid additional round-trip to the server to configure the feature. Can we piggy-back on some pre-existing call? Can we auto-enable the feature eg starting from second query on a connection? |
Perhaps my approach of enabling by default is overly aggressive. I was thinking that by causing a failure, we encourage people to upgrade to a new version of Trino, though I realize it would be a frustrating user experience, because it would not be clear from the exception. I was thinking our release notes could document the behavior, but nobody RTFM.
Right, the issue we've been trying to address with this change is reducing the number of round-trips. I am not sure how we could piggy-back on an existing call. We could get the version before executing the first statement on a connection and cache the result. Is there a smarter way to do it? Note that we've seen some usage patterns that don't reuse connections (e.g. because they execute statements with different roles or on behalf of different users). Such situations would not benefit from caching the version. But to summarize, is the recommendation to use an auto-detection mechanism and a toggle to allow users to configure behavior? |
5e189ce
to
18ccb30
Compare
f71a830
to
f4fb6d2
Compare
f4fb6d2
to
8259b41
Compare
80545e4
to
dc54d46
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % one last comment
trino/dbapi.py
Outdated
# not updating the cache | ||
value = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in case of failure with the query we'd not cache the failure and keep retrying on every new connection creation.
Is the failure expected to be transient? IMO we should cache failure as well and revert to legacy statements in such cases - there's no downside to doing so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking the failure would be transient, but your suggestion makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given this, what do you think about changing the query used for checking the capability. Instead of relying on the version check, just see if the new statement works. Basically run EXECUTE IMMEDIATE 'SELECT 1'
and see if it succeeds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense to me.
I also see that SELECT can be constrained via resource groups/access control, I'm not sure if EXECUTE
has such restrictions as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM except small style remark.
Please squash after applying #375 (comment) (if you want to add that) or squash otherwise too. |
75b013a
to
22a853a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
cc: @electrum on similar caching approach we can follow in JDBC driver when switching prepared statements to EXECUTE IMMEDIATE.
@aalbu please check if the release notes at #377 (comment) properly capture the change. |
Description
Make parametrized statement executions more efficient by taking advantage of trinodb/trino#17341.
When running against older versions of Trino that don't support the
EXECUTE IMMEDIATE
statement, the old behavior can be restored by passinglegacy_prepared_statements=True
when creating a connection.Draft PR until the back-end change is released.
Non-technical explanation
Release notes
( ) This is not user-visible or docs only and no release notes are required.
(X) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: