{"action":"created","issue":{"url":"https://api.github.com/repos/dexidp/dex/issues/1341","repository_url":"https://api.github.com/repos/dexidp/dex","labels_url":"https://api.github.com/repos/dexidp/dex/issues/1341/labels{/name}","comments_url":"https://api.github.com/repos/dexidp/dex/issues/1341/comments","events_url":"https://api.github.com/repos/dexidp/dex/issues/1341/events","html_url":"https://github.com/dexidp/dex/issues/1341","id":381258554,"node_id":"MDU6SXNzdWUzODEyNTg1NTQ=","number":1341,"title":"Postgres: forcing transactions to be serializable breaks concurrent login","user":{"login":"vito","id":1880,"node_id":"MDQ6VXNlcjE4ODA=","avatar_url":"https://avatars1.githubusercontent.com/u/1880?v=4","gravatar_id":"","url":"https://api.github.com/users/vito","html_url":"https://github.com/vito","followers_url":"https://api.github.com/users/vito/followers","following_url":"https://api.github.com/users/vito/following{/other_user}","gists_url":"https://api.github.com/users/vito/gists{/gist_id}","starred_url":"https://api.github.com/users/vito/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vito/subscriptions","organizations_url":"https://api.github.com/users/vito/orgs","repos_url":"https://api.github.com/users/vito/repos","events_url":"https://api.github.com/users/vito/events{/privacy}","received_events_url":"https://api.github.com/users/vito/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2018-11-15T17:11:28Z","updated_at":"2018-11-16T08:19:29Z","closed_at":null,"author_association":"CONTRIBUTOR","body":"Hi! Really glad to see Dex coming back to life lately - props to those who have volunteered!\r\n\r\nWe've got a long-lived fork with a bunch of changes we'd like to PR back: https://github.com/concourse/dex\r\n\r\nOne change in our fork was to remove the Postgres serialization level, because it was causing failures in our integration tests.\r\n\r\nPrior context: https://github.com/dexidp/dex/issues/628, https://github.com/dexidp/dex/issues/600, https://github.com/concourse/concourse/issues/2531\r\n\r\nI've noticed that when running our integration suites in parallel, if they all try to log in at the same time, a bunch of them fail with the following message:\r\n\r\n```\r\nFailed to set connector ID on auth request: update auth request: pq: could not serialize access due to read/write dependencies among transactions\r\n```\r\n\r\nHere are the logs from Postgres:\r\n\r\n```\r\n2018-11-15 16:55:47.558 UTC [797] ERROR:  could not serialize access due to read/write dependencies among transactions\r\n2018-11-15 16:55:47.558 UTC [797] DETAIL:  Reason code: Canceled on identification as a pivot, during write.\r\n2018-11-15 16:55:47.558 UTC [797] HINT:  The transaction might succeed if retried.\r\n2018-11-15 16:55:47.558 UTC [797] STATEMENT:  \r\n\t\t\t\tupdate auth_request\r\n\t\t\t\tset\r\n\t\t\t\t\tclient_id = $1, response_types = $2, scopes = $3, redirect_uri = $4,\r\n\t\t\t\t\tnonce = $5, state = $6, force_approval_prompt = $7, logged_in = $8,\r\n\t\t\t\t\tclaims_user_id = $9, claims_username = $10, claims_email = $11,\r\n\t\t\t\t\tclaims_email_verified = $12,\r\n\t\t\t\t\tclaims_groups = $13,\r\n\t\t\t\t\tconnector_id = $14, connector_data = $15,\r\n\t\t\t\t\texpiry = $16\r\n\t\t\t\twhere id = $17;\r\n```\r\n\r\nI've confirmed that each login request has its own distinct request ID (the `where id = $17`). And these tests aren't sharing any cookies or state, so they shouldn't be clobbering each others' request IDs. So I don't really see how this would legitimately cause any problems without the strict isolation level. I'm not really sure what's causing this to conflict, either, since they should be updating distinct rows.\r\n\r\nThe Postgres docs say \"applications using this level must be prepared to retry transactions due to serialization failures\", which Dex doesn't seem to be doing, and it doesn't feel right to force the client to retry API/login requests due to this, and I don't think they'd even have enough information to recognize that it's a serialization failure to retry in the first place.\r\n\r\nOne thing we tried was simply ripping out the isolation level config: https://github.com/concourse/dex/commit/9b88d6750b32c3fb79c02961e13011d9f1286170 - however there are explicit tests checking for this exact sort of failure.\r\n\r\nSo I'm not really sure what to do here. Any ideas? (Edit: submitted https://github.com/dexidp/dex/pull/1342 which adds retry logic on serialization errors.)"},"comment":{"url":"https://api.github.com/repos/dexidp/dex/issues/comments/439316255","html_url":"https://github.com/dexidp/dex/issues/1341#issuecomment-439316255","issue_url":"https://api.github.com/repos/dexidp/dex/issues/1341","id":439316255,"node_id":"MDEyOklzc3VlQ29tbWVudDQzOTMxNjI1NQ==","user":{"login":"srenatus","id":870638,"node_id":"MDQ6VXNlcjg3MDYzOA==","avatar_url":"https://avatars3.githubusercontent.com/u/870638?v=4","gravatar_id":"","url":"https://api.github.com/users/srenatus","html_url":"https://github.com/srenatus","followers_url":"https://api.github.com/users/srenatus/followers","following_url":"https://api.github.com/users/srenatus/following{/other_user}","gists_url":"https://api.github.com/users/srenatus/gists{/gist_id}","starred_url":"https://api.github.com/users/srenatus/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/srenatus/subscriptions","organizations_url":"https://api.github.com/users/srenatus/orgs","repos_url":"https://api.github.com/users/srenatus/repos","events_url":"https://api.github.com/users/srenatus/events{/privacy}","received_events_url":"https://api.github.com/users/srenatus/received_events","type":"User","site_admin":false},"created_at":"2018-11-16T08:19:29Z","updated_at":"2018-11-16T08:19:29Z","author_association":"COLLABORATOR","body":"A controlled retry doesn't seem like a bad fix. However, I agree that properly understanding what's going on here would be good. Maybe some other isolation level is what we want? Haven't dug into this yet... 🤔 "}}
{"actor":{"display_login":"srenatus"}}