Avoid N+1 in postgresql migrations #1612
Conversation
| import qualified Database.PostgreSQL.Simple.Types as PG | ||
|
|
||
| import qualified Blaze.ByteString.Builder.Char8 as BBB | ||
| import Control.Arrow |
There was a problem hiding this comment.
This file was getting pretty big, so I wanted to move all of the migrations stuff into a separate one. A lot of the code here is unchanged: in essence, all I've done is extract the querying part out so that it happens first, and so that we pull all the data we need at once (with 4 queries) rather than doing N+1s.
| backend <- ask | ||
| pure (SqlBackend.connPrepare backend) | ||
|
|
||
| -- NB: we do not perform these migrations in main.hs |
There was a problem hiding this comment.
the test suite is kind of cursed in that it leaves the schema in the test DB, because this means that if there's a bug in the code that determines what migrations you need to apply, you only see test failures if you run the tests twice in a row (if you're starting from a fresh DB).
I think I've made things slightly worse here by doing this... but I also think it's important to directly exercise the migrator like this.
|
|
||
| let | ||
| expected = | ||
| SchemaState |
There was a problem hiding this comment.
This almost acts like a golden test. If you want to change the expected output, the easiest thing to do is to just rerun the test, eyeball the "but got: ..." to make sure it makes sense, and then copy it into the code here.
| collectSchemaState | ||
| :: (Text -> IO Statement) -> [EntityNameDB] -> IO (Either Text SchemaState) | ||
| collectSchemaState getStmt entityNames = runExceptT $ do | ||
| existence <- getTableExistence getStmt entityNames |
There was a problem hiding this comment.
Each of these four functions performs exactly one query, and these are all of the queries the migrator now needs to perform.
| (errs, _) -> throwError (T.intercalate "\n" errs) | ||
| where | ||
| getTableExistenceSql = | ||
| "SELECT tablename FROM pg_catalog.pg_tables WHERE schemaname != 'pg_catalog'" |
There was a problem hiding this comment.
These are all basically the same queries as before, but instead of doing where tablename = ?, I'm doing where tablename = ANY (?), and substituting with the full list of tables.
| ([], xs) -> pure $ Map.unionsWith Map.union xs | ||
| (errs, _) -> throwError (T.intercalate "\n" errs) | ||
| where | ||
| -- TODO: should this filter by schema? |
There was a problem hiding this comment.
I noticed that the existing query doesn't filter by schema and I feel like it maybe should? but that's one for later I think
| allDefs | ||
| entity | ||
| (newcols, udspair) | ||
| (map dubiouslyRemoveReferences essColumns, Map.toList essConstraints) |
There was a problem hiding this comment.
This function is basically the same as before except for this one line - in the previous version, we were relying on getColumn to implicitly remove these references (or rather not fetch them in the first place), now we're doing it explicitly here
| -- otherwise no-op, `getAlters` will handle dropping this for us. | ||
| oldCol | ||
|
|
||
| -- | Indicates whether a Postgres Column is safe to drop. |
There was a problem hiding this comment.
Everything from here downwards is exactly the same as it was before
hdgarrood
changed the title
|
@parsonsmatt I found that with the version of these changes that you previously approved, when running against the work codebase, I saw a few mismatches in the case where tables in question had multiple (sometimes redundant) foreign key constraints on the same table, because the previous version of this PR didn't handle that case well. In particular, persistent could suggest migrations that fail to apply - it assumed that a given column could only have one FK constraint, which meant that if a column happened to have two identical FK constraints on it, it could suggest dropping the redundant one and creating the other, not realising that the other also already exists, which would of course fail. The most recent commits I've added resolve this problem by restoring the current behaviour, which is that we ignore all FK constraints on existing tables when migrating, except for the specific case where:
|
|
Okay, although this now works, it appears to be even slower than the previous version. So that needs fixing first too. |
3 participants
Fixes #1611. With these changes, we can migrate an arbitrary number of entities with just 4 queries.
Before submitting your PR, check that you've:
@sincedeclarations to the Haddockfourmoluon any changed files (restyledwill do this for you, soaccept the suggested changes if it makes them)
.editorconfigandfourmolu.yamlfiles for details)After submitting your PR:
(unreleased)on the Changelog