Skip to content

Cannot move row immediately after creating it #878

Closed
Closed
Cannot move row immediately after creating it#878
Assignees
prutz1311
Labels
bugConfirmed bugs or reports that are very likely to be bugs.
Milestone
3.10.0 - A Rubber Cactus Has Many Uses

Description

@revolter
revolter
opened on Nov 23, 2016

Details for the issue

Following these steps:

  • select table
  • click "Modify Table"
  • click "Add field"
  • enter row name
  • click "Move field up"

I am presented with a warning popup alert with the message:

renameColumn: cannot find column firebase_token.

This didn't happen in 3.9.0 (tested) though I don't have time now to test in the yesterday's nightly.

Wow, it looks like it's completely broken. After clicking "OK", if I go to "Modify table" again, the last row is named "Field18", being the default name. And if I only change (not move it) and hit "OK", it does nothing.

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: 10.11.6 (15G1108) )
  • Other: ___

I'm using DB4S version:

  • 3.9.1
  • 3.9.0
  • Other: Nightly 15.11.2016

I have also:

Activity

  1. revolter
    added
    bugConfirmed bugs or reports that are very likely to be bugs.
    on Nov 23, 2016
  2. justinclift
    added this to the 3.10.0 - A Rubber Cactus Has Many Uses milestone on Nov 23, 2016

  3. chrisjlocke commented on Nov 28, 2016

    @chrisjlocke
    chrisjlocke
    on Nov 28, 2016
    Member

    Tested this in the latest nightly (on Windows) and after creating a new table and then clicking 'Write Changes' threw up an error.
    image

    Creating a table, then moving a row threw up a similar error.
    image

    Seems similar to #390 and #836.

  4. justinclift commented on Nov 28, 2016

    @justinclift
    justinclift
    on Nov 28, 2016
    Member

    Interesting. I saw similar a week or so ago. Something seems to have gone wrong somewhere which needs tracking down. 👼

  5. prutz1311 commented on Nov 28, 2016

    @prutz1311
    prutz1311
    on Nov 28, 2016
    Contributor

    My suggestions: when a field is just created, it gets a name like FieldN and then after you rename it upon creation, bool DBBrowserDB::renameColumn method gets called. However, this method obtains the version of a table which still has the old name of the field because table schemas are loaded directly from database, therefore the membership check fails. If you move field up, the same bool DBBrowserDB::renameColumn method gets called.

    Currently I have no idea how to make this right. Here is my traceback when moving a field (truncated for short):

    #0  DBBrowserDB::renameColumn (this=0xba4558, tablename=..., name=..., to=..., move=-1)
    at /home/oleg/Документы/программирование/Sources/sqlitebrowser/src/sqlitedb.cpp:1111
#1  0x000000000056e0bd in EditTableDialog::moveCurrentField (this=0x7fffffffc150, down=false)
    at /home/oleg/Документы/программирование/Sources/sqlitebrowser/src/EditTableDialog.cpp:613
#2  0x000000000056dbf1 in EditTableDialog::moveUp (this=0x7fffffffc150)
    at /home/oleg/Документы/программирование/Sources/sqlitebrowser/src/EditTableDialog.cpp:564
#3  0x000000000064a4d4 in EditTableDialog::qt_static_metacall (_o=0x7fffffffc150, _c=QMetaObject::InvokeMetaMethod, _id=9, _a=0x7fffffffb370)
    at /home/oleg/Документы/программирование/Sources/sqlitebrowser/build/moc_EditTableDialog.cpp:120
