nim-lang / Nim

Nim is a statically typed compiled systems programming language. It combines successful concepts from mature languages like Python, Ada and Modula. Its design focuses on efficiency, expressiveness, and elegance (in that order of priority).
https://nim-lang.org
Other
16.54k stars 1.47k forks source link

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

Closed mikra01 closed 4 years ago

mikra01 commented 6 years ago

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 https://github.com/nim-lang/db_connector/issues/12

also in the past some issues relating this problem bubbled up: https://github.com/nim-lang/Nim/issues/3328 https://github.com/nim-lang/Nim/issues/6571

krux02 commented 6 years ago

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

krux02 commented 6 years ago

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

mikra01 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

yep, that's correct - you got me now :smiley: 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 commented 6 years ago

@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?

mikra01 commented 6 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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....