965084 - "ROLLBACK TO SAVEPOINT savepoint" doesn't fire appropriate triggers leaving orphaned files

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Jan Varga [:janv]]

I think this is a bug in sqlite. Basically, before each DoDatabaseWork() we start a savepoint and release/rollback to it according to the result of the method. The problem is when the AddHelper successfully adds a new row to the object_data table, but it fails to update index data tables. All rows are remove from the tables, but sqlite doesn't fire the delete trigger to release file infos that were created for blobs/files.

In other words, this only happens when we get a constraint error caused by filling an index and the error handler in JS calls event.preventDefault().

This probably doesn't happen very often, but once it happens the stored files will never be deleted. Only when entire database or origin is deleted (reboot/restart doesn't help here)

We need to fix this especially for bug 964561, because storing structured clones as files makes this bug more visible.

Comment 1

[D. Richard Hipp]

When you do a SAVEPOINT in SQLite, the engine merely remembers the current state of the database.  Later when you do ROLLBACK TO SAVEPOINT, the engine restores the database content to what it was at the SAVEPOINT.  No DELETE, INSERT, or UPDATE operations occur for this, so no triggers are fired.

Conceptually, SQLite makes a copy of the database file when you do a SAVEPOINT.  Then on ROLLBACK TO SAVEPOINT, it restores the database to that copy.  Key word: "Conceptually".  For efficiency reasons, SQLite does not really make a copy of the database on a SAVEPOINT; it uses other techniques that require far less I/O.  But the end result is the same.

Triggers are not needed to maintain the integrity of the SQLite database on a ROLLBACK since the database was in a valid state at the SAVEPOINT and will be in exactly the same state after the ROLLBACK.  But if I understand correctly, you have made changes to data structures external to SQLite, and you need some kind of notification of the ROLLBACK so that you can unwind those external changes.  Do I have that right?   A reasonable request, but triggers won't work for that.

Possible work-arounds:  (1) Do not allocate external resources until the transaction commits (an event which you can detect using the sqlite3_commit_hook() interface).  That way, if a ROLLBACK occurs, the resources where never allocated in the first place and don't need to be deallocated.  (2) Use a custom "virtual table" (http://www.sqlite.org/vtab.html) to create and manage the resources that need to be deallocated on ROLLBACK.  The virtual table API includes callbacks that fire on ROLLBACK TO SAVEPOINT and which you can use to deallocate as necessary.  (3) Keep track of what external resources need to be deallocated on ROLLBACK in your application logic and do the necessary deallocations just before or just after you tell SQLite to ROLLBACK.

Please contact the SQLite developers if you want to go with option (2) and would like assistance in coming up with a suitable virtual table.

Comment 2

[Jan Varga [:janv]]

Well it still sounds like a bug to me. Let's say I have defined an insert, update and delete trigger for a table. Now in case of a standard transaction (w/o savepoints), the insert trigger is fired immediately after a row is inserted. Then if the transaction is committed nothing else fires which is ok. If a rollback is called, then the delete trigger is fired which can undo what the insert trigger did.

In the case of "UPDATE" we had to enable recursive triggers to get the delete trigger be fired before the insert trigger (with disabled recursive triggers only the insert trigger fires).

So this all works ok.

Now, if I have a savepoint (nested transaction) in which a row is inserted and the operation fails because of a constraint error, then the insert trigger is not fired which is ok too.

However, if I have a savepoint in which a row is successfully inserted then the insert trigger is fired, but an additional operation fails, we call "ROLLBACK TO SAVEPOINT savepoint". One would expect that the delete trigger should be fired like in the case of standard simple transaction. But that doesn't work, the delete trigger is not fired.

These triggers are essential for reference counting of stored DOM blobs/files. We don't store them in the database, they are stored in separate files on the disk, only references are stored in the database.

Let me know if you need more details.

Comment 3

[Jan Varga [:janv]]

Anyway, it seems I can fix this in IDB implementation, it just won't be so nice.

Comment 4

[Dan Kennedy]

To clarify, rolling back either a savepoint (ROLLBACK TO) or a standard transaction (ROLLBACK) should never cause any triggers to be fired. In both cases, the database reverts to the state it was in when the transaction (or savepoint) was started. This means the effects of any triggers that modified the database during the transaction or savepoint are undone. But it doesn't work by firing DELETE triggers to undo the work of INSERT triggers or anything like that.

So, if you need to revert content stored externally whenever a database transaction or savepoint is rolled back, triggers alone won't do the job. See post (1) for some ideas.

> In the case of "UPDATE" we had to enable recursive triggers to get the 
> delete trigger be fired before the insert trigger (with disabled 
> recursive triggers only the insert trigger fires).

I imagine an IDB update is translated into a REPLACE or "INSERT OR REPLACE" statement. If recursive triggers are enabled and a row is REPLACEd, SQLite fires the delete triggers for the row being replaced and the insert triggers for the new row. If recursive triggers are disabled (the default) only the insert triggers are fired. See under "REPLACE" here:

  http://www.sqlite.org/lang_conflict.html

Comment 5

[Jan Varga [:janv]]

Ok, we'll fix it on our end. Thanks for all the info.

Comment 6

[Jan Varga [:janv]]

Attachment: patch v1

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8367531 [details] [diff] [review]
patch v1

Review of attachment 8367531 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

::: dom/indexedDB/IDBTransaction.cpp
@@ +1092,5 @@
>  
> +    if (mInSavepoint) {
> +      FileInfoEntry* unused;
> +      if (!mSavepointEntriesIndex.Get(id, &unused)) {
> +        mSavepointEntriesIndex.Put(id, entry);

Hm, rather than Get() followed by Put() why not just have a redundant Put()? Otherwise nuke the unused entry and just pass nullptr.

Comment 8

[Jan Varga [:janv]]

Oh, good idea, will try that.

Comment 9

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/35c57af24bf5

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/35c57af24bf5