Issue: Insert from_select Mysql returns number of rows affected, instead of lastrowid/pk
Author: mkmoisen
State: closed
Created: 2022-03-27 16:38:01

Using an insert_from with mysql will always return the number of rows inserted, instead of returning the lastrowid.  Postgresql instead will return a result cursor from which you can derive the lastrowid. So if we use the insert_from style with MySQL there is no way to obtain the lastrowid using the public API.

In Mysql:

```
# this will correctly return the lastrowid/pk:
Users.insert(name='foo').execute()

# this will always return 1, instead of the lastrowid/pk. 
Users.insert_from(
    Select(columns=['a']),
    ['name']
).execute()
```

With Postgresql, the behavior is different, presumably because postgresql will automatically add a returning (which is not available in mysql).

```
# This will return the PK, same as mysql:
Users.insert(name='foo').execute()

# This returns ModelTupleCursorWrapper
res = Users.insert_from(
    Select(columns=['a']),
    ['name']
).execute()

# get the pk
print(list(res)[0][0])

# let's get rid of the RETURNING clause, and the behavior is the same as mysql, which is to return 1:
res = Users.insert_from(
    Select(columns=['a']),
    ['name']
).returning().execute()
```






Using the pymysql driver, we can access the lastrowid for both regular inserts and insert_from style:

```
conn = pymysql.cononect(user='mysql', password='mysql', database='mysql')
cur = conn.cursor()

cur.execute("insert into users (name) values ('a')")
print(cur.lastrowid)

cur.execute("insert into users (name) select 'b'")
print(cur.lastrowid)
```

The reason for this behavior is because `_WriteQuery._execute` checks if the insert has a RETURNING clause (only available on Postgresql), and then calls `_WriteQuery.handle_result` which returns `database.rows_affected(cursor)`. Because of this implementation, it will always return the number of rows affected for MySQL.

I have a proposal to have mysql return a cursor while making this backwards compatible with the current api. 

If mysql users want to return the lastrowid from a INSERT ... SELECT, they should add a `.returning(Users.id)`. Today, this will fail of course because RETURNING isn't supported. The proposal is that if an insert is on mysql and contains _returning, peewee will strip the RETURNING clause, execute the SQL, but return either the cursor (like postgresql) or the lastrowid, instead of the rows affected.

There are many ways to do it. Here is one:

```
    def execute_returning(self, database):
        if self._cursor_wrapper is None:
            if database.returning_clause:
                cursor = database.execute(self)
                self._cursor_wrapper = self._get_cursor_wrapper(cursor)
            else:
                self._returning = None
                cursor = database.execute(self)
                self._cursor_wrapper = MysqlInsertFromSelectCursorWrapper(cursor)
        return self._cursor_wrapper
```

This MysqlInsertFromSelectCursorWrapper would basically do this:

```

class MysqlInsertFromSelectCursorWrapper(CursorWrapper):
    def iterator():
        yield (self.cursor.lastrowid,)
    
```


This behavior was changed in commit ab4337669 / 3.12.0 - https://github.com/coleifer/peewee/releases/tag/3.12.0 -- the issue #2026 shows one user asking how to get the affected rowcount when doing a bulk insert. So it seems you cannot please everyone. I think this may be particularly relevant for users who are writing UPSERT type bulk queries. At any rate, I do agree that making this be Peewee's default behavior in such cases may have been a little premature.

I think what I'm going to do is this:

* revert the logic in the above commit so that INSERT queries on non-returning DBs will return last-insert-id by default (for "simple" inserts as well as bulk)
* add a new flag that users can append to their INSERTs if they prefer to receive the rowcount instead.

Note that Sqlite also now supports the RETURNING clause, but for backwards-compatibility we do not return the cursor or append the RETURNING clause for Sqlite automatically at the moment (explicitly specifying returning works fine w/sqlite, though).

Hi @coleifer , that sound fine to me. Thank you.

I suppose the only issue is that the API is different when using postgresql compared to when using mysql. However I don't think this is too big of an issue, SQLAlchemy also has a different API for postgres cursors vs mysql cursors.

Thanks for letting me know about sqlite, I was under the impression it did not support RETURNING. I'll be adding that change to my framework.

---

