cylc-flow icon indicating copy to clipboard operation
cylc-flow copied to clipboard

rundb: use Cursor.execute* not Connection.execute*

Open oliver-sanders opened this issue 3 years ago • 1 comments

Spotted whilst reviewing the DB code, Cylc uses the sqlite3.Connection.execute* methods rather than the documented approach which uses the sqlite3.Cursor.execute* methods.

Full Explanation

The Python sqlite3 docs call the Cursor.execute* methods e.g:

conn = sqlite3.connect('/tmp/example')
c = conn.cursor()
c.execute('''create table stocks

This advice goes back at least as far at Python 2.6, however, Cylc calls the Connection.execute* methods instead e.g:

https://github.com/cylc/cylc-flow/blob/e92f7e8a9a2ebbc5c60e80c79fa24912719a2a80/cylc/flow/rundb.py#L487-L488

(where self.conn is a sqlite3.Connection object).

According to the docs Connection.execute* involves the creation of an intermediate Cursor object:

execute(sql[, parameters])

This is a nonstandard shortcut that creates a cursor object by calling the cursor() method, calls the cursor’s execute() method with the parameters given, and returns the cursor.

-- https://docs.python.org/3.7/library/sqlite3.html#sqlite3.Connection.commit

I'm not too keen on the word "nonstandard" here, but something which might be a larger concern is that due to the way we perform DB writes, we may be creating multiple intermediate cursor objects:

https://github.com/cylc/cylc-flow/blob/e92f7e8a9a2ebbc5c60e80c79fa24912719a2a80/cylc/flow/rundb.py#L423-L425

(where _execute_stmt calls Connection.executemany).

Motive

I don't know if this is an issue, probably not, we've used this approach for years without issue, however, unless there's a good reason not to I would prefer to follow the documented approach for piece of mind.

It's a quick piece of work to change approach, however, this should go into a minor (not maintenance) version if attempted.

Pull requests welcome!

oliver-sanders avatar Jul 22 '22 11:07 oliver-sanders

I think I've convinced myself that the current approach is safe although somewhat non-standard.

oliver-sanders avatar Jul 27 '22 09:07 oliver-sanders