Intermittent Database Corruption

[blitzmann]

We need to look into methods of avoiding database corruption. Many times out users run into issue in which they can't open their damage profile window, or their characters are jacked. This is usually due to database corruption and orphaned references.

At the very minimum we should have some sort of database cleaning function in the help menu / preferences. This is kinda ehh, because it will need to be manually maintained whenever new fields / tables are added (which isn't often, so not too big of a problem). It would basically run custom queries to determine things to delete.

I would prefer to figure this out at a lower level though. Either SQLite or SQLAlchemy seem to reuse PK IDs, so if you have four characters and delete one, the next one will have an ID of 4 instead of 5. There has to be some way of disabling this functionality.

Open to other suggestions.

[resinneublem]

Do we even know how these databases are being "corrupted"? Has anyone who has experienced this uploaded their database to be researched?

This sounds like it's a problem with the save/delete pyfa code orphaning stuff. So the best approach I can think of is:

1. Make it easy for someone to report a database problem to get the database into the hands of a developer. Maybe even submitting from inside the application.
2. Have a dev figure out where things went wrong.
3. Fix the code that orphaned references.

For 2) it may be easier to find and fix by implementing a pseudo binary log that can be replayed and bisected. http://souptonuts.sourceforge.net/readme_sqlite_tutorial.html (Under "Logging All Inserts, Updates, and Deletes")

sqlite also has limited Foreign Key support. Adding those can also prevent orphans from ever occurring.

[blitzmann]

Do we even know how these databases are being "corrupted"? Has anyone who has experienced this uploaded their database to be researched?

A few folks have sent me their databases. It usually turns out that the skills were not all deleted when a character was deleted. Damage patterns also have issues sometimes, can't remember off the top of my head why though.

Fix the code that orphaned references.

This is harder than it sounds apparently. SQLAlchemy should be handling this with it's references. We call cascade="all,delete-orphan" option for nearly everything...

And the problem with logging is that it's only useful if it's reproducable. The things that cause these errors tend not to be. And I don't want to enable logging by deafult in the off chance a user has an issue. =/

sqlite also has limited Foreign Key support. Adding those can also prevent orphans from ever occurring.

We already have those

[resinneublem]

Is anything from the application telling sqlite to use them?

Assuming the library is compiled with foreign key constraints enabled, it must still be enabled by the application at runtime, using the PRAGMA foreign_keys command. For example:

sqlite> PRAGMA foreign_keys = ON;

Foreign key constraints are disabled by default (for backwards compatibility), so must be enabled separately for each database connection....

https://www.sqlite.org/foreignkeys.html

It might be that they're just being ignored and that's how characterSkills can still live even after the character is deleted.

[blitzmann]

I just had a ForeignKey constraint exception when working on alpha clone feature, so I do believe that they are working... Further more, the ORM works via foreign keys. If they weren't implemented correctly, then things like items getting assigned their attributes wouldn't work either.

=/ Phantom bugs yay!

[Ebag333]

The DB attributes are done in a very old fashioned way for sqlAlchemy.

Additionally there's the weird scenario where we have cyclical references, so they could be getting imported multiple times (or not at all). Theoretically the mapper should be imported if the table is, but the way we import is....uh....kinda weird.

I'm not convinced that the tables and relationships are setup correctly. Obviously they're at least mostly setup correctly or it wouldn't work at all, but there's certainly some...oddities...in there.

There's also a lack of standardization, not all the tables are setup the same way with the same requirements. So there's potential for issues there.

When @a-tal and I reworked the imports, I could no longer get the tables to import properly. This was partially because of the cyclical imports in the mappers, but even just straight up table definitions I was having trouble with.

You can see how I reworked them here:
https://github.com/pyfa-org/Pyfa/blob/63323dc84cc81fdfef88296a5b35098ff560cf57/eos/db/saveddata/mapper.py

That's the newer (and recommended) method of doing tables. (Ignore the two non-table function definitions in there...)

I'm not saying that reworking the tables will fix all our problems, or even any of them. Just had issues with the tables for saveddata (and perhaps not coincidentally, no issues with the tables for the static data, in that branch we're using the original/old method of doing it).

[blitzmann]

I'm not convinced that the tables and relationships are setup correctly.

FOREIGN KEY constraint failed: delete from characters where id = 3 when trying to delete one of my characters through the database itself without deleting the skills. So, at least with respect to characters, this is working correctly.

The DB attributes are done in a very old fashioned way for sqlAlchemy.

Classic vs Declarative should make absolutely no difference here.

I was also not aware you were converting our table definitions and mapping to declarative. Why was this not discussed / why exactly is that mixed in with the imports stuff? The smaller the changes the better.

[Ebag333]

why exactly is that mixed in with the imports stuff?

I could no longer get the tables to import properly

[blitzmann]

I could no longer get the tables to import properly

What was the problem with the table imports? Please tell me all this refactoring isn't because of a single import. If that's a case, it's a huge overreaction. =/

[Ebag333]

They simply weren't loading. I don't have the exact error, but it was along the lines of could not find or load table. Simply switching to Declarative fixed it (all else stayed the same). Why? vOv

I don't think it was strictly the refactoring that was done, I refactored the static data in the same way (put them into their own classes) and it worked fine.

[blitzmann]

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

http://docs.sqlalchemy.org/en/latest/dialects/sqlite.html#sqlite-auto-incrementing-behavior

It's known that the rowid can be reused, which is one of the reasons we get these errors occasionally. This is a way to make SQLite behave like every other RDMS and not reuse IDs. This doesn't solve the integrity issues, but it would prevent crashes (and make things a bit more understandable considering the way it currently works is not really standard).

I'll be considering adding this to the codebase after some evaluations.

[blitzmann]

See #949 (comment) for a cause of known corruption for damage profiles

[blitzmann]

closing this issue as most known corruption is handled automatically now =3