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

db misses an API to precompile the query and bind data later. #9453

Closed
mikra01 opened this issue Oct 20, 2018 · 12 comments · Fixed by #14408
Closed

db misses an API to precompile the query and bind data later. #9453

mikra01 opened this issue Oct 20, 2018 · 12 comments · Fixed by #14408

Comments

@mikra01
Copy link

mikra01 commented Oct 20, 2018

dbFormat is used to substitute the param values.
the question mark (?) is matched on the entire sql-string instead
of binding by the database. This would harden the API and the
database is able to cache the (already parsed) prepared statements.
At the moment the database does a hard-parse on every invocation.

example for sqlite3 to demonstrate the issue:

import db_sqlite

const create_table1_sql = sql"""
create table if not exists ttab(
  id integer primary key,
  question text,
  answer text
);"""

const insert_sql = sql"""
insert into ttab (id,question,answer) values (?,'how do you do ?',?)
 """

let db = open(":memory:","","","") 
db.exec(create_table1_sql)
db.exec(insert_sql,1,"fine!") # index out of bounds is raised here
db.close

#[ runtime outcome is:
  db_sqlite.nim(139)       exec
  db_sqlite.nim(129)       tryExec
  db_sqlite.nim(119)       dbFormat
  system.nim(2939)         sysFatal
  Error: unhandled exception: index out of bounds [IndexError]
]#

Solution:

DbType already present within db_common.nim; for db_postres.nim
there is also a "prepare" proc already present but
unfortunately the parameter dbType is missing.

The API could be extended a little bit (introduce new procs without affecting the
present API) so that the caller is able to promote the dbTypes.
Or introduce a preparedStatement object (better solution I think) and bind the values like this: ps.setString(1,"fine!")
Binding (driver layer) would be not that difficult then.

Additional Information

this affects the entire db-driver layer.

Discussion can be found here:
https://forum.nim-lang.org/t/4320

related discussion here
nim-lang/db_connector#12

also in the past some issues relating this problem
bubbled up:
#3328
#6571

@krux02
Copy link
Contributor

krux02 commented Oct 20, 2018

Please rephrase the title, db_*.nim dbFormat problems is not a good title.

@krux02
Copy link
Contributor

krux02 commented Oct 20, 2018

Can you provide an example of how you imagine the changed API?

@mikra01 mikra01 changed the title db_*.nim dbFormat problems and bind calls missing db_*.nim dbFormat issue and bind calls missing Oct 20, 2018
@mikra01 mikra01 changed the title db_*.nim dbFormat issue and bind calls missing db_*.nim API issue and bind calls missing Oct 20, 2018
@mikra01
Copy link
Author

mikra01 commented Oct 20, 2018

I renamed it but give me a hint if it's not sufficient.
In my (personal) opinion java has the cleanest (enterprise proven) API:
https://docs.oracle.com/javase/tutorial/jdbc/basics/index.html
The resultset is a stream which can be consumed row by row
or randomly accessed (like a filestream)

An example how the preparedStatement stuff is working:
https://docs.oracle.com/javase/tutorial/jdbc/basics/prepared.html

Important thing is: the binding of the values is done at the database side
and not at the client ones. The entire job (binding, validation and so on
is performed by the db). plus: the prepared statement will be cached by the db so it's only
parsed once (works more or less vendor generic).
If you run millions of queries this is usually a major performance impact.

A good explanation how the binding is done can be found here:
https://www.sqlite.org/c3ref/stmt.html

Detailed information how it's working (postgresql but more or less generic)
https://www.postgresql.org/docs/11/static/protocol-flow.html
https://www.postgresql.org/docs/11/static/sql-prepare.html

@krux02
Copy link
Contributor

krux02 commented Oct 20, 2018

I think I get it now, you want an API to compile a query and then execute it with arbitrary data, instead of an API that generates a query string including the data that needs to be evaluated on each call.

And no the issue title is still not good, using the word issue is kind of redundant. But don't worry I will change the title as soon as you can confirm that I got you this time.

@mikra01
Copy link
Author

mikra01 commented Oct 20, 2018

yep, that's correct - you got me now 😃 the api would consist of a generic part and a vendor specific one. Also connection pooling and that stuff. But that is not that easy as it looks. I'd like to contribute here but I could only offer modest timeslots.. I also was not sure if I should file a bug or feature request.

@krux02
Copy link
Contributor

krux02 commented Oct 21, 2018

@mikra01 yes the issue templates are still quite new and not final. Since they are supposed to help your feedback is very welcome. What kind of issue template would you have prefer?

@krux02 krux02 changed the title db_*.nim API issue and bind calls missing db misses an API to precompile the query and bind data later. Oct 21, 2018
@mikra01
Copy link
Author

mikra01 commented Oct 21, 2018

@krux02 thank you the changed topic looks much better now. The thing is that all related issues here from the past are just symptoms (missing parameter binding).
I also checked the new issue templates. they look very good for me. clean and concise. nothing to add further...

@xzfc
Copy link
Contributor

xzfc commented Oct 29, 2018

I have made a fork of db_sqlite that uses sqlite3_bind_* calls instead of quoting everything into strings.
https://github.com/xzfc/ndb.nim

Noteworthy, you can just replace import db_sqlite with import ndb/sqlite in your example to get it work as intended.

Reusing precompiled statements is not supported at the moment, but I am considering adding it.

@mikra01
Copy link
Author

mikra01 commented Nov 1, 2018

@xzfc fine exactly thats it :-) (low-level api). Also dbNulls are handled the right way by your impl. I glanced at it shortly (but not tried yet). you could get rid of the dbQuoting-stuff now. Additional remark: within the iterator (fastRows iterator) you could put a finally in it to make the database happy if the iterator is aborted with break by the caller (I remember it was your thread in the forum a few days ago?) .

Edit: I thought about the caching of the preparedStatements: it's better (I think) to delegate that job to the caller. the user code knows better when (or when not) to cache. just ensure that prepare_v2 is only called once and for a new binding sqlite3_reset() must be called (I could imagine a kind of binding-iterator for bulk loads...). just ensure that finalize() is called but only within the destructor of the preparedStatement (or deInit).

@xzfc
Copy link
Contributor

xzfc commented Nov 2, 2018

@mikra01
Yep, in ndb/sqlite, dbQuote isn't used to generate query string, and iterators have proper finalizers (I have pushed it just now).

Concerning preparedStatements, yes, this makes sense.

@Araq
Copy link
Member

Araq commented Nov 2, 2018

To encourage you a bit, once ndb supports all of our db_* modules we'll refer people to your Nimble package and move ours to the graveyard.

@mikra01
Copy link
Author

mikra01 commented Feb 28, 2019

I recently found now a timeslot to implement what I thought. please take a look at it: https://github.com/mikra01/nimdb . A starter would be the examples directory (tested wth 0.19.9). At the moment I am fiddling with ODBC....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants