Skip to content
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 #1332

Closed
mikf opened this issue Feb 28, 2018 · 17 comments
Closed

Problems with WITHOUT ROWID tables #1332

mikf opened this issue Feb 28, 2018 · 17 comments
Assignees
Labels

Comments

@mikf
Copy link

@mikf mikf commented Feb 28, 2018

Details for the issue

Interacting with a WITHOUT ROWID table causes multiple issues:

CREATE TABLE test (item PRIMARY KEY, extra) WITHOUT ROWID;
INSERT INTO test VALUES("test1", 123);
INSERT INTO test VALUES("'test2': 'value'", 234);
  1. Trying to delete the second row in DB4S by selecting it and clicking "Delete Record" fails with
executeSQL:  "DELETE FROM `test` WHERE `item` IN (''test2': 'value'');" -> near "test2": syntax error
deleteRecord:  "near \"test2\": syntax error (DELETE FROM `test` WHERE `item` IN (''test2': 'value'');)"

Probably because of the single quote characters.

This row vanishes in the "Browse Data" view, even though it wasn't actually deleted, and re-appears whenever this view gets refreshed.

  1. The PRIMARY KEY column is displayed twice:

sqlitebrowser

Right-clicking on the column headers and selecting and unselecting "Show rowid column" removes the first item column, until the "Browse Data" view gets refreshed. Afterwards the "Filter" entries in the first row are misaligned.

sqlitebrowser2

After view refresh:
sqlitebrowser3

Useful extra information

I'm opening this issue because:

  • DB4S is crashing
  • DB4S has a bug
  • DB4S needs a feature
  • DB4S has another problem

I'm using DB4S on:

  • Windows: ( version: ___ )
  • Linux: ( distro: ___ )
  • Mac OS: ( version: ___ )
  • Other: ___

I'm using DB4S version:

  • 3.10.1
  • 3.10.0
  • 3.9.1
  • Other: ___

I have also:

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 28, 2018

Thanks for the bug report @mikf. Which Linux distro are you using?

Asking because we've fixed some bugs with the rowid bits since the last release. If you're using Ubuntu it's probably worth trying the nightly builds (info on the front page). If you're using a different distro though, you'd probably need to compile things yourself (not hard). 😄

@mikf
Copy link
Author

@mikf mikf commented Feb 28, 2018

So, I cloned this repo and compiled the latest version.
The display errors mentioned in the second point are all gone, but everything in the first one is still happening: Same error message and the row still disappears temporarily.

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 28, 2018

Arggh. Bug is still present then. Damn. 😉

@mikf
Copy link
Author

@mikf mikf commented Feb 28, 2018

DB4S tries to execute DELETE FROM `test` WHERE `item` IN (''test2': 'value'');, which also fails in the command-line tool.

sqlite> DELETE FROM `test` WHERE `item` IN (''test2': 'value'');
Error: near "test2": syntax error
sqlite>

But this query works if you double each single ' inside the field-value:

sqlite> DELETE FROM `test` WHERE `item` IN ('''test2'': ''value''');
sqlite>

The documentation also mentions to encode single quotation marks by putting two of them in a row:

A string constant is formed by enclosing the string in single quotes ('). A single quote within the string can be encoded by putting two single quotes in a row - as in Pascal. C-style escapes using the backslash character are not supported because they are not standard SQL.

So maybe replacing ' with '' is all there is to it?

@justinclift
Copy link
Member

@justinclift justinclift commented Feb 28, 2018

Yep, hopefully it's that simple. 😄

mgrojo added a commit that referenced this issue Feb 28, 2018
Quoted values in DELETE FROM and UPDATE SET take into account that value
could have a single quote and they are now doubled.

Detect tables without rowid in order to make invisible the "Show rowid
column"

See issue #1332
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Feb 28, 2018

@mikf I think both problems are now solved. 1. is fixed by properly doubling the single quote when needed and 2. by detecting WITHOUT ROWID tables and not showing the "Show rowid column" for them, since it doesn't make sense and breaks several functions.

However there is a background issue, now hidden in this particular case, but still possible:

This row vanishes in the "Browse Data" view, even though it wasn't actually deleted, and re-appears whenever this view gets refreshed.

That needs more thought.

Might you gather some time for testing with the next nightly build? The fix will be available tomorrow in https://nightlies.sqlitebrowser.org/latest/

@mgrojo mgrojo self-assigned this Feb 28, 2018
@justinclift
Copy link
Member

@justinclift justinclift commented Feb 28, 2018

@mikf is using Linux, so doesn't need to wait for a nightly build. 😉

@mikf
Copy link
Author

@mikf mikf commented Mar 1, 2018

I tested the changes in your last commit.
Deleting and updating rows with a ' in their "rowid" works and "Show rowid column" is no longer displayed, but I found some other, possibly related, issues:

  • The "rowid" column still gets shown whenever you either apply a filter or click the column header.
  • Updating the PRIMARY KEY column of a row doesn't properly update its associated rowid (at least that's my guess at to what is happening), which prevents any further updates in said row.
    The UI allows and shows changes made to such a row, but refreshing the view reverts everything back to the state just after the first update to the PRIMARY KEY column.
  • Records created by the "New Record" button can't be updated or deleted at all, even after refreshing.
    As before, changing values or deleting such a row is shown in the UI, but nothing actually happens in the underlying database.
mgrojo added a commit that referenced this issue 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.
@mgrojo
Copy link
Member

@mgrojo mgrojo commented Oct 6, 2018

@mikf I finally had some time to look at this and I think I've found a solution. I've committed it to a branch (string_pk_without_rowid) and made a pull request (#1559), because we want to make a release soon and I don't want to commit to master any code that might break other parts. But given that you can compile from the source, could you fetch the branch and give it a try for confirming whether it's fixed for you?

@mikf
Copy link
Author

@mikf mikf commented Oct 6, 2018

Had some trouble building your branch (same problems as #1439), but I eventually got it working with qmake + make (instead of cmake + make).

Your solution appear to have fixed all the issues mentioned above. I've been playing around with the test database from above and creating, modifying and deleting records in any order works as it should.

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Oct 6, 2018

Thanks for the information. I hope the fix can get to the release. It's good that you've confirmed that it's working for you.

@justinclift
Copy link
Member

@justinclift justinclift commented Oct 7, 2018

@mgrojo I haven't yet created the branch in git for 3.11 (am still getting a new Win dev environment set up, as I need to update our SQLcipher pieces anyway), so there's still time to add things in if you reckon they're important. 😄

@mgrojo
Copy link
Member

@mgrojo mgrojo commented Oct 7, 2018

@justinclift I'd like the PR reviewed by @MKleusberg. Although it's simple, I don't want to introduce some unexpected behaviour so close to the release version.

@justinclift
Copy link
Member

@justinclift justinclift commented Oct 7, 2018

No worries at all. 😄

mgrojo added a commit that referenced this issue Oct 8, 2018
…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.
MKleusberg added a commit that referenced this issue Oct 9, 2018
* 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.
MKleusberg added a commit that referenced this issue Oct 9, 2018
* 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.
@MKleusberg
Copy link
Member

@MKleusberg MKleusberg commented Feb 14, 2020

I think this is solved. Is the 3.11.2 release working for you, @mikf? 😄

@mikf
Copy link
Author

@mikf mikf commented Feb 15, 2020

3.11.2 is working without any issues. Thank you all for fixing this.

@mikf mikf closed this Feb 15, 2020
@justinclift
Copy link
Member

@justinclift justinclift commented Feb 17, 2020

Awesome, thanks for confirming @mikf. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants