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

deleteAll without filter: FetchedRecordsController's trackChanges not called #156

Closed
comaxime opened this issue Dec 18, 2016 · 20 comments
Closed
Labels

Comments

@comaxime
Copy link

When performing:
Record.deleteAll(db)
the associated FetchedRecordsController does not notify of the changes (trackChanges not triggered)

but when performing:
Record.filter(1 == 1).deleteAll(db)
it does :)

@groue
Copy link
Owner

groue commented Dec 18, 2016

Hello!

It looks like you've being bitten by the truncate optimization which GRDB is supposed to handle correctly since v0.85.0. And indeed I can't reproduce your issue: I wonder whether you are using the latest GRDB version.

@comaxime
Copy link
Author

comaxime commented Dec 18, 2016

Thanks for the fast answer!
Am actually using GRDB.swift v0.98.0 (Xcode 8.2, simulator iPad Air 2 iOS 10.2, fresh install)

databasePool.write { db in try? Record.deleteAll(db) }

Any more details I could provide?

@groue
Copy link
Owner

groue commented Dec 18, 2016

OK, looks like you have found a real bug! Good thing you have a workaround until it is fixed.

Yes you can help, because I'm clueless: would you mind sharing some code, so that I can eventually reproduce the problem? Something like:

let controller = try FetchedRecordsController(dbPool, ...) // or dbQueue
controller.track { ... }
try controller.performFetch()

try dbPool.write { db in
    try Record.deleteAll(db)
}

// wait for changes that never come :'(

@comaxime
Copy link
Author

I have tested with this attached file:
TestDeleteAll.swift.zip

A fetched records controller is tracking the changes:
1- Insert 3 records, filter(1==1).deleteAll, the fetched records controller prints the changes
2- Insert 3 records, deleteAll, the fetched records controller does not print anything

Here is what gets printed:
Delete all records with filter(1 == 1).deleteAll(db)
RecordsDidChange triggered: number of records = 0

then, after inserting again 3 records:
Delete all records with deleteAll(db)

recordsDidChange is not triggered

@groue
Copy link
Owner

groue commented Dec 18, 2016

OK Maxime, I can reproduce the bug. Thanks for the report!

@groue groue added the bug label Dec 18, 2016
@comaxime
Copy link
Author

OK, let me know if I can help any further (but I fear this is now beyond my competencies ;) )

@groue
Copy link
Owner

groue commented Dec 19, 2016

So far it's beyond mine as well ;-)

@groue
Copy link
Owner

groue commented Dec 19, 2016

@comaxime OK the bug is found! A fixed release will ship shortly.

@comaxime
Copy link
Author

Great, thanks.
Where was the bug hiding?

@groue
Copy link
Owner

groue commented Dec 19, 2016

The bug lies here in the StatementCompilationObserver type which tells GRDB what is happening to the database.

According to https://www.sqlite.org/lang_delete.html#truncateopt:

When the WHERE is omitted from a DELETE statement and the table being deleted has no triggers, SQLite uses an optimization to erase the entire table content without having to visit each row of the table individually.

This is what hides deletions from transaction observers.

The truncate optimization can also be disabled at runtime using the sqlite3_set_authorizer() interface. If an authorizer callback returns SQLITE_IGNORE for an SQLITE_DELETE action code, then the DELETE operation will proceed but the truncate optimization will be bypassed and rows will be deleted one by one.

The fix will be to return SQLITE_IGNORE for SQLITE_DELETE there, and to write robust tests!

@groue groue closed this as completed in e8a2291 Dec 20, 2016
@groue
Copy link
Owner

groue commented Dec 20, 2016

@comaxime GRDB v0.99.0 is out (changelog). Thanks for the report!

@comaxime
Copy link
Author

Thanks @groue for the fast action!

@comaxime
Copy link
Author

@groue have just tried v0.99.0, I still have the issue ;)

@groue
Copy link
Owner

groue commented Dec 20, 2016

Looks like I missed something…

@groue groue reopened this Dec 20, 2016
@egrimmius
Copy link

I don't know if this is an unintended consequence of the fix for this issue, but this new error I just received with version 0.99 seems to be related. This is within DatabaseMigrator's registerMigrationWithDisabledForeignKeyChecks. Here's the output:

INSERT INTO table_New SELECT * FROM oldTable
DROP TABLE "oldTable"
ROLLBACK TRANSACTION
error occurred in grdb: SQLite error 1 with statement ALTER TABLE "table_New" RENAME TO "oldTable": there is already another table or index with this name: oldTable

This same code was working fine with version 0.97.

@groue
Copy link
Owner

groue commented Dec 20, 2016

@egrimmius: your issue is unrelated. Please enable SQL tracing, look at the executed SQL, and if something still looks odd to you, open a new issue.

@groue
Copy link
Owner

groue commented Dec 20, 2016

@comaxime I'm dazzled. But not defeated yet!

@groue groue closed this as completed in 000f1d0 Dec 20, 2016
@groue groue reopened this Dec 20, 2016
@groue
Copy link
Owner

groue commented Dec 20, 2016

@comaxime SQLite is full of traps, but I think I tackled it this time: I can run your test file and get all needed notifications:

Insert 3 records
RecordsDidChange triggered: number of records = 3

Delete all records with filter(1 == 1).deleteAll(db)
RecordsDidChange triggered: number of records = 0

Insert 3 records
RecordsDidChange triggered: number of records = 3

Delete all records with deleteAll(db)
RecordsDidChange triggered: number of records = 0

Is it possible for you to run your app with commit 000f1d0 ? This would prevent me from shipping a half-baked release this time. If not, I'll release a 0.99.1 version, hoping for the best.

When testing, make sure that the fetched records controller is kept alive: as soon as it is deallocated, it stops observing transactions, and may miss the last database update. So keep your instance of TestDeleteAll safe somewhere.

@groue groue closed this as completed in 302b9c2 Dec 21, 2016
@groue
Copy link
Owner

groue commented Dec 21, 2016

@comaxime Version 0.99.1 has shipped.

@comaxime
Copy link
Author

Thanks @groue, indeed I confirm this is fixed :)

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

No branches or pull requests

3 participants