#4  0x00007ffff6cfc9d5 in QMetaObject::activate(QObject*, int, int, void**) () from /opt/qt57/lib/libQt5Core.so.5
#5  0x00007ffff7473f52 in QAbstractButton::clicked(bool) () from /opt/qt57/lib/libQt5Widgets.so.5

  6. revolter commented on Nov 29, 2016

    @revolter
    revolter
    on Nov 29, 2016
    MemberAuthor

    Maybe a git bisect between HEAD and 3.9.0 would help figure it out.

  7. prutz1311 commented on Dec 2, 2016

    @prutz1311
    prutz1311
    on Dec 2, 2016
    Contributor

    @revolter @MKleusberg
    I found that commit: e76e5b3. If i just remove this check, moving fields up/down works as expected. This check returns false, the function returns false to another functions, which in turn return false again with similar checks, and in majority of them there is no emit dbChanged(getDirty()) signal, which should trigger update of objMap data structure. I still have not figured out which checks should be corrected.

  8. prutz1311
    closed this as completedon Dec 2, 2016
  9. prutz1311
    reopened this on Dec 2, 2016

  10. justinclift commented on Dec 2, 2016

    @justinclift
    justinclift
    on Dec 2, 2016
    Member

    @prutz1311 Cool, it sounds like you're investigating this properly. 😄

    Do you want to assign it to yourself?

  11. prutz1311
    self-assigned this
    on Dec 2, 2016

  12. prutz1311 commented on Dec 5, 2016

    @prutz1311
    prutz1311
    on Dec 5, 2016
    Contributor

    It seems that we are not handling savepoints properly. For example, not every order of setting/releasing savepoints is possible:

    sqlite> SAVEPOINT RESTOREPOINT;
