New optional column: item_subtype

[jaredbeck]

Gives PT-AT the data it needs to fix #594. PT-AT will make changes like:

# paper_trail_association_tracking/reifiers/has_one.rb
# def load_versions(assoc, model, transaction_id, version_at, base_class_name)
            where("#{version_table_name}.item_type = ?", base_class_name).
+           where("#{version_table_name}.item_subtype = ?", assoc.class_name).
            where("created_at >= ? OR transaction_id = ?", version_at, transaction_id).

I have tested the above with the "cars and bicycles" example in person_spec.rb and it passes.

Replaces (reverts) #1108, which has proved problematic in #1135, leading to further proposed increases in complexity such as seanlinsley#1. @lorint I know you worked hard on #1108, and I did too, but if this is actually the simpler approach, as I propose, I hope we can both approach this objectively. I've never had to revert something as large as #1108 and I don't suggest it lightly.

I haven't written the docs or the migration to populate item_subtype. I would like to get your opinions on this approach first.

I made this an optional column because it is my understanding that this issue (#594) only affects PT-AT users with STI, a very small subset of our users.

[lorint]

Would be nice to have a generator similar to update_sti that fills in item_subtype for historic records.

[lorint]

Might as well at least keep this enhancement to the spec!

[lorint]

Small typo

[lorint]

If you're committed to going in this direction then it might be somewhat moot at this point, but I've pushed an update to Sean's PR#1 based on your review.

[jaredbeck]

1. Lorin, it sounds like you agree that this will give PT-AT the data it needs to solve Reify on associations fails if using Single Table Inheritance #594. Is that correct?
2. Sean, can you test your app against this branch? I assume that, because I've reverted Fix for issue #594, reifying sub-classed models that use STI #1108, that this branch fixes your issue Unable to join versions association #1135.

[seanlinsley]

I'll test this as soon as I can, but in the meantime these tests should verify the behavior if you'd like to add them to this PR:

• 3c08782
• https://github.com/seanlinsley/paper_trail/blob/cd8a5ca87986cab3df353e4a9b749e1333d5fd45/spec/models/management_spec.rb

[seanlinsley]

This PR fixes both issues 🙌

[jaredbeck]

.. in the meantime these tests should verify the behavior if you'd like to add them ..

Tests added.

[seanlinsley]

This is probably the most maintainable fix for #594 since it doesn't require patching Rails. @lorint what do you think about the approach in general? (save from it not having a migration generator)

BTW the test failures can be fixed by adding group(:id)

[seanlinsley]

We're now using this branch in production & will likely remove our object column next week.

Thank you both for all the time you've put into the upcoming release 💛

[jaredbeck]

@lorint does this work for you? Does it provide PT-AT with the data it needs to solve #594? Please let me know if you have any objections. If I don't hear from you, I'll probably merge this weekend.

[lorint]

It does have the data we would need, and potentially covers Rick Tyler's interest around the store_base_sti_class gem.

It does require a fair bit of rework internally, doing checks around item_type and item_subtype. And around 16GB of additional storage requirement for this new column. But this will get offset by the possible removal of the object column in the near future.

We are considering both removing object and implementing the item_type code from PR#1137, in which case we could backport our fixes that use item_type for reification to PT-AT using item_subtype.

[jaredbeck]

It does have the data we would need, and potentially covers Rick Tyler's interest around the store_base_sti_class gem.

Great, thanks.

And around 16GB of additional storage requirement for this new column.

If y'all are concerned about disk space, we could choose to store item_subtype only when it differs from item_type.

[jaredbeck]

We're now using this branch in production.

I usually delete topic branches immediately after merging, so please switch to master ASAP.

[lorint]

If y'all are concerned about disk space, we could choose to store item_subtype only when it differs from item_type.

For us the disk space is of interest only in light of how far-reaching it is to have a half billion records loaded into RAM at once. We've got 5 covering indexes that leverage item_type, and there's some right magic we're able to unfold for our end users based on this, 2 of these 5 b-tree indexes have item_type as their primary index. Hence any even slight update to behaviour around item_type can mean significant impact on a fair range of tests, and around the amount of work the database engine goes through overall. No worries about extra overhead during CUD operations. Entire focus is on finding related transactions quickly. For us in these indexes we will continue to store real class names as opposed to base_class so we can have just one column involved, and maintain consistent sub-second searches on item_type.

[jaredbeck]

For us the disk space is of interest only in light of how far-reaching it is to have a half billion records loaded into RAM at once.

Dang. And I thought Sean has a lot of records 😅

.. 2 of these 5 b-tree indexes have item_type as their primary index ..

db indexing practices are a personal interest of mine. I'd love to hear more about the shape of the queries and brainstorm about your index design. If it's not private IP, feel free to email me and we can chat more about about it.