MySQL on duplicate key update #674

parsonsmatt · 2017-05-31T21:57:35Z

MySQL has a native upsert in INSERT ... ON DUPLICATE KEY UPDATE .... This PR introduces that functionality to persistent-mysql.

It also introduces a bulkInsertOnDuplicateKeyUpdate function, which has an API I'd like feedback on.

…-duplicate-key-update

parsonsmatt · 2017-05-31T22:02:36Z

persistent-mysql/Database/Persist/MySQL.hs

+  => [record] -- ^ A list of the records you want to insert, or update
+  -> [SomeField record] -- ^ A list of the fields you want to copy over.
+  -> [Update record] -- ^ A list of the updates to apply that aren't dependent on the record being inserted.
+  -> SqlPersistT m ()


This function would be used like:

bulkInsertOnDuplicateKeyUpdate [ {- a big ol' list of records you want to insert/update -} ] [ SomeField UserName, SomeField UserEmail ] -- this copies the values that are being inserted to existing records [ UserModified =. now, UserEncounted +=. 1 ] -- this update is performed for any field that matches the inserted records

gregwebs · 2017-06-01T05:07:48Z

Travis CI is pointint to some import issues on older versions

parsonsmatt · 2017-06-02T15:30:06Z

@gregwebs Alright, CI is building now 😄 Any comments on the API or implementation?

psibi · 2017-06-02T16:25:45Z

Minor comment: I would prefer the name inserManyOnDuplicateKeyUpdate instead of the current one as that is roughly equivalent to the current naming convention if I'm not wrong.

parsonsmatt · 2017-06-02T17:34:24Z

👍 Good call @psibi. I've implemented that change.

MaxGabriel · 2017-06-02T18:56:00Z

persistent-mysql/Database/Persist/MySQL.hs

+--
+-- The third parameter is a list of updates to perform that are independent of
+-- the value that is provided. You can use this to increment a counter value.
+-- These updates only occur if the original record is present in the database.


The documentation for the third parameter is kind of confusing to me

It's a little difficult to explain. If you pass something like

insertManyOnDuplicateKeyUpdate _ [SomeField UserName] [UserAge +=. 1]

then it generates the following clause in the Update:

INSERT INTO ... VALUES ... ON DUPLICATE KEY UPDATE ... `user`.`name` = VALUES(`user`.`name`) `user`.`age` = `user`.`age` + 1

So the name gets copied from whatever record was being inserted and had a key collision, and the age gets incremented by 1.

Since the Update field requires a specific value, that means that we can't make it dependent on the record that we're updating. MySQL would support something like

ON DUPLICATE KEY UPDATE user.age = VALUES(user.age) + 2

which would add 2 to the record that hit the duplicate key. There's not a good way to make the update have more complicated values, unfortunately, so this feature would be difficult to incorporate as-is.

MaxGabriel · 2017-06-02T18:58:33Z

persistent-mysql/Database/Persist/MySQL.hs

+    Multiply -> T.concat [n, "=", n, "*?"]
+    Divide -> T.concat [n, "=", n, "/?"]
+    BackendSpecificUpdate up ->
+      error . T.unpack $ "BackendSpecificUpdate" <> up <> "not supported"


Should there be a space after BackendSpecificUpdate and before not supported?

Probably! It's vendored from part of the update function in persistent. I could push it further upstream and refactor that code to use it as well.

MaxGabriel · 2017-06-02T18:59:45Z

persistent-mysql/Database/Persist/MySQL.hs

+commaSeparated :: [Text] -> Text
+commaSeparated = T.intercalate ", "
+
+parenWrapped :: Text -> Text


These functions make the code much more readable, nice idea

Thanks! I can use them in the other instances of these if you'd like.

MaxGabriel · 2017-06-02T19:02:00Z

persistent-mysql/Database/Persist/MySQL.hs

+commaSeparated = T.intercalate ", "
+
+parenWrapped :: Text -> Text
+parenWrapped = ("(" <>) . (<> ")")


Not that it's a huge deal since this function is so small, but is point free really better here? It's longer than just parenWrapped t = "(" <> t <> ")"

Whenever I get the chance, I try to make smiley faces in code. Tbh T.concat ["(", t, ")"] is the tiniest bit cleaner and probably more efficient.

MaxGabriel · 2017-06-02T19:08:21Z

persistent-mysql/Database/Persist/MySQL.hs

@@ -720,6 +724,7 @@ showColumn (Column n nu t def _defConstraintName maxLen ref) = concat
        Just s -> -- Avoid DEFAULT NULL, since it is always unnecessary, and is an error for text/blob fields
                  if T.toUpper s == "NULL" then ""
                  else " DEFAULT " ++ T.unpack s
+                  {-# LANGUAGE GADTs #-}


Is this intentionally here?

oh, no, that is definitely a mistake

MaxGabriel · 2017-06-02T20:53:41Z

LGTM

gregwebs · 2017-06-04T14:09:00Z

awesome! Can this be used to fill out upsert in the typeclass?

parsonsmatt · 2017-06-04T20:10:28Z

Unfortunately not, as this doesn't return the updated records. We'd need something like upsert_ :: record -. [Update record] -> SqlPersistT m () for this to make it into the class.

gregwebs · 2017-06-04T22:00:42Z

That could make sense to add upsert_ then. Could you also add a SELECT in the same transaction to get the upsert behavior though?

parsonsmatt · 2017-06-05T02:45:22Z

Ooh, I think that would work. I'm not sure it'd be any more efficient than the current default implementation which does a SELECT and then an UPDATE if present and INSERT if missing -- still two queries.

psibi · 2017-06-05T02:52:27Z

@parsonsmatt I just looked on the code. Is there any reason why native upsert for mysql has been done like this instead of going via the typeclass route: https://www.stackage.org/haddock/lts-8.17/persistent-2.6.1/Database-Persist-Sql.html#t:SqlBackend

The function connUpsertSql there has been specifically created for implementing native upsert feature for backends. In fact, Postgres right now uses that. Sorry for commenting about this so late.

parsonsmatt · 2017-06-05T04:45:54Z

@psibi The type class function upsert returns the updated or inserted record. MySQL's ON DUPLICATE KEY UPDATE doesn't have any way of returning a value, so we can't do that. If the type class had upsert_ :: rec -> [Update rec] -> SqlPersistT m (), then this would fit that signature.

gregwebs · 2017-06-05T21:04:54Z

@psibi I am in favor of adding upsert_.

psibi · 2017-06-07T11:26:01Z

Yeah, upsert_ would be good. It can be probably added as a typeclass method and a custom function be passed as a part of SqlBackend type which will generate the appropriate query for the specific backend. For those backends, for which the function hasn't been passed, upsert_ can simply be defined as upsert >> return ().

gregwebs · 2017-06-18T22:50:54Z

@psibi can you merge and release this as is?

psibi · 2017-06-19T04:42:32Z

persistent-mysql-2.6.1 has been released with the relevant CHANGELOG in Hackage.

parsonsmatt added 5 commits May 31, 2017 15:42

upsert

05293e2

Merge branch 'master' of github.com:yesodweb/persistent into mysql-on…

19b0a6d

…-duplicate-key-update

Expose, clean up bits

a1c379b

consistent

b1d5bb5

cleaner constraints

6bed164

parsonsmatt commented May 31, 2017

View reviewed changes

parsonsmatt changed the title ~~[WIP] MySQL on duplicate key update~~ MySQL on duplicate key update May 31, 2017

parsonsmatt added 2 commits June 1, 2017 09:52

no mo mconcat

3a1e545

it builds

696ab24

parsonsmatt changed the title ~~MySQL on duplicate key update~~ [WIP] MySQL on duplicate key update Jun 1, 2017

parsonsmatt changed the title ~~[WIP] MySQL on duplicate key update~~ MySQL on duplicate key update Jun 1, 2017

fix syntax error

48e27e0

name change

f12f3a0

MaxGabriel reviewed Jun 2, 2017

View reviewed changes

Address comments

9cea669

psibi merged commit 3d7ff00 into yesodweb:master Jun 19, 2017

naushadh added a commit to naushadh/persistent that referenced this pull request Aug 7, 2017

Port yesodweb#674 from mysql-haskell.

4b63ba9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MySQL on duplicate key update #674

MySQL on duplicate key update #674

parsonsmatt commented May 31, 2017

parsonsmatt May 31, 2017

gregwebs commented Jun 1, 2017

parsonsmatt commented Jun 2, 2017

psibi commented Jun 2, 2017

parsonsmatt commented Jun 2, 2017

MaxGabriel Jun 2, 2017

parsonsmatt Jun 2, 2017

MaxGabriel Jun 2, 2017

parsonsmatt Jun 2, 2017

MaxGabriel Jun 2, 2017

parsonsmatt Jun 2, 2017

MaxGabriel Jun 2, 2017

parsonsmatt Jun 2, 2017

MaxGabriel Jun 2, 2017

parsonsmatt Jun 2, 2017

MaxGabriel commented Jun 2, 2017

gregwebs commented Jun 4, 2017

parsonsmatt commented Jun 4, 2017

gregwebs commented Jun 4, 2017

parsonsmatt commented Jun 5, 2017

psibi commented Jun 5, 2017

parsonsmatt commented Jun 5, 2017

gregwebs commented Jun 5, 2017

psibi commented Jun 7, 2017

gregwebs commented Jun 18, 2017

psibi commented Jun 19, 2017

MySQL on duplicate key update #674

MySQL on duplicate key update #674

Conversation

parsonsmatt commented May 31, 2017

Choose a reason for hiding this comment

gregwebs commented Jun 1, 2017

parsonsmatt commented Jun 2, 2017

psibi commented Jun 2, 2017

parsonsmatt commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaxGabriel commented Jun 2, 2017

gregwebs commented Jun 4, 2017

parsonsmatt commented Jun 4, 2017

gregwebs commented Jun 4, 2017

parsonsmatt commented Jun 5, 2017

psibi commented Jun 5, 2017

parsonsmatt commented Jun 5, 2017

gregwebs commented Jun 5, 2017

psibi commented Jun 7, 2017

gregwebs commented Jun 18, 2017

psibi commented Jun 19, 2017