me and @tbrisker are doing review review of our database indices, so far we have been able to identify missing or extra indices. Next, I want to take a closer look on foreign keys. First of all, current status of this in Rails.
Rails 4.x added add_foreign_key method and we use that in our codebase. It looks like we missed about four dozens of foreign keys tho, I have a list and I can add them in my upcoming PR.
But when I was testing that, I quickly realized that SQLite3 does not actually work with foreign keys in Rails. This is because ALTER TABLE syntax is very picky in SQLite3 and Rails does not workaround that.
While the database itself supports it, this needs to be explicitly enabled by PRAGMA statement and it is nontrivial to do that. From Rails 4.2 release notes:
The migration DSL now supports adding and removing foreign keys. They are dumped to schema.rb as well. At this time, only the mysql, mysql2 and postgresql adapters support foreign keys.
Now, I am going to add missing foreign keys migrations in my upcoming PR but we need to be aware that this does not work (it never worked) for SQLite3. This is another strong argument why we should be all using PostgreSQL or MySQL for development. I don’t do this myself, full disclosure - because it’s so convenient to stay with sqlite3. I think it’s a time for change.
Please let’s not start discussion about dropping sqlite3 completely, I don’t like this idea. Let’s not make it more difficult for newcomers to start contributing to Foreman. It’s enough if most of core developers use postgresql, we still have Jenkins. Let’s keep it.
As part of my upcoming PR (Refactor #24573: Adding missing foreign keys and indices - Foreman) I am going to clean up indices and my plan is to enable active_record_doctor on Jenkins so any commit that messes up with indices is not approved. I will need help with this, please ping me if you are able to guide me or write jenkins job for this (it’s just a rake command and return value that does the check).
Now, the big question which I actually wanted to discuss here is whether we want to keep status quo about foreign keys, remove them completely or add those which are missing.
It looks like we are missing many foreign keys and nothing bad really happened. This questions whole presence of it. Of course, by design you want to have consistent references in a database, but it used to cause pains for Rails app in the past. Not sure how much of this changed in recent versions tho.
The other extreme is also problematic. As @tbrisker mentioned in the PR, we can’t add them all, for example the way we delete logs/reports won’t allow this, we could add most of them.
What are the motives to move away from one extreme to another? I marked status quo as I am not really familiar with the benefits of either of the approaches.
It’s always a good idea to use foreign keys from the day one of a project. The major reason is database consistency. But we could not use it because Rails 4.1 or older did not have that and it struggled with it. Now Rails supports it, we attempted to add some of them but we were not good in maintaining those.
It is important to understand that foreign key is just a database constraint, it is not an index. Well, in most databases, except MySQL - this system automatically creates an index as well. Which is a good idea in most cases anyway.
It’s hard to add a foreign key to existing column because this can lead to problems during migration (what to do with orphaned records). Also this can create regressions. And finally since foreign key usually means adding an index (in MySQL this is enforced, for other DBs it is “usually” good idea), this can lead to performance degradation for updates.
It’s a tradeoff, the more I think about this more I am not sure what is the best approach and I start to lean towards status-quo, maybe with small review and adding only the most obvious foreign keys/indices.