sqlite> SAVEPOINT sqlitebrowser_rename_column;
sqlite> RELEASE RESTOREPOINT;
sqlite> SAVEPOINT RESTOREPOINT;
sqlite> RELEASE RESTOREPOINT;
sqlite> RELEASE sqlitebrowser_rename_column;
Error: no such savepoint: sqlitebrowser_rename_column
sqlite>

    That is, if you release a savepoint X, the savepoints between setting and releasing X get released, and you cannot access them anymore.

    I added a log statement in bool DBBrowserDB::executeSQL method and here is what I got when I opened a database, created a new column, tried to move it and closed the database:

    [debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "PRAGMA foreign_keys = \"1\";"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT `edittable_wordcount_save_1480896969992`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "ALTER TABLE `wordcount` ADD COLUMN \t`Field3`\tINTEGER"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT RESTOREPOINT;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:992) - setting savepoint 'sqlitebrowser_rename_column'
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT sqlitebrowser_rename_column;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "CREATE TABLE `sqlitebrowser_rename_column_new_table` (\n\t`word`\tTEXT,\n\t`cnt`\tINTEGER,\n\t`new_field`\tINTEGER\n);"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "INSERT INTO sqlitebrowser_rename_column_new_table SELECT `word`,`cnt`,`Field3` FROM `wordcount`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE RESTOREPOINT;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "PRAGMA defer_foreign_keys = \"1\";"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "DROP TABLE `wordcount`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "ALTER TABLE `sqlitebrowser_rename_column_new_table` RENAME TO `wordcount`"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE RESTOREPOINT;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "RELEASE RESTOREPOINT;" -> no such savepoint: RESTOREPOINT
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "PRAGMA defer_foreign_keys = \"0\";"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT RESTOREPOINT;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:1104) - releasing savepoint 'sqlitebrowser_rename_column'
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE sqlitebrowser_rename_column;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "RELEASE sqlitebrowser_rename_column;" -> no such savepoint: sqlitebrowser_rename_column
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:1108) - "переименованиеСтолбца: ошибока освобождения точки сохранения. БД говорит: no such savepoint: sqlitebrowser_rename_column (RELEASE sqlitebrowser_rename_column;)"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:992) - setting savepoint 'sqlitebrowser_rename_column'
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT sqlitebrowser_rename_column;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "CREATE TABLE `sqlitebrowser_rename_column_new_table` (\n\t`word`\tTEXT,\n\t`new_field`\tINTEGER,\n\t`cnt`\tINTEGER\n);"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "INSERT INTO sqlitebrowser_rename_column_new_table SELECT `word`,`new_field`,`cnt` FROM `wordcount`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE RESTOREPOINT;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "PRAGMA defer_foreign_keys = \"1\";"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "DROP TABLE `wordcount`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "ALTER TABLE `sqlitebrowser_rename_column_new_table` RENAME TO `wordcount`"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE RESTOREPOINT;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "RELEASE RESTOREPOINT;" -> no such savepoint: RESTOREPOINT
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "PRAGMA defer_foreign_keys = \"0\";"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "SAVEPOINT RESTOREPOINT;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:1104) - releasing savepoint 'sqlitebrowser_rename_column'
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE sqlitebrowser_rename_column;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "RELEASE sqlitebrowser_rename_column;" -> no such savepoint: sqlitebrowser_rename_column
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:1108) - "переименованиеСтолбца: ошибока освобождения точки сохранения. БД говорит: no such savepoint: sqlitebrowser_rename_column (RELEASE sqlitebrowser_rename_column;)"
[warning] DB Browser for SQLite (unknown:0) - QXcbConnection: XCB error: 3 (BadWindow), sequence: 1280, resource id: 15531982, major code: 40 (TranslateCoords), minor code: 0
[warning] DB Browser for SQLite (unknown:0) - QXcbConnection: XCB error: 3 (BadWindow), sequence: 1339, resource id: 15530383, major code: 40 (TranslateCoords), minor code: 0
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "ROLLBACK TO SAVEPOINT `edittable_wordcount_save_1480896969992`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE `edittable_wordcount_save_1480896969992`;"
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "ROLLBACK TO SAVEPOINT RESTOREPOINT;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "ROLLBACK TO SAVEPOINT RESTOREPOINT;" -> no such savepoint: RESTOREPOINT
[debug] DB Browser for SQLite (../../src/sqlitedb.cpp:613) - "RELEASE RESTOREPOINT;"
[warning] DB Browser for SQLite (../../src/sqlitedb.cpp:629) - executeSQL:  "RELEASE RESTOREPOINT;" -> no such savepoint: RESTOREPOINT

    So the handling of savepoint needs to be improved. Some questions arise:

    1. Should we return false if there is no savepoint to release? (Doing this causes many thigs to break down)
    2. Should we release all savepoints in between or do something else? (Seems like we should)
  13. prutz1311
    modified the milestones: 3.9.1 - A bit of polish shouldn't hurt :D, 3.10.0 - A Rubber Cactus Has Many Uses on Dec 5, 2016

  14. justinclift commented on Dec 5, 2016

    @justinclift
    justinclift
    on Dec 5, 2016
    Member

    Hmmm, at first thought yeah... 2. seems like the better idea.

    Want to try it out and see if turns out to be ok?

  15. prutz1311
    added 2 commits that reference this issue on Dec 5, 2016

    Improve handling of SQLite savepoints (sqlitebrowser#878) (sqlitebrow…

    86dd6af

    Improve handling of SQLite savepoints (sqlitebrowser#878) (sqlitebrow…

    2f45341
  16. justinclift
    added a commit that references this issue on Dec 10, 2016

    Merge pull request #895 from prutz1311/master

    e6a8f88
  17. vtronko
    added 2 commits that reference this issue on Dec 29, 2016

    Improve handling of SQLite savepoints (#878) (#836)

    78346ba

    Merge pull request #895 from prutz1311/master

    8eaf73d

  18. MKleusberg commented on May 8, 2017

    @MKleusberg
    MKleusberg
    on May 8, 2017
    Member

    @justinclift and @revolter Have you tried Oleg's changes? They seem to work for me 😃

  19. justinclift commented on May 17, 2017

    @justinclift
    justinclift
    on May 17, 2017
    Member

    Yep, just tested it here on OSX too. Working well.

    Thanks heaps @prutz1311. 😀

  20. justinclift
    closed this as completedon May 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

bugConfirmed bugs or reports that are very likely to be bugs.

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions