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

Concurrent appends to same stream and ExpectedVersion.Any on Postgres throws WrongExpectedVersion #478

Open
stoft opened this issue Nov 18, 2020 · 3 comments

Comments

@stoft
Copy link

stoft commented Nov 18, 2020

We tried to concurrently append to the same stream with ExpectedVersion.Any (commutative events), using Postgres as underlying DB, and it fails throwing WrongExpectedVersion. We've tried both using a single connection and with dedicated connections but with the same result.

For comparison I tried doing the same with MySQL which deadlocked on a single connection but passed on dedicated connections. I may give SQL Server a whirl tomorrow. The In Memory Store works fine.

SSS version: 1.2.0-beta.8 (latest published version of SSS.Postgres on nuget)
Postgres: 9.6 (local docker) & 12.3 (AWS RDS)

The example test case below uses dedicated connections (F# with Expecto, if needed I could probably convert it to C# within a few days):

ftest "repro concurrency error" {
    let streamStore () =
        let settings =
            SqlStreamStore.PostgresStreamStoreSettings
                ("Host=localhost;Port=5432;User Id=user;Password=password;Database=db")

        settings.Schema <- "teststreamstore"

        new SqlStreamStore.PostgresStreamStore(settings)

    let appendMsg msg =
        try
            (streamStore ())
                .AppendToStream(SqlStreamStore.Streams.StreamId
                                    ("test"),
                                SqlStreamStore.Streams.ExpectedVersion.Any,
                                [| msg |])
            |> Async.AwaitTask
            with exn ->
                failtest ("should not fail: " + exn.Message)

    let message () =
        SqlStreamStore.Streams.NewStreamMessage
            (System.Guid.NewGuid (), "some-type", "{}", "{}")

    List.map (fun _ -> appendMsg (message ())) [ 0 .. 10 ]
    |> List.map Async.RunSynchronously
};
Error Message:
   Append failed due to WrongExpectedVersion.Stream: test, Expected version: -2
  Stack Trace:
     at SqlStreamStore.PostgresStreamStore.AppendToStreamInternal(String streamId, Int32 expectedVersion, NewStreamMessage[] messages, CancellationToken cancellationToken)
@yreynhout
Copy link
Member

yreynhout commented Nov 19, 2020

One of the first things I spot here is that you're not using 1 instance of SqlStreamStore (which is preferred in the same process space). If the idea is to test with separate instances (is this what you mean by dedicated connections?) then at least make sure you dispose each instance when you're done. That said, it should not make a difference for the behaviour you observe.

Looking at https://github.com/SQLStreamStore/SQLStreamStore/blob/master/src/SqlStreamStore.Postgres/PostgresStreamStore.Append.cs#L13 it's obvious that the postgres implementation does not special case ExpectedVersion.Any thus being more likely to run into the retry limit IF the underlying sql causes it. For that we'd have to dig into https://github.com/SQLStreamStore/SQLStreamStore/blob/master/src/SqlStreamStore.Postgres/PgSqlScripts/AppendToStream.sql and deeper (which does cater for ExpectedVersion.Any).

The simplest thing to do is add a test to the test suite which mimics your test above and see if we run into the same trouble. If we do, we have a reproducible scenario to work with.

@stoft
Copy link
Author

stoft commented Nov 20, 2020

@yreynhout yes, that's what I meant by dedicated connections. We've tried both setups with the same result.

I'll try to cobble together a test in C# next week. 🙂

roetlich added a commit to insurello/SQLStreamStore that referenced this issue Nov 25, 2020
Add a test to AcceptanceTests.AppendStream that fails for postgres but passes for all other DBs. This is to reproduce a bug with concurrent appends in postgres, see: SQLStreamStore#478

Co-authored-by: Rasmus Larsson <stoft@users.noreply.github.com>
roetlich added a commit to insurello/SQLStreamStore that referenced this issue Nov 26, 2020
Add a test to AcceptanceTests.AppendStream that fails for postgres but passes for all other DBs. This is to reproduce a bug with concurrent appends in postgres, see: SQLStreamStore#478

Co-authored-by: Rasmus Larsson <stoft@users.noreply.github.com>
@roetlich
Copy link
Contributor

We did add that C# example by the way: #480
The CI passed on that one, but the test failed for postgres.

roetlich added a commit to insurello/SQLStreamStore that referenced this issue Jul 21, 2021
Add a test to AcceptanceTests.AppendStream that fails for postgres but passes for all other DBs. This is to reproduce a bug with concurrent appends in postgres, see: SQLStreamStore#478

Co-authored-by: Rasmus Larsson <stoft@users.noreply.github.com>
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

No branches or pull requests

3 participants