Postgresql `constraint=` attribute support #979

robbassi · 2019-10-30T15:44:08Z

This PR adds support for the constraint= attribute in the Postgresql backend. It addresses part of issue #939. Unfortunately, I wasn't able to figure out how to accomplish this with sqlite, and I will make another PR for that once I get a better understanding of sqlite internals.

Before submitting your PR, check that you've:

Bumped the version number

After submitting your PR:

Update the Changelog.md file with a link to your PR
Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

robbassi · 2019-10-30T15:46:42Z

persistent-postgresql/Database/Persist/Postgresql.hs

-                , "AND constraint_name=?"
-                ]
-        let ref = refName tableName' cname
+        let sql = T.concat ["SELECT DISTINCT "


Due to the way postgresql stores the column metadata, I had to useDISTINCT here in order to avoid duplicate rows being returned for composite keys.

parsonsmatt

Looks great! Can you add a test to verify that it works?

parsonsmatt · 2019-10-30T18:20:20Z

persistent-postgresql/Database/Persist/Postgresql.hs

+                           case find (\f -> fk == foreignConstraintNameDBName f) fdefs of
+                             Just _ -> c { cReference = Nothing }
+                             Nothing -> c
+                         Nothing -> c) xs,ys)


On a purely stylistic note, I'd rather see this written as another named function so this definition could be:

excludeForeignKeys (xs, ys) = (map excludeForeignKey xs, ys) excludeForeignKey c = case cReference f of ....

parsonsmatt · 2019-10-30T18:20:51Z

persistent-postgresql/Database/Persist/Postgresql.hs

