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

Unexpected data loss modifying table #1481

Open
tlhackque opened this Issue Jul 25, 2018 · 16 comments

Comments

Projects
None yet
4 participants
@tlhackque
Copy link

tlhackque commented Jul 25, 2018

Details for the issue

What did you do?

I added Not Null & Unique to the primary keys of some tables with the "Database structure" "Modify table" GUI of DB4S.

What did you expect to see?

Just the attributes changed

What did you see instead?

Entire dependent table emptied. (!)

Here is what I think happened:

The dependent table has a number of fields that use FOREIGN KEY clauses to reference the tables where I added "Not Null" and "Unique". The "references" clauses also include on update and on delete cascade.

The (clever DB4S) implementation of checking "NN" and "U" is that the tables are copied to a temporary table; the original is DROPped and the temporary table is renamed to replace the original. (This does take a while with a several million row table...)

The DROP caused an implicit DELETE of all rows, triggering the dependent table's "on delete" clause per, emptying it.

If foreign key constraints are enabled when it is prepared, the DROP TABLE command performs an implicit DELETE to remove all rows from the table before dropping it. The implicit DELETE does not cause any SQL triggers to fire, but may invoke foreign key actions or constraint violations.

This all makes sense at the statement level - but is unexpected in the GUI.

You appear to be trying to handle this by bracketing the sequence with
PRAGMA defer_foreign_keys = "1";

PRAGMA defer_foreign_keys = ;

However, this doesn't prevent the implicit DELETEs. I suspect that the (untested) easy fix is to use
PRAGMA foreign_keys = 0;
instead. This is suggested in the referenced page on sqlite.org:

The properties of the DROP TABLE and ALTER TABLE commands described above only apply if foreign keys are enabled. If the user considers them undesirable, then the workaround is to use PRAGMA foreign_keys to disable foreign key constraints before executing the DROP or ALTER TABLE command. Of course, while foreign key constraints are disabled, there is nothing to stop the user from violating foreign key constraints and thus creating an internally inconsistent database.

Since you are replicating the existing table's contents, the result should not cause any (new) foreign key constraint violations.

Here is an abbreviated copy of the SQL log in one such sequence:

