Problems with WITHOUT ROWID tables with PK of string type #1559
Conversation
This fixes two related problems: - When the PK is updated the hidden column 0 must be updated too, otherwise any further editing of the same row before a table refresh is broken. - When a new record is inserted and the PK has string type we cannot simply make a pseudo auto-increment and insert that value as rowid because that added key would be impossible to update because our UPDATE clause will try to update a column with a string and it contains an integer. In this case it's better to let the user enter the PK value, so the new Add Record dialog is directly invoked. See issue #1332 and (tangentially) #1049. The first should be fixed now. The later not, but at least there is now a workaround: removing the AUTOINCREMENT option and use the WITHOUT ROWID one.
| lock.unlock(); | ||
| // Special case for rowid columns | ||
| if(m_headers.at(index.column()) == m_sRowidColumn) { | ||
| cached_row.replace(0, newValue); |
There was a problem hiding this comment.
No changes to the data cache should be made after the lock.unlock(); line. The Qt docs aren't really specific about calling unlock() on a QMutexLocker twice but for a plain QMutex they say it's undefined behaviour. So I guess the easiest way to fix this looks like this:
if(m_headers.at(index.column()) == m_sRowidColumn) {
cached_row.replace(0, newValue);
const QModelIndex& rowidIndex = index.sibling(index.row(), 0);
lock.unlock();
emit dataChanged(rowidIndex, rowidIndex);
} else {
lock.unlock();
}
emit dataChanged(index, index);
return true;
src/MainWindow.cpp
Outdated
| if(m_browseTableModel->insertRow(row)) | ||
| bool isWithoutRowidTable = db.getObjectByName(currentlyBrowsedTableName())->type() == sqlb::Object::Table && db.getObjectByName<sqlb::Table>(currentlyBrowsedTableName())->isWithoutRowidTable(); | ||
|
|
||
| if(!isWithoutRowidTable && m_browseTableModel->insertRow(row)) |
There was a problem hiding this comment.
This works for inserting but it only works around a deeper problem we have here. You can see that we still have problem even with this commit by inserting a new row in the Execute SQL tab like this:
INSERT INTO test VALUES(7, 'bla');Then go back to the Browse Table tab and modify the 'bla' value to something else and click the refresh button. It then goes back to 'bla'. Same for deleting the row.
The problem with insertion here is that we auto generate a new PK, then insert the row with that, then read back that row and use store the values we got in our internal buffer. However, we insert the row in a similar way as my query above but for reading it back we query it like this:
SELECT * FROM test WHERE item='7';and this returns no results.
Same for updating the row from above or deleting it. We always use ```item='7'``` which returns no results. It's actually even worse. Try this database for example:
```sql
CREATE TABLE test (item PRIMARY KEY, extra1, extra2) WITHOUT ROWID;
INSERT INTO test VALUES(1, 123, 1);
INSERT INTO test VALUES('1', 123, 2);
Now go back to the Browse Data tab and edit the '123' in the row that ends with the '1'. Refresh and you will see that we actually modified the other row.
So long story short, I'm not sure about what to do here. I'm not strongly leaning against the changes here but I think they hide a potentially useful feature (auto incrementing primary key in without rowid tables, I guess most PKs in without rowid tables will be numeric so this is pretty useful for most users) while still leaving us with worse problems 😉
Any idea how to solve the '1'<>1 issue for without rowid tables?
There was a problem hiding this comment.
I thought in avoiding the inline insertion only for without-rowid-tables-with-string-type-pk, but finally went for the simple solution.
What if we check for the column type before making the auto-increment and if it's string, let it fail by not giving it a value? Then it enters in the Add Record dialog as fallback.
SQLite is generally very permissive with types and performs automatic conversions in numerous cases, but then it bites you in cases like this, when you don't expect it.
Another option that I'm not sure if it would work. We insert the auto-incremented value as a string. In integer columns, it is supposed to be inserted as an integer because it is automatically converted. In a string column it should be inserted as a string and then the WHERE item='7' clause would work.
…e 2) Update after review: - cached_row is not modified after unlock(); - Alternative solution to initial key value insertion: When a new record is inserted and the PK has string type we still make a pseudo auto-increment and insert that value as a string literal. In this way we can later update that row. When we inserted it as integer an actual integer will be inserted by SQLite, and our UPDATE clause, which always uses string in the WHERE condition, won't match the row (SQLite does not convert to integer when the column is of type string in this context). See issue #1332.
|
I've implemented an alternative solution that restores the inline row insertion and only assures that the auto-incremented PK value is inserted as a string if the column is of type string. In this way we don't hide the auto-incrementing primary key feature, but avoid to insert an integer that we wouldn't be able later to reference.
Why do you think that? In that case wouldn't the special case for "INTEGER PRIMARY KEY" fit better? I've taken a look to the documentation:
Note that this alternative solution is only for not shooting ourselves in the feet (not inserting an integer where a string should be). But this does not solve the case of the user inserting an integer to this text primary column. In that case, he puts a bull's-eye in his foot and later we shoot there 😉 Seriously, I think the only way to solve the underlying problem would be to add cell types to the model and take them into account for updates. That would be also necessary for solving #1416, but that isn't really so important as this. |
|
Thanks for the pretty good analysis, @mgrojo 👍 I pretty much completely agree. The reason why I was thinking that most WITHOUT ROWID tables use an integer primary key is that I remembered the first announcements of the WITHOUT ROWID feature to be not so much about string PKs but more about saving disk space for large tables and providing better optimisations in general. This has somewhat changed in the meantime but you can still read sentences like "In an elegant system, all tables would behave as WITHOUT ROWID tables even without the WITHOUT ROWID keyword." or "A WITHOUT ROWID table is an optimization that can reduce storage and processing requirements." in the SQLite docs which implies that every (new) table should be a WITHOUT ROWID table - and because people are used to integer PKs I was thinking they'll continue to use them 😄 Most of the WITHOUT ROWID issues we dealt with in the past had integer PKs too 😉 The updated code looks like the best from both worlds and shouldn't break anything else. So I'm going to merge this and then cherry-pick the changes to the 3.11.0 branch to have this in the upcoming release 😄 |
* Problems with WITHOUT ROWID tables with PK of string type This fixes two related problems: - When the PK is updated the hidden column 0 must be updated too, otherwise any further editing of the same row before a table refresh is broken. - When a new record is inserted and the PK has string type we cannot simply make a pseudo auto-increment and insert that value as rowid because that added key would be impossible to update because our UPDATE clause will try to update a column with a string and it contains an integer. In this case it's better to let the user enter the PK value, so the new Add Record dialog is directly invoked. See issue #1332 and (tangentially) #1049. The first should be fixed now. The later not, but at least there is now a workaround: removing the AUTOINCREMENT option and use the WITHOUT ROWID one. * Problems with WITHOUT ROWID tables with PK of string type (alternative 2) Update after review: - cached_row is not modified after unlock(); - Alternative solution to initial key value insertion: When a new record is inserted and the PK has string type we still make a pseudo auto-increment and insert that value as a string literal. In this way we can later update that row. When we inserted it as integer an actual integer will be inserted by SQLite, and our UPDATE clause, which always uses string in the WHERE condition, won't match the row (SQLite does not convert to integer when the column is of type string in this context). See issue #1332.
|
Nice. One annoying bug less in our next release. |
This fixes two related problems:
otherwise any further editing of the same row before a table refresh is
broken.
simply make a pseudo auto-increment and insert that value as rowid
because that added key would be impossible to update because our
UPDATE clause will try to update a column with a string and it contains
an integer. In this case it's better to let the user enter the PK value,
so the new Add Record dialog is directly invoked.
See issue #1332 and (tangentially) #1049. The first should be fixed now.
The later not, but at least there is now a workaround: removing the
AUTOINCREMENT option and use the WITHOUT ROWID one.