Skip to content
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

Support upsert with empty updates #301

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

parsonsmatt
Copy link
Collaborator

@parsonsmatt parsonsmatt commented Mar 5, 2022

Fixes #300

Before submitting your PR, check that you've:

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).

@parsonsmatt
Copy link
Collaborator Author

Ah, dang. upsert is gonna need a new type signature.

In the event that we do an INSERT ... ON CONFLICT DO NOTHING RETURNING ??, postgresql does not return anything if we don't insert anything.

So the most proper type of upsert is then upsert :: (PersistEntity ent, OnlyOneUniqueKey ent) => ent -> [Update ent] -> SqlPersistT m (Maybe (Entity ent))

At least, as a default.

Playing around locally, the following form works:

WITH inserted AS (
    INSERT INTO "OneUnique" (name, value) 
    VALUES ("asdf", 0)
    ON CONFLICT (value)
    DO NOTHING
    RETURNING id, name, value
)
SELECT id, name, value
FROM inserted
UNION
SELECT id, name, value
FROM "OneUnique"
WHERE value = 0

But I have no idea what sorts of performance this has. This StackOverflow question/answer has some details and it looks to be somewhat complicated.

Maybe the type of upsert should really be upsert :: entity -> NonEmpty (Update entity) -> SqlPersistT m (Entity entity). Then we have our no-op insertOnConflictDoNothing :: entity -> SqlPersistT m (Maybe (Entity entity)). Then you get:

coolUpsert entity updates = 
    case NonEmpty.nonEmpty updates of
        Just nonEmptyUpdates ->
            Just <$> upsert entity nonEmptyUpdates
        Nothing ->
            insertOnConflictDoNothing entity

It might be tempting to write:

insertOnConflictDoNothing :: record -> SqlPersistT m record 
insertOnCOnflictDoNothing rec = do
    xs <- rawSql thequery thevalues
    case xs of
        [] -> pure rec
        (a : _) -> pure a

But this only means that the uniqueness key is the same, not the rest of the values.

Copy link
Collaborator Author

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dang, this is more complicated than I thought.

@@ -51,6 +51,34 @@ import Common.Test
import Common.Test.Import hiding (from, on)
import PostgreSQL.MigrateJSON

spec :: Spec
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is just moved up to the top

@@ -1038,18 +1066,20 @@ testInsertUniqueViolation =
sqlErrorHint = ""}

testUpsert :: SpecDb
testUpsert =
describe "Upsert test" $ do
testUpsert = describe "Upsert test" $ do
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed formatting to be 4 space indent

Comment on lines 1080 to 1082
itDb "Works with no updates" $ do
_ <- EP.upsert u1 []
pure ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New test case here

Comment on lines +1081 to +1083
itDb "Works with no updates" $ do
_ <- EP.upsert u1 []
pure ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this passes. Hooray.

Comment on lines +1084 to +1087
itDb "Works with no updates, twice" $ do
Entity u1Key u1' <- EP.upsert u1 []
Entity u1Key_ u1'' <- EP.upsert u1 { oneUniqueName = "Something Else" } []
pure ()
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this fails with ErrorCall "head: empty list".

The reason is that:

INSERT INTO "OneUnique" (name, value)
VALUES (?, ?)
ON CONFLICT DO NOTHING
RETURNING id, name, value

does not return anything if we don't perform an INSERT - so the return is [] which he head on and it goes boom.

DO UPDATE always does something. But we really don't want to do something silly like DO UPDATE SET blah = EXCLUDED.blah because this does unnecessary writes and triggers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect SQL query generated for PostgreSQL.upsert
1 participant