New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Problems with WITHOUT ROWID tables with PK of string type #1559

Merged
merged 2 commits into from Oct 9, 2018

Conversation

Projects
None yet
2 participants
@mgrojo
Copy link
Contributor

mgrojo commented Oct 6, 2018

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

@mgrojo mgrojo requested a review from MKleusberg Oct 6, 2018

@mgrojo mgrojo referenced this pull request Oct 6, 2018

Open

Problems with WITHOUT ROWID tables #1332

4 of 14 tasks complete
@@ -393,6 +393,12 @@ bool SqliteTableModel::setTypedData(const QModelIndex& index, bool isBlob, const
{
cached_row.replace(index.column(), newValue);
lock.unlock();
// Special case for rowid columns
if(m_headers.at(index.column()) == m_sRowidColumn) {
cached_row.replace(0, newValue);

This comment has been minimized.

@MKleusberg

MKleusberg Oct 8, 2018

Member

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;
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))

This comment has been minimized.

@MKleusberg

MKleusberg Oct 8, 2018

Member

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?

This comment has been minimized.

@mgrojo

mgrojo Oct 8, 2018

Author Contributor

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.

Problems with WITHOUT ROWID tables with PK of string type (alternativ…
…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.
@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 8, 2018

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.

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 😉

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:

  1. When To Use WITHOUT ROWID

The WITHOUT ROWID optimization is likely to be helpful for tables that have non-integer or composite (multi-column) PRIMARY KEYs and that do not store large strings or BLOBs.

WITHOUT ROWID tables will work correctly (that is to say, they provide the correct answer) for tables with a single INTEGER PRIMARY KEY. However, ordinary rowid tables will run faster in that case. Hence, it is good design to avoid creating WITHOUT ROWID tables with single-column PRIMARY KEYs of type INTEGER.

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.

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Oct 9, 2018

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 😄

@MKleusberg MKleusberg merged commit 16ba6db into master Oct 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MKleusberg MKleusberg deleted the string_pk_without_rowid branch Oct 9, 2018

MKleusberg added a commit that referenced this pull request Oct 9, 2018

Problems with WITHOUT ROWID tables with PK of string type (#1559)
* 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.
@mgrojo

This comment has been minimized.

Copy link
Contributor Author

mgrojo commented Oct 9, 2018

Nice. One annoying bug less in our next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment