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

allow execution of queries without prepared statements #29

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tdawber
Copy link

@tdawber tdawber commented Jan 19, 2017

Execute queries as prepared statements by default, but allow the option of executing them as standard statements.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

5 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling a1f8d3c on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

coveralls commented Jan 19, 2017

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling 935beae on tdawber:non-prepared-statements into c2a160f on baztian:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling 935beae on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling 935beae on tdawber:non-prepared-statements into c2a160f on baztian:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 78.49% when pulling 935beae on tdawber:non-prepared-statements into c2a160f on baztian:master.

@tdawber
Copy link
Author

tdawber commented Jan 19, 2017

@baztian, could you have a look over this and see if it's better suited than the other approach? Ta!

@baztian
Copy link
Owner

baztian commented Jan 19, 2017

Thanks again for you pull request. I was proposing to change the connect method signature not the execute method signature. Or do you know dbs or situations where one statement should be prepared and the other not?

@tdawber
Copy link
Author

tdawber commented Jan 23, 2017

There's an overhead in first preparing, then executing the query, both client-side and database-side. It's not big, but if you're firing off a lot of queries, it can have a cost, so I think being able to choose on a query-by-query basis whether you'll use prepared statements or not might be beneficial?

I ran a few simple tests using jaydebeapi and a local hsql database, and prepared statements were consistently 5% slower than direct queries.

So, personally, I'd prefer to choose the more appropriate option on a query-by-query basis.

@baztian
Copy link
Owner

baztian commented Jan 23, 2017

Thanks for the clarification. I think it depends on the database and on the type of query you run and how often you run it. So your solution to have a choice at statement execution time seems to be good. Anyways it is not db-api standard.
MySql seems to have the parameter prepared=True at the cursor() method. Can you please have a look at other DB drivers and find if the is something like a de factor standard?
Also an interesting read: http://stackoverflow.com/questions/4614131/should-i-always-prefer-working-with-prepared-sql-statements-for-performance-ben

@baztian
Copy link
Owner

baztian commented Jan 23, 2017

There is no support (and no fallback or warning) if parameters have been supplied.

@toby-coleman
Copy link

If you are using a library like Pandas to run SQL queries, it would be nice to be able to change this option on the connect method. Reason being that the execute method is called inside Pandas, so in this scenario you can't change it when running an individual query.

@raelik
Copy link

raelik commented Jun 27, 2017

I had just opened another PR similar to this (it simply used a regular statement when no parameters are given), but I closed it and figured I'd just add to the discussion here. Not all database drivers give the option to disable server-side prepared statements, and we've run into specific issues with Vertica when using some of their internal "procedures" within a prepared statement (Vertica doesn't have stored procedures. What you would normally do as one is instead implemented as a function with a side effect. Gross.)

@raelik
Copy link

raelik commented Jun 27, 2017

I should also note that besides adding a level of protection against SQL injection, prepared statements are often used for performance purposes when the same query is being executed multiple times with different parameters (executemany() does this). But with JDBC, that requires that the original PreparedStatement object be used, which isn't possible in execute() due to how the Cursor closes and recreates the statement every time. You could achieve this by using a dictionary to hold the prepared statements, with the SQL acting as the key. This is really a completely separate topic though, but my point is that there is absolutely no reason, with the way that the Cursor currently implements execute(), to use a prepared statement there at all if there are no parameters being passed. executemany() is a different story, but you wouldn't likely be using that without parameters.

@koljamaier
Copy link

koljamaier commented Jun 13, 2019

I have the exact same setup as @fmcmac with Drill. Is there a working solution with a parameter like use_prepared_statements, that won't wrap my queries with SELECT *...?

@thomaskelm
Copy link

Has there been any update on this Pull request? I'm also using the same Drill setup and you like to be able to run queries besides select statements

@jbelina
Copy link

jbelina commented May 27, 2022

It's been over 2 years since the last request for an update on this PR.

Can this update either be applied, or at least add a comment with the reason why it will not be merged in?

@keweishang
Copy link

@baztian We also run into this issue when PREPARE isn't needed. I think either passing this flag via the connect function or via the execute function would be great. I slightly prefer to use connect function. Please let us know if this PR or a new PR can be approved and merged? Thank you :)

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.

10 participants