New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persistent does not implement upsert #613

Closed
chreekat opened this Issue Oct 4, 2016 · 23 comments

Comments

Projects
None yet
4 participants
@chreekat
Contributor

chreekat commented Oct 4, 2016

Upsert is an either/or. Either the row exists and is updated, or it does not exist and is inserted.

Persistent, however, implements a function that inserts and then updates a row that does not exist. The tests expect that behavior, even though it is incorrect. What's more, Persistent's version of upsert replaces a row that exists, rather than just updating it![*] Nowhere have I read that to be the desired behavior.

[*] An existing row is only replaced if upsert's second argument is [], but I still think that's a bug.

In short:

Expected Actual
Row Exists updated replaced if updates == []
Row Doesn't Exist inserted inserted and then updated

Correct me if I'm wrong. I'll be sending along a patch soon in the meanwhile.


Upsert: An operation that inserts rows into a database table if they do not already exist, or updates them if they do.
https://en.wiktionary.org/wiki/upsert

\

A relational database management system uses SQL MERGE (also called upsert) statements to INSERT new records or UPDATE existing records depending on whether [the given] condition matches.
https://en.wikipedia.org/wiki/Merge_(SQL)

\

"UPSERT" is a DBMS feature that allows a DML statement's author to atomically either insert a row, or on the basis of the row already existing, UPDATE that existing row instead....
https://wiki.postgresql.org/wiki/UPSERT

\

Illustrative (questionable) test:

it "upsert with updates" $ db $ do
    deleteWhere ([] :: [Filter Upsert])
    let email = "dude@example.com"
    Nothing :: Maybe (Entity Upsert) <- getBy $ UniqueUpsert email
    let up0 = Upsert email 0
    Entity _ up1 <- upsert up0 [UpsertCounter +=. 1]
    upsertCounter up1 @== 1

https://github.com/yesodweb/persistent/blob/master/persistent-test/src/PersistentTest.hs#L509

upsertCounter up1 should be 0.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 5, 2016

Member

Hey, can you show an example of this by a small snippet in both scenarios ? Does this affect the end result now ?

Member

psibi commented Oct 5, 2016

Hey, can you show an example of this by a small snippet in both scenarios ? Does this affect the end result now ?

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

Here's a complete example. This contrivance has users and their (unique) accounts.

I want to add two units to a user's account. If the account exists, increment the balance. If it does not, initialize it with two units.