Incidentally, apparently MariaDB [supports RETURNING](https://mariadb.com/kb/en/insertreturning/). MariaDB is "compatible" with MySQL and even uses the same python database driver (such as pymysql or mysql-connector etc). Have you tried Peewee with MariaDB? I don't have MariaDB installed, but would be curious to know if the pymsql/etc drivers are able to handle support for RETURNING.

If the drivers do support RETURNING, peewee will work so long as the user manually adds .returning(Users), right?

Off the top of your head, do you know if I would be able to detect if a user is using MariaDB? I'm writing a framework on top of peewee. I use the following to check for different databases, which won't work for MariaDB:

    is_postgres = isinstance(db, PostgresqlDatabase)
    is_mysql = isinstance(db, MySQLDatabase)


I do notice, however, that the value of `cursor.lastrowid` is not quite correct when doing an INSERT FROM on MariaDB with pymysql -- perhaps that may have something to do with the original change, although I didn't mention it anywhere if that's the case (dammit).

Example script:

```python

#... insert 4 rows into a table named "reg" ...
Reg.insert_many([{'name': 'reg%s' % i} for i in range(1, 5)]).execute()

query = Reg.select(Reg.name)
lastrow = User.insert_from(query, ['username']).execute()
print(lastrow)  # prints 1 !!
for row in User:
    print(row.id, row.username)
# 1 reg1
# 2 reg2
# 3 reg3
# 4 reg4  <-- this is the last row id, presumably?
```

To make sure there were no bugs elsewhere I dropped a print statement into the `InsertQuery.handle_result` to print the value of `cursor.lastrowid`, and confirmed that pymysql is indeed returning `1` in this case.

So possibly this is a mariadb/oracle mysql difference, as it sounded like you were getting the actual last inserted id (4) as opposed to 1?

-----------------------------

Also to respond to your new post, yes, Peewee works seamlessly with the new RETURNING syntax on MariaDB. It has the additional benefit of being useful when using a non-integer primary key.

>Off the top of your head, do you know if I would be able to detect if a user is using MariaDB?

Sure:

```python
conn = db.connection()  # Get db-api2 conn
print(conn.server_version) # or
print(conn.get_server_info())
# 5.5.5-10.7.3-MariaDB-1:10.7.3+maria~bullseye-log
```

5f6a2ed should resolve the issue by restoring the original behavior (returning last-id by default).

Hi @coleifer 

Thanks for the change.

Regarding Sqlite:

> Note that Sqlite also now supports the RETURNING clause, but for backwards-compatibility we do not return the cursor or append the RETURNING clause for Sqlite automatically at the moment (explicitly specifying returning works fine w/sqlite, though).

I wonder, would it be OK to check the version of sqlite and then set db.returning_clause accordingly for the user?

You could add the following to `SqliteDatabase.__init__`:

    self.returning_clause = sqlite3.sqlite_version >= '3.35.0'

[The SQLite release notes](https://www.sqlite.org/changes.html) say that support for RETURNING was added in 3.35.0.

>I wonder, would it be OK to check the version of sqlite and then set db.returning_clause accordingly for the user?

There were just enough "gotcha's" with this that I have chosen not to do it yet automatically - especially as most sqlite users are dynamically linking against their system sqlite which likely to be quite old. Setting `db.returning_clause = True` yourself on SqliteDatabase is quite simple if you do a sqlite version check (as your snippet shows).

There were also some issues with the way the sqlite query compiler was handling the result. Namely, I recall that if you were using Sqlite and specified a RETURNING clause, you *HAD* to step through all the results in order to get the query to execute fully (relevant to bulk inserts/updates/deletes). If you left the result cursor un-stepped-through I recall it could cause quite a bit of strange results.

8c80725cdfc745a082325006c14c50b90de8354d -- this is the commit I reverted. The commit text mentions another issue:

>This caused some unexpected issues when the returning cursor was not
>consumed (e.g. within a savepoint), that cause the savepoint to not 
>able to be released. Since this is backwards-incompatible, I am going to
>roll it back for now.

I think those issues *may* have been resolved in a more recent release than the 3.35 one (and may be also related to this error message mismatch regarding 3.35 and 3.36: https://github.com/coleifer/pysqlite3/issues/34 ).

Relevant sqlite forum link: https://www.sqlite.org/forum/forumpost/e5e99c6497b3ecb9

Hi @coleifer 

Thanks for the links. 

That foreign key constraint error message bug in sqlite3 is unfortunate. 

I'm writing a library that uses peewee. I wanted to automatically add a returning clause if the database supported it.

I think it might be best for me to assume that sqlite users don't have access to RETURNING for the time being.


The issue with the query execution I believe is fixed in 3.36 and is far more concerning than the error code. So if you check for 3.36 or newer it may be safe to use returning everywhere.

I'm going to revisit that patch and see how things are looking when I can find some free time.