@@ -908,7 +920,7 @@ getAddReference :: [EntityDef] -> DBName -> DBName -> DBName -> Maybe (DBName, D
 getAddReference allDefs table reftable cname ref =
    case ref of
        Nothing -> Nothing
-        Just (s, _) -> Just $ AlterColumn table (s, AddReference (refName table cname) [cname] id_)
+        Just (s, constraintName) -> Just $ AlterColumn table (s, AddReference constraintName [cname] id_)


robbassi · 2019-10-30T18:43:55Z

Looks great! Can you add a test to verify that it works?

For sure!

robbassi · 2019-10-30T21:03:35Z

persistent-postgresql/test/CustomConstraintTest.hs

+  describe "custom constraint used in migration" $ do
+    it "alter table uses custom constraint name" $ runDb $ do
+      ms <- getMigration customConstraintMigrate
+      let alter = find (== alterQuery) ms


This seems a bit hacky, but I couldn't think of a better way to ensure that the migration uses the correct constraint name. Below, we check the contraints table to ensure it is created with the correct name, but if that fails, it is not clear why. I figured the most common reason for a failure would be the alter table query using the wrong name.

@parsonsmatt Thoughts?

I ended up removing this test case as it makes the tests non-idempotent. Now we just ensure the constraint exists after running the migration.

parsonsmatt

Awesome!! One more change though - this is a modification to persistent-postgresql, not persistent, and the version bump and changelog entry should point there and not in persistent.

parsonsmatt · 2019-10-31T19:01:02Z

persistent/ChangeLog.md

+## 2.10.5
+
+* Added support for the `constraint=` attribute to the Postgresql backend. [#979](https://github.com/yesodweb/persistent/pull/979)
+


This should go in persistent-postgresql/changelog.md

parsonsmatt · 2019-10-31T19:01:52Z

persistent/persistent.cabal

@@ -1,5 +1,5 @@
 name:            persistent
-version:         2.10.4
+version:         2.10.5


Should go in persistent-postgresql/persistent-postgresql.cabal

This reverts commit d67dc37.

This reverts commit 142856d.

parsonsmatt · 2019-10-31T19:51:52Z

Once CI is passed I'll merge and release. Thanks so much!

robbassi · 2019-10-31T20:00:42Z

Awesome. Thank you for the prompt replies!

MaxGabriel · 2020-01-03T22:13:28Z

Oh, heads up that this change seems to break certain foreign key setups. Still investigating, but for example for this table:

mercury-web-backend-test=# \d+ synapse_recipient_ach_nodes
                                          Table "public.synapse_recipient_ach_nodes"
          Column           |           Type           | Collation | Nullable | Default | Storage  | Stats target | Description
---------------------------+--------------------------+-----------+----------+---------+----------+--------------+-------------
 recipient_payment_data_id | uuid                     |           | not null |         | plain    |              |
 bank_account_id           | uuid                     |           | not null |         | plain    |              |
 ach_node_id               | text                     |           | not null |         | extended |              |
 created_at                | timestamp with time zone |           | not null |         | plain    |              |
 updated_at                | timestamp with time zone |           | not null |         | plain    |              |
Indexes:
    "synapse_recipient_ach_nodes_pkey" PRIMARY KEY, btree (recipient_payment_data_id, bank_account_id, ach_node_id)
Foreign-key constraints:
    "synapse_recipient_ach_nodes_ach_node_id_fkey" FOREIGN KEY (ach_node_id) REFERENCES synapse_ach_nodes(node_id)
    "synapse_recipient_ach_nodes_bank_account_id_fkey" FOREIGN KEY (bank_account_id) REFERENCES bank_accounts(id)
    "synapse_recipient_ach_nodes_recipient_payment_data_id_fkey" FOREIGN KEY (recipient_payment_data_id) REFERENCES recipient_payment_data(id)
    "synapse_recipient_ach_nodes_recipient_payment_data_id_fkey1" FOREIGN KEY (recipient_payment_data_id, bank_account_id) REFERENCES recipient_accounts(recipient_payment_data_id, bank_account_id)
Triggers:
    synapse_recipient_ach_nodes_insert BEFORE INSERT ON synapse_recipient_ach_nodes FOR EACH ROW EXECUTE PROCEDURE create_timestamps()
    synapse_recipient_ach_nodes_update BEFORE UPDATE ON synapse_recipient_ach_nodes FOR EACH ROW EXECUTE PROCEDURE update_timestamps()

the query returns:

[[PersistText "recipient_accounts",PersistText "synapse_recipient_ach_nodes_recipient_payment_data_id_fkey1"],[PersistText "recipient_payment_data",PersistText "synapse_recipient_ach_nodes_recipient_payment_data_id_fkey"]]

robbassi · 2020-01-06T20:17:01Z

@MaxGabriel Good catch, I should be able to look into this tonight.

jkeuhlen · 2020-01-07T16:01:34Z

@robbassi Were you able to look into it? If not, it's on my list today and I'm happy to dig in/help take a look.

robbassi · 2020-01-07T16:53:48Z

@jkeuhlen Ok great. I'm planning to work on it today, but any help would be appreciated!

jkeuhlen · 2020-01-07T18:13:59Z

persistent-postgresql/Database/Persist/Postgresql.hs

+          [] -> return Nothing
+          [[PersistText table, PersistText constraint]] ->
+            return $ Just (DBName table, DBName constraint)
+          _ -> error "Postgresql.getColumn: error fetching constraints"


@robbassi So this is the place where the error is thrown. Is there a reason you assume a single constraint per table here and throw the error? A simple idea of a schema that this breaks is a table that records sales like:

Sales buyer PersonId seller PersonId Person name Text

robbassi · 2020-01-07T19:29:05Z

@jkeuhlen I beleive the issue here is with multiple foreign key contraints for a single column, not the table. This was actually ported from the MySQL implementation (#937) for custom constraint names. I just verified that this issue also exists in the MySQL backend.

To fix this we'll need to come up with a better way of fetching the column metadata when building migrations, let me know if you have any ideas.

jkeuhlen · 2020-01-07T19:42:51Z

@robbassi Yeah that makes sense. I was just writing a test for this case, and it was surprised that it passed (since I was testing the table not the column). Would it be sufficient to change the definition of Column?

data Column = Column
    { cName      :: !DBName
    , cNull      :: !Bool
    , cSqlType   :: !SqlType
    , cDefault   :: !(Maybe Text)
    , cDefaultConstraintName   :: !(Maybe DBName)
    , cMaxLen    :: !(Maybe Integer)
    , cReference :: !(Maybe (DBName, DBName)) -- table name, constraint name
    }
    deriving (Eq, Ord, Show)

Specifically, the column reference could be changed to [(DBName, DBName)]. I'm not familiar enough with persistent to know the full ramifications of that decision though.

robbassi · 2020-01-07T20:09:34Z

@jkeuhlen I think something like that should work. Though, I'm trying to better understand the use case here. I don't believe persistent can generate tables like this through migrations. Which leads me to believe that this was some pre-exisiting table that @MaxGabriel was trying to run a migration against. @MaxGabriel could you let us know if this table was created through a persistent migration?

jkeuhlen · 2020-01-07T20:14:00Z

oh @robbassi I work with @MaxGabriel so I can confirm (and test with his original issue). We aren't creating any of these through the persistent migrations. But we do check our table definitions against the models with persistent and that's where we run into the problem.

robbassi · 2020-01-07T20:20:34Z

@jkeuhlen Ok cool. So when you say you check the table definitions against the model, is that essentially running a migration and seeing what changes?

jkeuhlen · 2020-01-07T20:30:44Z

@robbassi Exactly, we run a migration and capture the output, then do some tweaking internally since we know of a few mismatches, then fail if the generated migrations don't match the models (minus our expected mismatches).

jkeuhlen · 2020-01-07T20:31:28Z

And the failure occurs during migration generation at the line I pointed out here: https://github.com/yesodweb/persistent/pull/979/files#r363880641

robbassi · 2020-01-07T20:43:28Z

@jkeuhlen Ok got it. I'm going through the code now to see if there is an easier fix, otherwise I'll look into modifying the Column definition.

jkeuhlen · 2020-01-07T20:54:50Z

@robbassi Awesome, thanks! Let me know if I can help in any way (failing test cases maybe?).

robbassi · 2020-01-07T21:12:54Z

@jkeuhlen That would be great! I created a branch for the fix here: robbassi/persistent/tree/fix-migration-multiple-fk

jkeuhlen · 2020-01-07T22:19:30Z

@robbassi So I have a pretty reliable way to make things break, but I don't really understand why they are breaking.

Using this schema:

share [mkPersist sqlSettings, mkMigrate "customConstraintMigrate"] [persistLowerCase|
CustomConstraint1
    some_field Text
    deriving Show

CustomConstraint2
    cc_id CustomConstraint1Id constraint=custom_constraint
    deriving Show

CustomConstraint3
    cc_id1 CustomConstraint1Id constraint=cc_id1_custom_constraint1
    cc_id2 CustomConstraint1Id constraint=cc_id2_custom_constraint2
    deriving Show
|]

This all seems to work fine actually. However, when the constraints aren't specified in the schema, things go awry. In this case, I just comment out the constraint= syntax and run the generated migrations manually:

    it "allows constraints not expressed in persistent" $ runDb $ do
      runMigration customConstraintMigrate
      rawExecute "ALTER TABLE \"custom_constraint3\" ADD CONSTRAINT \"cc_id1_custom_constraint1\" FOREIGN KEY(\"cc_id1\") REFERENCES \"custom_constraint1\"(\"id\")" []
      rawExecute "ALTER TABLE \"custom_constraint3\" ADD CONSTRAINT \"cc_id2_custom_constraint1\" FOREIGN KEY(\"cc_id2\") REFERENCES \"custom_constraint1\"(\"id\")" []
      vals <- getMigration customConstraintMigrate
      liftIO $ print vals
      pure ()

Leads to:

       uncaught exception: ErrorCall
       Postgresql.getColumn: error fetching constraints. Expected a single table and a single constraint for table: custom_constraint3 and a column: cc_id1 but got: [[PersistText "custom_constraint1",PersistText "cc_id1_custom_constraint1"],[PersistText "custom_constraint1",PersistText "custom_constraint3_cc_id1_fkey"]]
       CallStack (from HasCallStack):
         error, called at ./Database/Persist/Postgresql.hs:823:13 in persistent-postgresql-2.10.1.1-JVkLBlDLwV8GsrinRLi7sQ:Database.Persist.Postgresql

Any idea what's going on here? It seems like the manual ALTER TABLE command is generating an extra custom_constraint3_cc_id1_fkey but I don't know where that one comes from. I'm also just running the output of the migration that persistent generated in the first place.

(N.B. this is using the slightly improved error message from https://github.com/yesodweb/persistent/compare/postgresFixConstraint)

robbassi · 2020-01-07T22:39:44Z

@jkeuhlen It looks like custom_constraint3_cc_id1_fkey is referencing cc_id1 from custom_constraint3. It's possible it was left over from another migration. If you drop it manually does it come back?

jkeuhlen · 2020-01-07T22:44:27Z

@robbassi Yeah, it still shows up with an entirely fresh DB (I dropped test, recreated it, and ran only this test case).

jkeuhlen · 2020-01-07T22:51:09Z

@robbassi custom_constraint3_cc_id1_fkey is the default constraint name that you get if you just do constraint instead of constraint=, so I wonder if that's where it comes from?

robbassi · 2020-01-07T22:58:09Z

@jkeuhlen Ah, so if you changed:

CustomConstraint3
    cc_id1 CustomConstraint1Id constraint=cc_id1_custom_constraint1
    cc_id2 CustomConstraint1Id constraint=cc_id2_custom_constraint2
    deriving Show

to something like:

CustomConstraint3
    cc_id1 CustomConstraint1Id
    cc_id2 CustomConstraint1Id constraint=cc_id2_custom_constraint2
    deriving Show

That would explain it.

jkeuhlen · 2020-01-07T23:06:01Z

@robbassi Yup. I realized after the fact that persistent adds the first foreign key automatically. I think this test case is a good illustration of the problem then, even if it is a trivial example. I'll PR it over to your branch.

The more complicated example would be like the one Max mentioned before where you have two different foreign keys on the same field because one is in a compound key.

robbassi · 2020-01-08T18:03:15Z

@jkeuhlen I updated my branch with the fix. I may still clean things up a bit, but the issue should be fixed now. I verified your test case passes now, but could you try out the original use case just to make sure we caught everything?

jkeuhlen · 2020-01-08T19:10:56Z

@robbassi The original use case passes now, thanks so much for fixing this!

robbassi · 2020-01-08T19:52:46Z

@jkeuhlen Glad I could help! I'll make a PR for this shortly, would you mind creating an issue for this? Just want to make sure this gets tracked properly.

robbassi added 3 commits October 30, 2019 11:32

Add new custom constraint name logic to Postgresql backend

f4802be

Fix test failures

6b35638

Bumped version number

142856d

robbassi commented Oct 30, 2019

View reviewed changes

Update changelog

d67dc37

parsonsmatt reviewed Oct 30, 2019

View reviewed changes

Refactored excludeForeignKeys for clarity

8ce0e70

Added test to ensure the custom constraint is used

6493074

robbassi commented Oct 30, 2019

View reviewed changes

Refactored tests

3d4504b

parsonsmatt requested changes Oct 31, 2019

View reviewed changes

robbassi added 2 commits October 31, 2019 15:37

Revert "Update changelog"

bd58dcf

This reverts commit d67dc37.

Bump version, update changelog for persistent-postgresql

7220023

parsonsmatt approved these changes Oct 31, 2019

View reviewed changes

Revert "Bumped version number"

72dd219

This reverts commit 142856d.

robbassi requested a review from parsonsmatt October 31, 2019 19:46

parsonsmatt merged commit 2bb1475 into yesodweb:master Oct 31, 2019

robbassi deleted the postgres-custom-contraint-names branch October 31, 2019 20:38

robbassi mentioned this pull request Nov 7, 2019

MySQL constraint= attribute test #986

Merged

3 tasks

jkeuhlen reviewed Jan 7, 2020

View reviewed changes

jkeuhlen mentioned this pull request Jan 8, 2020

persistent-postgresql getColumn throws an error when a column has more than one foreign key constraint #1009

Closed

		## 2.10.5

		* Added support for the `constraint=` attribute to the Postgresql backend. [#979](https://github.com/yesodweb/persistent/pull/979)

Postgresql constraint= attribute support #979

Postgresql constraint= attribute support #979

Conversation

robbassi commented Oct 30, 2019 • edited Loading

robbassi Oct 30, 2019

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

parsonsmatt Oct 30, 2019

Choose a reason for hiding this comment

parsonsmatt Oct 30, 2019

Choose a reason for hiding this comment

robbassi commented Oct 30, 2019

robbassi Oct 30, 2019 • edited Loading

Choose a reason for hiding this comment

robbassi Oct 31, 2019

Choose a reason for hiding this comment

parsonsmatt left a comment

Choose a reason for hiding this comment

parsonsmatt Oct 31, 2019

Choose a reason for hiding this comment

parsonsmatt Oct 31, 2019

Choose a reason for hiding this comment

parsonsmatt commented Oct 31, 2019

robbassi commented Oct 31, 2019

MaxGabriel commented Jan 3, 2020

robbassi commented Jan 6, 2020

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 7, 2020

jkeuhlen Jan 7, 2020

Choose a reason for hiding this comment

robbassi commented Jan 7, 2020 • edited Loading

jkeuhlen commented Jan 7, 2020 • edited Loading

robbassi commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 7, 2020 • edited Loading

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020 • edited Loading

robbassi commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 7, 2020

jkeuhlen commented Jan 7, 2020

robbassi commented Jan 8, 2020

jkeuhlen commented Jan 8, 2020

robbassi commented Jan 8, 2020

Postgresql `constraint=` attribute support #979

Postgresql `constraint=` attribute support #979

robbassi commented Oct 30, 2019 •

edited

Loading

robbassi Oct 30, 2019 •

edited

Loading

robbassi commented Jan 7, 2020 •

edited

Loading

jkeuhlen commented Jan 7, 2020 •

edited

Loading

robbassi commented Jan 7, 2020 •

edited

Loading

jkeuhlen commented Jan 7, 2020 •

edited

Loading