improvement: replace pairwise operation ordering with a dependency-graph topological sort#798
improvement: replace pairwise operation ordering with a dependency-graph topological sort#798jechol wants to merge 1 commit into
Conversation
a2614b3 to
8d2a4f1
Compare
jechol
changed the title
zachdaniel
left a comment
zachdaniel
left a comment
There was a problem hiding this comment.
This is very cool and I was always aware that the pairwise "after" was a hack, glad that you've invested the energy to create a better way 🤩
Have some comments/questions and will need to do a deeper review soon of the specific fact emissions etc.
|
|
||
| #{described} | ||
|
|
||
| This usually means a structural foreign key (or other declared \ |
There was a problem hiding this comment.
If this happens it's not something the user can really do anything about or should have to address. We should explain that this is almost certainly a bug and that they should open an issue.
There was a problem hiding this comment.
Fixed — the message now says it's most likely a bug and asks to open an issue.
| {:column_renamed_from, key(table, schema, old_attribute.source)}, | ||
| {:any_attribute_added, key(table, schema)}, | ||
| {:table_touched, key(table, schema)}, | ||
| {:table_finalized, key(table, schema)} |
There was a problem hiding this comment.
Thinking maybe we should describe these facts somewhere because I'm not fully understanding for example why adding an attribute provides "table finalized".
There was a problem hiding this comment.
Added a "Facts" section to the moduledoc since — every fact is described there now. On the specific example: requiring a fact doesn't mean waiting for one provider — it creates an edge from every operation in the batch that provides it, and if nothing provides it, it's vacuously satisfied (the structure already exists from earlier migrations). That's why every structural op, including AddAttribute, provides the catch-all fact (since renamed to table_structure_ready): a single requirement then means "after all of this table's structural work in this batch" — or nothing at all if the table isn't being touched.
9c18a9b to
25e5eaa
Compare
041c825 to
d081ecd
Compare
…aph topological sort Replaced the ~69-clause pairwise after?/2 insertion sort in MigrationGenerator.sort_operations/2 with an explicit dependency-graph model: each operation provides/requires facts (operation_deps.ex), and toposort_operations/1 runs Kahn's algorithm over the resulting graph, raising OperationCycleError on a genuine cycle instead of silently falling back to an arbitrary order. The old insertion sort had two structural problems: it's non-transitive and clause-order-dependent (two pairs of clauses were found to be dead code — a broader/earlier clause always shadowed a more specific/later one for the same struct pair), and it had no general notion of an operation declaring a dependency on a fact, which the old global "always last"/"always first" tiers for AddCheckConstraint, AlterDeferrability, RemovePrimaryKeyDown, AddPrimaryKey papered over with hardcoded ad-hoc rules rather than real DDL requirements. streamline/2, group_into_phases/3, clean_phases/1 are unchanged; they only consume the final order. RenameTable's struct is normalized to use the same generic :table field every other operation struct uses (previously :old_table/:new_table), since the new ordering can place it next to code that reads op.table directly without special-casing rename operations. Verified against a large real-world Ash application (~150 resources) via a from-scratch schema regen applied to a real Postgres database, plus applying the same diff against a database restored from a production-sized backup, confirming the resulting schema matches in both cases. This surfaced two real bugs, both fixed here: - A resource's own schema option defaults to nil, but attribute.references.schema is loaded as an explicit "public" string — building fact keys from these without normalizing silently dropped real cross-table FK ordering constraints. - A unique index with a where/base_filter can reference columns that aren't part of its keys (e.g. a soft-delete-scoped identity whose where clause checks archived_at, a column outside the index's own keys), with no structured way to know which ones without parsing raw SQL. AddUniqueIndex now conservatively requires every attribute added to the table first, but only when such a filter is actually present: applying that margin unconditionally would also force a self- referencing attribute (one whose own FK targets this same index, e.g. a self-referential belongs_to) to finish before the index it depends on, creating a genuine two-way cycle between that attribute and the index. test/migration_generator_test.exs: 102/102 passing (two tests fixed for pre-existing fragility exposed by valid reordering). New test/migration_generator/operation_deps_test.exs unit-tests the provides/requires model directly. Full mix test: 860 passed.
d081ecd to
af738f1
Compare
|
Addressed all review comments (replies inline). The branch has changed quite a bit since your review — easier to re-review fresh than against the old diff. Two pre-existing ordering bugs were also found and fixed along the way (details in code comments and tests). One removal worth flagging: the custom-statement guard in Ready for another look. |
2 participants
Motivation
Migration operations were ordered by a ~69-clause pairwise
after?/2comparator feeding an insertion sort. It's non-transitive and clause-order-dependent — two clause pairs turned out to be dead code, where a broader/earlier clause always shadowed a more specific one for the same struct pair.It also had no way for an operation to declare "this depends on some fact being true yet." Global "always last" / "always first" tiers for things like
AddCheckConstraint,AlterDeferrability,RemovePrimaryKeyDown, andAddPrimaryKeypapered over what are really specific DDL ordering requirements with ad-hoc rules.What changed
Each operation now declares facts it
providesand facts itrequires(operation_deps.ex), andtoposort_operations/1runs Kahn's algorithm over the resulting dependency graph.A genuine cycle raises
OperationCycleErrorinstead of silently falling back to an arbitrary order — a later PR builds on this to letcustom_statementsopt into cross-table ordering, and a cycle there should fail loudly rather than produce a migration that runs in the wrong order.group_into_phases/3andclean_phases/1are unchanged — they only consume the final order.streamline/2is nearly unchanged: its custom-statement guard (from 12cf97d) is removed, since the structs it protected now carry:schemaand the toposort can't place a same-table custom statement inside an add/alter window.RenameTable's struct now uses the same generic:tablefield every other operation struct uses (previously:old_table/:new_table), andRemovePrimaryKeynow carries its:keysso its FK-ordering requirement can be column-scoped.Bugs found during verification
Verified against a large real-world Ash application (~150 resources): a from-scratch schema regen applied to a real Postgres database, plus applying the same diff against a database restored from a production-sized backup, confirming the resulting schema matches in both cases. That surfaced two real bugs, both fixed here:
schemaoption defaults tonil, butattribute.references.schemais loaded as an explicit"public"string — building fact keys from these without normalizing silently dropped real cross-table FK ordering constraints.where/base filter can reference columns that aren't part of its keys (e.g. a soft-delete-scoped identity whosewherechecksarchived_at, a column outside the index's own keys), with no structured way to know which ones without parsing raw SQL.AddUniqueIndexnow conservatively requires every attribute added to the table first, but only when such a filter is actually present: applying that margin unconditionally would also force a self-referencing attribute (one whose own FK targets this same index, e.g. a self-referentialbelongs_to) to finish before the index it depends on — a genuine two-way cycle between that attribute and the index.Tracing each fact's provider/consumer pairs also surfaced two pre-existing ordering bugs, fixed here with regression tests:
downthat recreates the constraint before re-adding the column it references. The dependency direction is now reversed: column removal/rename waits for the constraint's removal.Testing
test/migration_generator_test.exs: all passing (two tests fixed for pre-existing whitespace fragility the new ordering exposed), plus new integration tests pinning the generatedup/downorder for the bugs above. Newtest/migration_generator/operation_deps_test.exsunit-tests the provides/requires model directly. Fullmix test: 858 passed.Writing up what each fact means (in response to review) surfaced facts that were carried over from the old
after?/2rules but had no remaining consumer once traced through — each was removed individually and verified against the full suite. Every remaining fact has a documented provider/consumer pair and test coverage.Contributor checklist
Leave anything that you believe does not apply unchecked.