{-# LANGUAGE EmptyDataDecls             #-}
{-# LANGUAGE FlexibleContexts           #-}
{-# LANGUAGE GADTs                      #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses      #-}
{-# LANGUAGE OverloadedStrings          #-}
{-# LANGUAGE QuasiQuotes                #-}
{-# LANGUAGE TemplateHaskell            #-}
{-# LANGUAGE TypeFamilies               #-}
import           Control.Monad.IO.Class  (liftIO)
import           Database.Persist
import           Database.Persist.Sqlite
import           Database.Persist.TH

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
Usr
Account
    usr UsrId
    balance Int
    UniqueAccount usr
    deriving Show
|]

main :: IO ()
main = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr
    upsert (Account u 2) [AccountBalance +=. 2]
    liftIO . print =<<  map (account . entityVal) <$> selectList [] []
  where
    account :: Account -> Account
    account = id

On master, this prints

[Account {accountUsr = UsrKey {unUsrKey = SqlBackendKey {unSqlBackendKey = 1}}, accountBalance = 4}]

The user's balance is 4 instead of 2.

This definitely affects the end result. After my patch, the balance is 2, as expected.

Contributor

chreekat commented Oct 5, 2016

Here's a complete example. This contrivance has users and their (unique) accounts.

I want to add two units to a user's account. If the account exists, increment the balance. If it does not, initialize it with two units.

{-# LANGUAGE EmptyDataDecls             #-}
{-# LANGUAGE FlexibleContexts           #-}
{-# LANGUAGE GADTs                      #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses      #-}
{-# LANGUAGE OverloadedStrings          #-}
{-# LANGUAGE QuasiQuotes                #-}
{-# LANGUAGE TemplateHaskell            #-}
{-# LANGUAGE TypeFamilies               #-}
import           Control.Monad.IO.Class  (liftIO)
import           Database.Persist
import           Database.Persist.Sqlite
import           Database.Persist.TH

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
Usr
Account
    usr UsrId
    balance Int
    UniqueAccount usr
    deriving Show
|]

main :: IO ()
main = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr
    upsert (Account u 2) [AccountBalance +=. 2]
    liftIO . print =<<  map (account . entityVal) <$> selectList [] []
  where
    account :: Account -> Account
    account = id

On master, this prints

[Account {accountUsr = UsrKey {unUsrKey = SqlBackendKey {unSqlBackendKey = 1}}, accountBalance = 4}]

The user's balance is 4 instead of 2.

This definitely affects the end result. After my patch, the balance is 2, as expected.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 5, 2016

Member

Oh yeah, this seems wrong. It should be 2.

Member

psibi commented Oct 5, 2016

Oh yeah, this seems wrong. It should be 2.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

Good, I'm not crazy. :)

In the new tests in #614, I also demonstrate the other bug, which is that an existing row is first replaced, and then updated. Clearly it should only be updated.

Contributor

chreekat commented Oct 5, 2016

Good, I'm not crazy. :)

In the new tests in #614, I also demonstrate the other bug, which is that an existing row is first replaced, and then updated. Clearly it should only be updated.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 5, 2016

Member

@chreekat There are lots of tests there in that PR and I'm lazy. Can you point out the line number in that PR (or demo it using a small snippet ) demonstrating the other "bug" which you are talking about ?

I also checked your above script for Postgres 9.5+ and it also had the same bug. Note that postgres 9.5 + has a different code flow for the upsert query and right now it's not being tested in Travis CI.

Member

psibi commented Oct 5, 2016

@chreekat There are lots of tests there in that PR and I'm lazy. Can you point out the line number in that PR (or demo it using a small snippet ) demonstrating the other "bug" which you are talking about ?

I also checked your above script for Postgres 9.5+ and it also had the same bug. Note that postgres 9.5 + has a different code flow for the upsert query and right now it's not being tested in Travis CI.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

Combining the tests to be efficient with boilerplate:

#!/usr/bin/env stack
{- stack script
      --resolver lts-8.6
      --package persistent,persistent-sqlite,persistent-template
-}
{-# LANGUAGE EmptyDataDecls             #-}
{-# LANGUAGE FlexibleContexts           #-}
{-# LANGUAGE GADTs                      #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses      #-}
{-# LANGUAGE OverloadedStrings          #-}
{-# LANGUAGE QuasiQuotes                #-}
{-# LANGUAGE TemplateHaskell            #-}
{-# LANGUAGE TypeFamilies               #-}
import           Control.Monad.IO.Class  (liftIO)
import           Database.Persist
import           Database.Persist.Sqlite
import           Database.Persist.TH

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
Usr
Account
    usr UsrId
    balance Int
    UniqueAccount usr
    deriving Show
|]

main :: IO ()
main = insertBug >> updateBug

insertBug = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr

    upsert (Account u 2) [AccountBalance +=. 2]
    liftIO (putStr "Should be 2: ")
    liftIO . print =<<  map (accountBalance . entityVal) <$> selectList [] []

updateBug = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr

    insert (Account u 0)
    upsert (Account u 100) []
    liftIO (putStr "Should be 0: ")
    liftIO . print =<<  map (accountBalance . entityVal) <$> selectList [] []

The second bug is not as general as I first reported, but it's still a bug. I'll rewrite the issue. The second bug is specifically "If no updates are provided, an existing row is replaced." I still don't think that's what upsert is supposed to do.

Contributor

chreekat commented Oct 5, 2016

Combining the tests to be efficient with boilerplate:

#!/usr/bin/env stack
{- stack script
      --resolver lts-8.6
      --package persistent,persistent-sqlite,persistent-template
-}
{-# LANGUAGE EmptyDataDecls             #-}
{-# LANGUAGE FlexibleContexts           #-}
{-# LANGUAGE GADTs                      #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE MultiParamTypeClasses      #-}
{-# LANGUAGE OverloadedStrings          #-}
{-# LANGUAGE QuasiQuotes                #-}
{-# LANGUAGE TemplateHaskell            #-}
{-# LANGUAGE TypeFamilies               #-}
import           Control.Monad.IO.Class  (liftIO)
import           Database.Persist
import           Database.Persist.Sqlite
import           Database.Persist.TH

share [mkPersist sqlSettings, mkMigrate "migrateAll"] [persistLowerCase|
Usr
Account
    usr UsrId
    balance Int
    UniqueAccount usr
    deriving Show
|]

main :: IO ()
main = insertBug >> updateBug

insertBug = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr

    upsert (Account u 2) [AccountBalance +=. 2]
    liftIO (putStr "Should be 2: ")
    liftIO . print =<<  map (accountBalance . entityVal) <$> selectList [] []

updateBug = runSqlite ":memory:" $ do
    runMigration migrateAll

    u <- insert Usr

    insert (Account u 0)
    upsert (Account u 100) []
    liftIO (putStr "Should be 0: ")
    liftIO . print =<<  map (accountBalance . entityVal) <$> selectList [] []

The second bug is not as general as I first reported, but it's still a bug. I'll rewrite the issue. The second bug is specifically "If no updates are provided, an existing row is replaced." I still don't think that's what upsert is supposed to do.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

@psibi oh, the new backend-specific upsert is also implemented incorrectly for Postgres? I'll inspect that, but I can't test it locally atm.

Contributor

chreekat commented Oct 5, 2016

@psibi oh, the new backend-specific upsert is also implemented incorrectly for Postgres? I'll inspect that, but I can't test it locally atm.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

Yep, https://github.com/yesodweb/persistent/blob/master/persistent/Database/Persist/Sql/Orphan/PersistUnique.hs#L34

That implements the second bug by using defaultUpsert when updates == []. I don't think the new connUpsertSql has the right shape for a quick fix, though. Postgres will need ON CONFLICT (...) DO UPDATE SET (...) when updates /= [], and ON CONFLICT (...) DO NOTHING when updates == [].

That's at https://github.com/yesodweb/persistent/blob/master/persistent-postgresql/Database/Persist/Postgresql.hs#L246

I can't fix this now, since I can't even use latest persistent because of esqueleto's foot-dragging. :( (I discovered this late yesterday and had to patch lts-6.17's version of persistent for my use...)

Contributor

chreekat commented Oct 5, 2016

Yep, https://github.com/yesodweb/persistent/blob/master/persistent/Database/Persist/Sql/Orphan/PersistUnique.hs#L34

That implements the second bug by using defaultUpsert when updates == []. I don't think the new connUpsertSql has the right shape for a quick fix, though. Postgres will need ON CONFLICT (...) DO UPDATE SET (...) when updates /= [], and ON CONFLICT (...) DO NOTHING when updates == [].

That's at https://github.com/yesodweb/persistent/blob/master/persistent-postgresql/Database/Persist/Postgresql.hs#L246

I can't fix this now, since I can't even use latest persistent because of esqueleto's foot-dragging. :( (I discovered this late yesterday and had to patch lts-6.17's version of persistent for my use...)

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 5, 2016

Member

@chreekat Don't worry, I will fix that postgres bug as soon as I get some time. Regarding your second "update bug", I don't understand why it is a bug. Why do you think it should be inserted and not new value ? My understand is that if there is already a row present for that unique constraint, we just replace all other values for that unique constraint. Note that the upsert function itself will throw an exception if there are more than one unique constraint for a table.

Member

psibi commented Oct 5, 2016

@chreekat Don't worry, I will fix that postgres bug as soon as I get some time. Regarding your second "update bug", I don't understand why it is a bug. Why do you think it should be inserted and not new value ? My understand is that if there is already a row present for that unique constraint, we just replace all other values for that unique constraint. Note that the upsert function itself will throw an exception if there are more than one unique constraint for a table.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 5, 2016

Contributor

I interpret upsert newVal [] to mean, "Insert newVal if it violates no constraints. Otherwise, do nothing."

That would be equivalent to Postgres' insert into ... on conflict (...) do nothing.

It would also be equivalent to MySQL's insert ignore ....

The behavior you are talking about is equivalent to MySQL's replace. I think it's a useful function, but it's not upsert.

+ edited to mention the correct MySQL equivalent

Contributor

chreekat commented Oct 5, 2016

I interpret upsert newVal [] to mean, "Insert newVal if it violates no constraints. Otherwise, do nothing."

That would be equivalent to Postgres' insert into ... on conflict (...) do nothing.

It would also be equivalent to MySQL's insert ignore ....

The behavior you are talking about is equivalent to MySQL's replace. I think it's a useful function, but it's not upsert.

+ edited to mention the correct MySQL equivalent

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 6, 2016

Contributor

Another way to look at it: if I really want to replace an existing entry with upsert, I would write

upsert newVal
       [Field1 =. field1 newVal, Field2 =. field2 newVal, ..., FieldN =. fieldN newVal]

Making upsert newVal [] a special case that does the same thing is surprising to me. And, how then should I do the equivalent of insert ... on conflict do nothing?

Contributor

chreekat commented Oct 6, 2016

Another way to look at it: if I really want to replace an existing entry with upsert, I would write

upsert newVal
       [Field1 =. field1 newVal, Field2 =. field2 newVal, ..., FieldN =. fieldN newVal]

Making upsert newVal [] a special case that does the same thing is surprising to me. And, how then should I do the equivalent of insert ... on conflict do nothing?

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 6, 2016

Member

Okay, I see your point. IMO, this needs a good documentation with examples on haddocks.

Member

psibi commented Oct 6, 2016

Okay, I see your point. IMO, this needs a good documentation with examples on haddocks.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 6, 2016

Member

@chreekat By the way, the postgres 9.5+ server doesn't have the "insert bug". Yesterday, I was running an old version of the persistent library. But, I did encounter a new bug - while testing it (which is completely unrelated to this). I will send a PR for that soon.

Member

psibi commented Oct 6, 2016

@chreekat By the way, the postgres 9.5+ server doesn't have the "insert bug". Yesterday, I was running an old version of the persistent library. But, I did encounter a new bug - while testing it (which is completely unrelated to this). I will send a PR for that soon.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 6, 2016

Contributor

Agreed about haddocks. A version bump too, since it's changing behavior. I'm busy today, and maybe all next week, but I'll get to it if I can.

Contributor

chreekat commented Oct 6, 2016

Agreed about haddocks. A version bump too, since it's changing behavior. I'm busy today, and maybe all next week, but I'll get to it if I can.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 7, 2016

Member

I'm working on a related code in Persistent and will try to fix this by tonight.

Member

psibi commented Oct 7, 2016

I'm working on a related code in Persistent and will try to fix this by tonight.

psibi added a commit that referenced this issue Oct 7, 2016

Add proper upsert behavior
CC: @chreekat

This implements the upsert behaviour as described in
#613

It specifically fixes the bug outlined here:
#613 (comment)
@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Oct 7, 2016

Member

@chreekat This commit 64a9c9a specifically fixes both the bug described by you in the issue. Would love to hear your feedback. I will add Haddock docs soon with examples for upsert.

Member

psibi commented Oct 7, 2016

@chreekat This commit 64a9c9a specifically fixes both the bug described by you in the issue. Would love to hear your feedback. I will add Haddock docs soon with examples for upsert.

@chreekat

This comment has been minimized.

Show comment
Hide comment
@chreekat

chreekat Oct 7, 2016

Contributor

Well, #614 fixes this bug as well as the tests. Did I miss something?

Contributor

chreekat commented Oct 7, 2016

Well, #614 fixes this bug as well as the tests. Did I miss something?

@mitchellwrosen

This comment has been minimized.

Show comment
Hide comment
@mitchellwrosen

mitchellwrosen Mar 29, 2017

Contributor

Output of stack script --resolver lts-8.6 bug.hs (persistent-2.6.1) on @chreekat's first example given above:

[Account {accountUsr = UsrKey {unUsrKey = SqlBackendKey {unSqlBackendKey = 1}}, accountBalance = 4}]

Looks like the bug is still there, since accountBalance should be 2.

Contributor

mitchellwrosen commented Mar 29, 2017

Output of stack script --resolver lts-8.6 bug.hs (persistent-2.6.1) on @chreekat's first example given above:

[Account {accountUsr = UsrKey {unUsrKey = SqlBackendKey {unSqlBackendKey = 1}}, accountBalance = 4}]

Looks like the bug is still there, since accountBalance should be 2.

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Mar 30, 2017

Member

@mitchellwrosen The combined PR of mine and @chreekat 's is available here which fixes the problem: #618

Greg wanted some changes for MongoDB in that pull request to which I haven't yet gotten back to. Also, I there there are some conflicts in that file (which should be easily resolved) right now. I will see if I can give some time to the PR this weekend. If you want to take up the task of fixing the PR, feel free to do so and let me know.

Member

psibi commented Mar 30, 2017

@mitchellwrosen The combined PR of mine and @chreekat 's is available here which fixes the problem: #618

Greg wanted some changes for MongoDB in that pull request to which I haven't yet gotten back to. Also, I there there are some conflicts in that file (which should be easily resolved) right now. I will see if I can give some time to the PR this weekend. If you want to take up the task of fixing the PR, feel free to do so and let me know.

@mitchellwrosen

This comment has been minimized.

Show comment
Hide comment
@mitchellwrosen

mitchellwrosen Mar 30, 2017

Contributor

@psibi I hope I don't have to fix up this PR, but I do need this fix in. Would be great if you or someone else with better knowledge of the code could close this one out. Thanks!

Contributor

mitchellwrosen commented Mar 30, 2017

@psibi I hope I don't have to fix up this PR, but I do need this fix in. Would be great if you or someone else with better knowledge of the code could close this one out. Thanks!

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Apr 8, 2017

Member

@mitchellwrosen I have updated the PR. Can you see if it works for you ?

Member

psibi commented Apr 8, 2017

@mitchellwrosen I have updated the PR. Can you see if it works for you ?

@mitchellwrosen

This comment has been minimized.

Show comment
Hide comment
@mitchellwrosen

mitchellwrosen Apr 8, 2017

Contributor

Both bugs originally reported by @chreekat were fixed in the latest #618. Thank you!

Contributor

mitchellwrosen commented Apr 8, 2017

Both bugs originally reported by @chreekat were fixed in the latest #618. Thank you!

psibi added a commit to psibi/persistent that referenced this issue Apr 10, 2017

@psibi

This comment has been minimized.

Show comment
Hide comment
@psibi

psibi Apr 10, 2017

Member

persistent-2.7.0 has been released which fixes this. Closing the issue.

Member

psibi commented Apr 10, 2017

persistent-2.7.0 has been released which fixes this. Closing the issue.

@psibi psibi closed this Apr 10, 2017

@snoyberg snoyberg removed the in progress label Apr 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment