Use EXECUTE IMMEDIATE for parameterized statement executions

[aalbu]

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 passing legacy_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:

* Fix some things. ({issue}`issuenumber`)

[hashhar]

When running against older versions of Trino that don't support the EXECUTE IMMEDIATE statement, the old behavior can be restored by passing legacy_prepared_statements=True when creating a connection.

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.
The default behaviour should be to auto-cofigured.

cc: @findepi is this reasonable?

[hashhar]

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.

[hovaesco]

Since client knows server version can we do this automatically instead of user having to opt-into it?

How client knows server version? I assume client need to execute SELECT version() to know it.

[aalbu]

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.

We do: https://github.com/trinodb/trino-python-client/blob/master/tests/integration/test_dbapi_integration.py#L107-L120

[hovaesco]

Some checks are failing, please run pre-commit install to run these checks locally before committing.

[findepi]

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.
The default behaviour should be to auto-cofigured.

I agree that -- if this feature is important -- it should auto-configure.
People should typically be expected not to know to turn a thing on.

Alternatively, we enable it by default after some time, once we believe only compliant server versions are still in use.
(Even with auto-configure, we should remove the auto-configuration code after some time).

Since client knows server version can we do this automatically instead of user having to opt-into it?

How client knows server version? I assume client need to execute SELECT version() to know it.

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?

[aalbu]

I agree that -- if this feature is important -- it should auto-configure. People should typically be expected not to know to turn a thing on.

Alternatively, we enable it by default after some time, once we believe only compliant server versions are still in use. (Even with auto-configure, we should remove the auto-configuration code after some time).

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.

How client knows server version? I assume client need to execute SELECT version() to know it.

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?

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?

[aalbu]

@findepi, @hashhar, @mdesmet PTAL.

[hashhar]

LGTM % one last comment

[hashhar]

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.

[aalbu]

Yeah, I was thinking the failure would be transient, but your suggestion makes sense.

[aalbu]

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.

[hashhar]

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.

[mdesmet]

LGTM except small style remark.

[hashhar]

Please squash after applying #375 (comment) (if you want to add that) or squash otherwise too.

[hashhar]

LGTM.

cc: @electrum on similar caching approach we can follow in JDBC driver when switching prepared statements to EXECUTE IMMEDIATE.

[hashhar]

@aalbu please check if the release notes at #377 (comment) properly capture the change.