PRAGMA foreign_keys = "1";
PRAGMA database_list;
SELECT type,name,sql,tbl_name FROM "main".sqlite_master;
PRAGMA encoding
PRAGMA foreign_keys
CREATE TABLE "sqlb_temp_table_1" (
	"id"	integer UNIQUE,
[snip]
	FOREIGN KEY("cdid") REFERENCES "CDs"("diskid"),
	PRIMARY KEY("id")
);
INSERT INTO "main"."sqlb_temp_table_1" SELECT "id",[snip] FROM "main"."FILEs";
PRAGMA defer_foreign_keys
PRAGMA defer_foreign_keys = "1";
DROP TABLE "main"."FILEs";
ALTER TABLE "main"."sqlb_temp_table_1" RENAME TO "FILEs"
PRAGMA defer_foreign_keys = "0";
CREATE UNIQUE INDEX "pathindex" ON "FILEs" (
[snip]

Useful extra information

The info below often helps, please fill it out if you're able to. :)

What operating system are you using?

  • [x ] Windows: ( _version:_10 pro ___ )
  • Linux: ( distro: ___ )
  • Mac OS: ( version: ___ )
  • Other: ___

What is your DB4S version?

  • 3.10.1
  • 3.10.0
  • 3.9.1
  • Other: _7-Jul nightly

Did you also

@tlhackque

This comment has been minimized.

Copy link
Author

tlhackque commented Jul 25, 2018

Also, as noted the copy/drop/rename sequence can be quite slow on a large table. And if you hit the wrong box, or change your mind, you wait for each click...

I don't see why the action is triggered on each click of a checkbox. It would be better to aggregate all changes and apply them when OK is clicked.

If the concern is that the GUI reflect the DB state, you could display modified items in a different color, footnoted as "pending".

This is less severe than the data loss, but still annoying. (The copy/drop/rename time is measured in minutes for a modest 2M row table...).

@justinclift justinclift added the bug label Jul 25, 2018

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Jul 25, 2018

Ouch. That's definitely a bug we'll need to fix before the next release.

Thanks @tlhackque. 😄

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Jul 25, 2018

This is the relevant code:

sqlitebrowser/src/sqlitedb.cpp

Lines 1385 to 1394 in 01bf059

// We need to disable foreign keys here. The reason is that in the next step the entire table will be dropped and there might be foreign keys
// in other tables that reference this table. These foreign keys would then cause the drop command in the next step to fail. However, we can't
// simply disable foreign keys here since that is not allowed from inside a transaction and we definitely are inside a transaction at that point.
// So what we do instead is defer foreign key enforcement until the end of the transaction which effectively disables foreign keys for us here.
// But because we don't really want to defer foreign keys, the former value of that pragma is saved here in order to restore the old value later.
QString foreignKeysOldSettings = getPragma("defer_foreign_keys");
setPragma("defer_foreign_keys", "1");
// Delete the old table
if(!executeSQL(QString("DROP TABLE %1;").arg(tablename.toString()), true, true))

It seems as if this approach has worked at some point. Maybe with a different SQLite version.

@tlhackque

This comment has been minimized.

Copy link
Author

tlhackque commented Jul 26, 2018

I can't speak to the SQL history. I can speak to my experience.

I believe that the code you referenced is successful in solving a different, but related problem.

By enabling "defer_foreign_keys", it prevents the DROP from failing, since consistency rules are applied at COMMIT time - and by then, everything is back in place.

This doesn't address my case - the DROP creates an implied DELETE, which runs the ON DELETE CASCADE clauses - deleting rows in the dependent table. In my case, all of them.

With respect to these issues, defer_foreign_keys is a subset of the required effect. It will prevent the DROP from failing. It won't prevent the DROP from cascading DELETEs to the tables referring to the table with foreign keys.

Setting foreign_keys to zero addresses both scenarios.

However, the comments indicate that you can't disable foreign keys because you're in a transaction. Which means that either you have to commit the transaction; roll it back, change the table, open a new transaction, and replay the transaction; or defer these actions until the transaction is committed.

Note that the performance issue of doing the copy/drop/rename sequence for every checkbox click militates toward deferral for other reasons...

@tlhackque

This comment has been minimized.

Copy link
Author

tlhackque commented Aug 5, 2018

The fact that a single click can cause (silent) data loss is worrisome. Also, that the natural reaction to clicking a checkbox by mistake is to click it again - but while that restores the checkbox, it doesn't restore the deleted records.

It occurred to me that until you come up with an implement a transparent solution, you might want to disable anything that does an implicit DROP if foreign keys are enabled and any table has an ON clause associated with a foreign key. Note that ON DELETE SET (NULL, DEFAULT) can cause as much damage as CASCADE. While less obvious than emptying an entire table, losing references corrupts a database.

If you were to implement this, the user can still do anything by manually disabling foreign keys and applying any pending changes first.

While passing responsibility to the user is ugly, this seems preferable to allowing silent data loss/corruption...

FWIW.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Aug 5, 2018

The fact that a single click can cause (silent) data loss is worrisome.

Definitely agreed. Personally, I consider this a "blocker" bug for the next release. We're just a bit short on developer time atm, thus the lack of attention here. 😦

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Aug 5, 2018

We didn't have a "release blocker" label already, so just created one and added this to it.

@mgrojo mgrojo self-assigned this Aug 7, 2018

@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Aug 7, 2018

The only solution I see is to open a new transaction just for the table modification. It's the only way to disable the foreign_key pragma. This will force saving the changes before the table modification and after. I'm trying to implement this following the recommendations in:
https://www.sqlite.org/lang_altertable.html

mgrojo added a commit that referenced this issue Aug 7, 2018

Prevent data loss when editing table with foreign keys enabled
Our procedure for editing tables may involve a drop of the old table
definition. This will trigger the "ON DELETE CASCADE" clauses of the
child tables, leading to unexpected and silent data loss.

Following the procedure described in:
https://www.sqlite.org/lang_altertable.html
we can guarantee that no data is deleted, but at the cost of opening a
new transaction specific for the table modification. The possible pending
changes must be saved before. This is only done when the foreign_keys
pragma is enabled.

See issue #1481
@mgrojo

This comment has been minimized.

Copy link
Contributor

mgrojo commented Aug 7, 2018

I've opened a PR for this, since it has a drawback (needing to save the changes before and after the table modification, for the pragma foreign_keys changes to take effect). I'm also unsure about deleting or not the use of the pragma defer_foreign_keys. It is still there in the source code.

@tlhackque

This comment has been minimized.

Copy link
Author

tlhackque commented Aug 7, 2018

I don't think you need to prompt the user unless there are pending changes to commit. I didn't see a test for that in your commit.

If foreign_keys is OFF, defer_foreign_keys is irrelevant, so unless it's reachable in another path through the code I think that it can be removed.

lang_altertable seems to be the right reference. The remark "can make other arbitrary changes to the format of a table using a simple sequence of operations" seems a bit, er, oversimplified :-)

FWIW - a few random thoughts. I haven't read the DB4S code, so perhaps you handle some or all of them already.

There seem to be a number of potential failure cases that might cause problems:

  • I'm not sure that DROP TABLE can always be rolled back. The doc says:

The dropped table is completely removed from the database schema and the disk file. The table can not be recovered.

At one point, I'm pretty sure that DROP TABLE was considered a DDL change and could not be rolled back.

On the other hand, this simple test seems to say that it (sometimes) can:

sqlite3 test.db
SQLite version 3.19.1 2017-05-24 13:08:33
Enter ".help" for usage hints.
sqlite> create table a (a1,a2);
sqlite> create table b (b1,b2);
sqlite> .tables
a  b
sqlite> begin transaction;
sqlite> drop table b;
sqlite> .tables
a
sqlite> rollback;
sqlite> .tables
a  b

It may be worth verifying with the SQLite folks that DROP is (now) something that can always be rolled-back if something goes wrong. If so, they should update their documentation. And If I'm correct that this has not always been the case, what version number you need to check for before relying on it.

  • The DROP can certainly fail if there's an active cursor on the table or if the disk is full.
  • The "simpler" second procedure may offer an alternative - it doesn't require copying table data/drop or rename to modify UNIQUE/NOT NULL, etc. It would certainly speed up the case that I ran into (~2M records to copy). You still need the current procedure for some cases - e.g. to delete a column.
  • The are a lot of details hidden in statements like"Perhaps use the old format of the triggers and indexes saved from step 3 above as a guide, making changes as appropriate for the alteration" and "If any views refer to table X in a way that is affected by the schema change" ... "recreate them with whatever changes are necessary to accommodate the schema change". Determining what changes are "necessary" in the general case seems like a hard problem.
  • In any situation where a VIEW is involved - it may include any arbitrary SELECT statement - which can include the WITH CTE that DB4S had (has?) trouble parsing.
  • Rollback may be required if the foreign_key_check in step 10 fails. It is interesting (and helpful) that the check is performed even if foreign_keys is OFF:
sqlite3 test.db
SQLite version 3.19.1 2017-05-24 13:08:33
Enter ".help" for usage hints.
sqlite>  CREATE TABLE a (a1,a2);
sqlite> pragma foreign_keys=on;
sqlite>  CREATE TABLE b (b1, b2 references a (a1));
sqlite> insert into b (b1,b2) values ('x','y');
Error: foreign key mismatch - "b" referencing "a"
sqlite> pragma foreign_keys=off;
sqlite> insert into b (b1,b2) values ('x','y');
sqlite> pragma foreign_key_check;
Error: foreign key mismatch - "b" referencing "a"
sqlite>
  • Any simultaneous access to the database might cause any of the interim steps to fail the transaction. You might minimize this by setting a long timeout and using an EXCLUSIVE transaction.

