classification
Title: sqlite3.Cursor.lastrowid isn't populated when executing a SQL REPLACE statement
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ghaering Nosy List: Alex.LordThorsen, berker.peksag, ghaering, jim_minter, python-dev, r.david.murray
Priority: normal Keywords: patch

Created on 2013-01-04 17:24 by jim_minter, last changed 2016-06-14 12:26 by berker.peksag. This issue is now closed.

Files
File name Uploaded Description Edit
Issue16864_py35.patch Alex.LordThorsen, 2014-04-17 20:04 review
sqlite_lastrowid_35.patch Alex.LordThorsen, 2015-05-10 21:03 review
sqlite_lastrowid_35_v2.patch Alex.LordThorsen, 2015-05-11 04:04
sqlite_lastrowid_35_updated.patch Alex.LordThorsen, 2015-05-14 01:06
sqlite_lastrowid_35_updated_2.patch Alex.LordThorsen, 2015-05-14 02:35 review
sqlite_after_review_1.patch Alex.LordThorsen, 2015-05-17 23:33 review
sqlite_review_2.patch Alex.LordThorsen, 2015-05-19 06:58 review
replace_lastrowid_3_6.patch Alex.LordThorsen, 2015-06-11 00:56 review
Messages (15)
msg179049 - (view) Author: Jim Minter (jim_minter) Date: 2013-01-04 17:24
sqlite3 doesn't populate the lastrowid member of the Cursor object when a SQL REPLACE statement is executed.

The following snippet doesn't work as I would expect:
cursor = db.execute("REPLACE INTO table(column) VALUES ('datum')")
print cursor.lastrowid  # prints None

The following snippet, with SQL which is in effect identical to SQLite, does work as expected:
cursor = db.execute("INSERT OR REPLACE INTO table(column) VALUES ('datum')")
print cursor.lastrowid  # prints some rowid

Looking at Modules/_sqlite/cursor.c, in _pysqlite_query_execute(), the following snippet is found:

if (!multiple && statement_type == STATEMENT_INSERT) {
    Py_BEGIN_ALLOW_THREADS
    lastrowid = sqlite3_last_insert_rowid(self->connection->db);
    Py_END_ALLOW_THREADS
    self->lastrowid = PyLong_FromLong((long)lastrowid);
} else {
    Py_INCREF(Py_None);
    self->lastrowid = Py_None;
}