Considering the complexity, this would seem to be a good time to add "backup database" to the "Tools" menu. (I expose the built-in backup in my applications - it's quite fast.) You may want to offer it before executing the complex metadata changes.

@tlhackque

This comment has been minimized.

Copy link
Author

tlhackque commented Aug 7, 2018

Oops - I missed that you do check for "dirty"; cancel my first comment above. Sorry,.

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Aug 7, 2018

It may be worth verifying with the SQLite folks that DROP is (now) something that can always be rolled-back if something goes wrong. If so, they should update their documentation.

@chrisjlocke Any interest in handling that? 😄

MKleusberg added a commit that referenced this issue Aug 9, 2018

Prevent data loss when editing table with foreign keys enabled
Our procedure for editing tables may involve a drop of the old table
definition. This will trigger the "ON DELETE CASCADE" clauses of the
child tables, leading to unexpected and silent data loss.

Following the procedure described in:
https://www.sqlite.org/lang_altertable.html
we can guarantee that no data is deleted, but at the cost of opening a
new transaction specific for the table modification. The possible pending
changes must be saved before. This is only done when the foreign_keys
pragma is enabled.

See issue #1481
@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Aug 9, 2018

@tlhackque We have just merged @mgrojo's PR. So when you test the next nightly build, you can give this one a try too 😄

We'll still need to figure out the rollback of a DROP though. With the current approach we just assume that works...

Regarding the other points:

The DROP can certainly fail if there's an active cursor on the table or if the disk is full.

We check for errors when dropping the table. If there is an error you'll get a message box and we roll back.

The "simpler" second procedure may offer an alternative - it doesn't require copying table data/drop or rename to modify UNIQUE/NOT NULL, etc. It would certainly speed up the case that I ran into (~2M records to copy). You still need the current procedure for some cases - e.g. to delete a column.

I noticed that too. We'll need to rewrite parts of the alter table code anyway to allow multiple changes at once without copying the entire table every time. It makes sense to keep that second procedure in mind when doing this.

The are a lot of details hidden in statements like"Perhaps use the old format of the triggers and indexes saved from step 3 above as a guide, making changes as appropriate for the alteration" and "If any views refer to table X in a way that is affected by the schema change" ... "recreate them with whatever changes are necessary to accommodate the schema change". Determining what changes are "necessary" in the general case seems like a hard problem.

It is. Especially for triggers it's very hard because they can contain all sorts of SQL statements with references to a table and its structure. We currently just add the triggers as they were before and hope for the best. If there is an error we show that to the user along with the problematic SQL statement and tell them to fix that statement.
For indices its not that bad and we handle renamed tables and columns automatically. If there is an error anyway, we show the same message as for triggers.

In any situation where a VIEW is involved - it may include any arbitrary SELECT statement - which can include the WITH CTE that DB4S had (has?) trouble parsing.

Views are as hard as triggers. But as far as I remember views persist even if the referenced table is deleted. So we don't even know what view is affected by the changes and can't even tell the user about that. And that is for all statements, even simple non-WITH ones.

Rollback may be required if the foreign_key_check in step 10 fails. It is interesting (and helpful) that the check is performed even if foreign_keys is OFF:

@mgrojo is doing that in his PR. So it should be fine 😄

Any simultaneous access to the database might cause any of the interim steps to fail the transaction. You might minimize this by setting a long timeout and using an EXCLUSIVE transaction.

That's a good point. We might want to look into that. Thanks 😄

@mgrojo mgrojo referenced this issue Aug 9, 2018

Closed

Cant modify table if trigger/FK exists #1398

3 of 10 tasks complete

@mgrojo mgrojo referenced this issue Sep 14, 2018

Closed

Not Prompted To Save Changes After Amending Table #1530

2 of 11 tasks complete
@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 8, 2018

Looking the two issues we have marked as "release blockers" for 3.11.0, is this one still current?

@MKleusberg

This comment has been minimized.

Copy link
Member

MKleusberg commented Dec 8, 2018

Pretty sure we can close this. The main issue here is definitely solved by @mgrojo. Not sure if we should keep this open for the remaining questions but either way, the unexpected data loss issue is gone 😄

@justinclift

This comment has been minimized.

Copy link
Member

justinclift commented Dec 8, 2018

Cool. Lets remove the Release Blocker tag. We can figure out the other bits later. 😄

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