I suggest this should read something like:
if (!multiple && (statement_type == STATEMENT_INSERT || statement_type == STATEMENT_REPLACE)) {
instead of:
if (!multiple && statement_type == STATEMENT_INSERT) {

Thanks,

Jim
msg179058 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2013-01-04 17:55
Considering that sqlite's 'replace' is a synonym for 'insert or replace', I think the logic error is actually in the detect_statement_type function.  Since actions are conditionally taken on the REPLACE statement type in the code, including at least one that adjusts the lastrowid, I don't think the fix for lastrowid is as simple as just always setting it.  But I'm not that familiar with sqlite internals, so perhaps someone with more knowledge will weigh in.
msg216666 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2014-04-17 07:09
Have a unit test that replicates this bug. Working on the C code to fix it right now.
msg216735 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2014-04-17 20:04
Patch that fixes Issue16864. Follows Jim Minters suggestion.

Unit test will reproduce the issue without the c modifications. C modifications fix the issue.
msg241137 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-04-15 18:20
Made some review comments.  There also needs to be a documentation change since the docs currently say it is set for insert only.  There should be a .. versionchanged directive.  This may be small enough not to pass the what's new threshold, but I'd rather add a note and let the person who does the final edit on that document decide, so please add that as well.

(You will note that I've changed this to enhancement...it is documented as *only* working for INSERT, so I believe it must be treated as an enhancement to make it work for REPLACE.)
msg242867 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-05-10 21:03
Updated the patch to have a versionchanged for lastrowid in Doc/Library/sqlite3.rst and Doc/whatsnew/3.5.rst

One thing of note is that I wasn't able to get the indentation to be on the same level for sqlite3.rst (it would either intent the description text or the versionchange text).

Now that I'm actually starting to understand the dbapi and sqlite3 I've come to the conclusion that the lastrowid method should update the lastrowid for the INSERT OR ROLLBACK, INSERT OR ABORT, INSERT OR FAIL, INSERT OR IGNORE statements as well as the INSERT and INSERT OR REPLACE statements. 

I'm unsure how hard or simple supporting those statements will be

The code in question is

 704         Py_DECREF(self->lastrowid);$
 705         if (!multiple && statement_type == STATEMENT_INSERT) {$
 706             sqlite_int64 lastrowid;$
 707             Py_BEGIN_ALLOW_THREADS$
 708             lastrowid = sqlite3_last_insert_rowid(self->connection->db);$
 709             Py_END_ALLOW_THREADS$
 710             self->lastrowid = _pysqlite_long_from_int64(lastrowid);

And the difficulty will be if sqlite3_last_insert_rowid (line 708) does or doesn't return a row id if the OR STATEMENT portion of those inserts are triggered.

The Problem I'm having is that when I tried to find sqlite3_last_insert_rowid in the Modules/_sqlite directory I got nothing

$ grep -r "sqlite3_last_insert_rowid" Modules/_sqlite/
Modules/_sqlite//cursor.c:            lastrowid = sqlite3_last_insert_rowid(self->connection->db);

When I pulled the grep out to the entire cpython repository 

$ grep -r "sqlite3_last_insert_rowid" .
Binary file ./build/lib.macosx-10.10-x86_64-3.5-pydebug/_sqlite3.cpython-35dm-darwin.so matches
Binary file ./build/lib.macosx-10.10-x86_64-3.5-pydebug/_sqlite3.so matches
Binary file ./build/temp.macosx-10.10-x86_64-3.5-pydebug/Users/alexlord/mercurial/cpython/Modules/_sqlite/cursor.o matches
./Modules/_sqlite/cursor.c:            lastrowid = sqlite3_last_insert_rowid(self->connection->db);

I still didn't find anything and I'm not sure where to go from here.
msg242868 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-05-10 21:16
Thanks for Alex_gayner and lifeless. They pointed out the sqlite3_last_row_id is part of the sqlite3 module itself (not cpython).

https://www.sqlite.org/c3ref/last_insert_rowid.html

According the documentation we can expect that if a constraint stops an insertion then the lostrowid is not modified. As such the changes required to add full INSERT OR CONDITION support for last row id is unit tests for each statement and changing the conditional to recognize all insert cases.
msg242882 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-05-11 04:04
Went back to test to see if the other statements are covered already. Unit tests show that lastrowid is set properly no matter what form of sqlite insert statement is used.

sqlite_lastrowid_35_v2.patch contains the doc changes, unit test changes, and code change.

I believe that the latest version of this patch is ready for a code review by a core dev.
msg243326 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-16 15:17
Added review comments.
msg243431 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-05-17 23:33
There was a bunch of things wrong with that patch.

In addition to the issues you brought up in the review I was mixing up what the actual problem is

REPLACE INTO is an alias for INSERT OR REPLACE. INSERT OR REPLACE was correctly setting the lastrowid values but REPLACE INTO was not setting the last rowid value. I changed the documentation modifications to reflect this.

In addition I cleaned up the unit tests. The unit tests were kind of a mess because I was trying to figure out what the problem was and never refactored after getting a reproduction.

 I at first went down the path of making the tests use a for loop like you suggested

    for statement in ["INSERT OR REPLACE", "REPLACE"]:
        sql = "{} INTO test(id, unique_test) VALUES (?, ?)".format(
            statement)
        self.cu.execute(sql, (1, "foo"))
        self.assertEqual(self.cu.lastrowid, 1)
        self.cu.execute(sql, (1, "foo"))$
        self.assertEqual(self.cu.lastrowid, 1) 

Which I don't think is as nice as a cleaned up unrolled version

    self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)
    self.cu.execute("INSERT OR REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)
    self.cu.execute("REPLACE INTO test(id, unique_test) VALUES (?, ?)", (1, "bar",))
    self.assertEqual(self.cu.lastrowid, 1)

I've created a patch that fixes all of the issues brought up in the code review and is just generally much cleaner.
msg243442 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-05-18 01:00
All lines need to be wrapped to <80 columns.

The idea behind the loop is this:

    sql = "{} INTO test(id, unique_test) VALUES (?, ?)"
    self.cu.execute(sql.format('INSERT OR REPLACE, (1, "foo")
    for statement in ["INSERT OR REPLACE", "REPLACE"]:
        with self.subTest(statement=statement):
            self.cu.execute(sql.format(statement), (1, "foo"))
            self.assertEqual(self.cu.lastrowid, 1) 

What this does is run *both* tests, even if one fails, and reports the results separately (labeled with 'statement='INSERT OR REPLACE', ect).
msg243560 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-05-19 06:58
Oh, alright. That makes a lot of sense. Sorry for being dense. I should have read the docs on subtest.

All lines are under 80 characters and I modified the unit test to use subtest.

Thanks for taking the time to walk me through this.
msg245153 - (view) Author: Alex LordThorsen (Alex.LordThorsen) * Date: 2015-06-11 00:56
Adding a patch for 3.6 since 3.5 is in beta.
msg268553 - (view) Author: Roundup Robot (python-dev) Date: 2016-06-14 12:25
New changeset 2126e8cbc12f by Berker Peksag in branch 'default':
Issue #16864: Cursor.lastrowid now supports REPLACE statement
https://hg.python.org/cpython/rev/2126e8cbc12f
msg268554 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-06-14 12:26
Thanks for the patch, Alex.
History
Date User Action Args
2016-06-14 12:26:00berker.peksagsetstatus: open -> closed

nosy: + berker.peksag
messages: + msg268554

resolution: fixed
stage: patch review -> resolved
2016-06-14 12:25:21python-devsetnosy: + python-dev
messages: + msg268553
2015-08-19 09:56:01ghaeringsetassignee: ghaering
2015-06-11 00:56:57Alex.LordThorsensetfiles: + replace_lastrowid_3_6.patch

messages: + msg245153
versions: + Python 3.6, - Python 3.5
2015-05-19 06:58:30Alex.LordThorsensetfiles: + sqlite_review_2.patch

messages: + msg243560
2015-05-18 01:00:34r.david.murraysetmessages: + msg243442
2015-05-17 23:33:21Alex.LordThorsensetfiles: + sqlite_after_review_1.patch

messages: + msg243431
2015-05-16 15:17:52r.david.murraysetmessages: + msg243326
2015-05-14 02:35:20Alex.LordThorsensetfiles: + sqlite_lastrowid_35_updated_2.patch
2015-05-14 01:06:51Alex.LordThorsensetfiles: + sqlite_lastrowid_35_updated.patch
2015-05-11 21:39:36ned.deilysetnosy: + ghaering

stage: needs patch -> patch review
2015-05-11 04:04:48Alex.LordThorsensetfiles: + sqlite_lastrowid_35_v2.patch

messages: + msg242882
2015-05-10 21:16:58Alex.LordThorsensetmessages: + msg242868
2015-05-10 21:03:51Alex.LordThorsensetfiles: + sqlite_lastrowid_35.patch

messages: + msg242867
2015-04-15 18:20:05r.david.murraysettype: behavior -> enhancement
messages: + msg241137
versions: - Python 2.7, Python 3.4
2014-04-17 20:04:55Alex.LordThorsensetfiles: + Issue16864_py35.patch
keywords: + patch
messages: + msg216735
2014-04-17 15:56:56pitrousetstage: needs patch
type: enhancement -> behavior
versions: + Python 3.4, Python 3.5, - Python 3.3
2014-04-17 07:09:00Alex.LordThorsensetnosy: + Alex.LordThorsen
messages: + msg216666
2013-01-04 17:55:48r.david.murraysetnosy: + r.david.murray
messages: + msg179058
2013-01-04 17:24:20jim_mintercreate