{"action":"created","comment":{"url":"https://api.github.com/repos/zalando/patroni/pulls/comments/186658715","pull_request_review_id":118277950,"id":186658715,"diff_hunk":"@@ -1645,52 +1717,77 @@ def basebackup(self, conn_url, env):\n \n         return ret\n \n-    def pick_synchronous_standby(self, cluster):\n-        \"\"\"Finds the best candidate to be the synchronous standby.\n-\n-        Current synchronous standby is always preferred, unless it has disconnected or does not want to be a\n-        synchronous standby any longer.\n-\n-        :returns tuple of candidate name or None, and bool showing if the member is the active synchronous standby.\n+    def current_sync_state(self, cluster):\n+        \"\"\"Returns current synchronous replication state as a dict:\n+        * numsync: number of synchronous nodes requested(including leader)\n+        * sync: set of nodes potentially being synced to\n+        * active: set of nodes that are sync capable\n         \"\"\"\n-        current = cluster.sync.sync_standby\n-        current = current.lower() if current else current\n+        sync_standby_names = self.query(\"SHOW synchronous_standby_names\")\n+        result = parse_sync_standby_names(sync_standby_names)\n+\n+        active = []\n         members = {m.name.lower(): m for m in cluster.members}\n-        candidates = []\n-        # Pick candidates based on who has flushed WAL farthest.\n-        # TODO: for synchronous_commit = remote_write we actually want to order on write_location\n+        current = None\n         for app_name, state, sync_state in self.query(\n-                \"\"\"SELECT LOWER(application_name), state, sync_state\n+                \"\"\"SELECT LOWER(application_name), state, sync_state \n                      FROM pg_stat_replication\n-                    ORDER BY flush_{0} DESC\"\"\".format(self.lsn_name)):\n+                     ORDER BY flush_{0} DESC\"\"\".format(self.lsn_name)):\n             member = members.get(app_name)\n             if state != 'streaming' or not member or member.tags.get('nosync', False):\n                 continue\n-            if sync_state == 'sync':\n-                return app_name, True\n-            if sync_state == 'potential' and app_name == current:\n-                # Prefer current even if not the best one any more to avoid indecisivness and spurious swaps.\n-                return current, False\n-            if sync_state == 'async':\n-                candidates.append(app_name)\n-\n-        if candidates:\n-            return candidates[0], False\n-        return None, False\n-\n-    def set_synchronous_standby(self, name):\n-        \"\"\"Sets a node to be synchronous standby and if changed does a reload for PostgreSQL.\"\"\"\n-        if name and name != '*':\n-            name = quote_ident(name)\n-        if name != self._synchronous_standby_names:\n-            if name is None:\n+            if state == 'sync' or (state == 'potential' and current is None):\n+                current = member.name\n+            active.append(member.name)\n+\n+        result['active'] = set(active)\n+        result['numsync'] = result['num'] + 1 if not result.get('has_star') else 1\n+        result['sync'] = set([self.name] + result['members']).difference(['*'])\n+\n+        return result\n+\n+    def set_synchronous_state(self, num, sync=None):\n+        \"\"\"Updates synchronization state.\n+\n+        Parameters:\n+        `num` specifies number of nodes in sync (including leader).\n+        `sync` - set of nodes to sync to, including leader.\n+\n+        Number of nodes in sync can be bigger than the set to sync to. Commiters will then have to wait for a new\n+        replica to become sync. As a special case with num >= 2 and no standbys, synchronous_standby_names will get\n+        set to `*`. Such a state will only be enabled with a 1/1 quorum and minimum_replication_factor >= 2. The\n+        state parses as 1/1 sync and if any node appears the sync state will first get changed. At worst any unknown\n+        node being synced to will be unknown to Patroni and we will not be able to promote it on leader failure. This\n+        can happen anyway in the case if one of the nodes is configured with nofailover, but without nosync.\n+\n+        TODO: Should we have a special synchronization mode that requires that all nodes that accept synchronization\n+        must also be eligible for failover?\n+        \"\"\"\n+\n+        if num == 1 or num is None:\n+            # Turn off sync replication\n+            assert not sync or sync == set([self.name])\n+            ssn = None\n+        else:\n+            assert self.name in sync\n+            sync_standbys = sync.difference([self.name])\n+            standby_list = \", \".join(sorted(sync_standbys)) if sync_standbys else \"*\"","path":"patroni/postgresql.py","position":176,"original_position":176,"commit_id":"0ae0cb0602a82b85a6f51b45d64f2097f6d11c72","original_commit_id":"0ae0cb0602a82b85a6f51b45d64f2097f6d11c72","user":{"login":"ants","id":64938,"avatar_url":"https://avatars2.githubusercontent.com/u/64938?v=4","gravatar_id":"","url":"https://api.github.com/users/ants","html_url":"https://github.com/ants","followers_url":"https://api.github.com/users/ants/followers","following_url":"https://api.github.com/users/ants/following{/other_user}","gists_url":"https://api.github.com/users/ants/gists{/gist_id}","starred_url":"https://api.github.com/users/ants/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ants/subscriptions","organizations_url":"https://api.github.com/users/ants/orgs","repos_url":"https://api.github.com/users/ants/repos","events_url":"https://api.github.com/users/ants/events{/privacy}","received_events_url":"https://api.github.com/users/ants/received_events","type":"User","site_admin":false},"body":"Thanks for catching this. I will add it back in.","created_at":"2018-05-08T08:53:54Z","updated_at":"2018-05-08T08:53:54Z","html_url":"https://github.com/zalando/patroni/pull/672#discussion_r186658715","pull_request_url":"https://api.github.com/repos/zalando/patroni/pulls/672","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/zalando/patroni/pulls/comments/186658715"},"html":{"href":"https://github.com/zalando/patroni/pull/672#discussion_r186658715"},"pull_request":{"href":"https://api.github.com/repos/zalando/patroni/pulls/672"}},"in_reply_to_id":186061285},"pull_request":{"url":"https://api.github.com/repos/zalando/patroni/pulls/672","id":185165486,"html_url":"https://github.com/zalando/patroni/pull/672","diff_url":"https://github.com/zalando/patroni/pull/672.diff","patch_url":"https://github.com/zalando/patroni/pull/672.patch","issue_url":"https://api.github.com/repos/zalando/patroni/issues/672","number":672,"state":"open","locked":false,"title":"Quorum commit feature","user":{"login":"ants","id":64938,"avatar_url":"https://avatars2.githubusercontent.com/u/64938?v=4","gravatar_id":"","url":"https://api.github.com/users/ants","html_url":"https://github.com/ants","followers_url":"https://api.github.com/users/ants/followers","following_url":"https://api.github.com/users/ants/following{/other_user}","gists_url":"https://api.github.com/users/ants/gists{/gist_id}","starred_url":"https://api.github.com/users/ants/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ants/subscriptions","organizations_url":"https://api.github.com/users/ants/orgs","repos_url":"https://api.github.com/users/ants/repos","events_url":"https://api.github.com/users/ants/events{/privacy}","received_events_url":"https://api.github.com/users/ants/received_events","type":"User","site_admin":false},"body":"I did some work on the subject, did not have enough time to get it finished, but far enough to solicit some feedback.\r\n\r\n## Description\r\n\r\nThe feature generalizes synchronous_mode to multiple nodes being in sync. Trade-off between synchronization overhead and fault tolerance is user selectable via `replication_factor` configuration parameter, specifying how many nodes must contain a commit before it is acknowledged.\r\n\r\nCluster will retain self-healing capability if within one HA loop interval up to `replication_factor - 1` nodes fail.\r\n\r\nThe old `synchronous_commit` corresponds to `replication_factor = 2`.\r\n\r\nOn PostgreSQL 10 the quorum commit `ANY k (n)` feature is used. On 9.6 the `k (n)` variant is used picking first nodes. On even older versions maximum `replication_factor` is 2. However, this is still an improvement from status quo as it delegates sync standby selection to PostgreSQL reducing commit latency on sync standby failure.\r\n\r\nDCS sync state is changed from `{\"leader\": .., \"sync_standby\": ..}` to `{\"leader\": .., \"quorum\": .., \"members\": [..]}`, where leader is kept mainly as an informative field.\r\n\r\n## State\r\n\r\nThe main quorum management engine should be in a reasonable state although I haven't tested it yet.\r\n\r\nOpen issues:\r\n\r\n* Is `replication_factor` a reasonable UI for this? Do we want to keep `synchronous_commit` as the main switch for this functionality.\r\n* Nodes that aren't actually taking part in synchronous replication can still promote themselves if they can verify their wal position with enough nodes that are taking part. Does anyone see a problem with this? It causes minor extra complexity when promoting such node, but on the other hand simplifies health checks.\r\n* I could not figure out a way how to find out from PostgreSQL when `synchronous_standby_names` becomes active. Currently there is a small race condition there. I'm not even sure if the previous check of pg_stat_replication showing walsender as sync was enough as a cursory review of postgres code hints that a walsender might think it is sync while a backend hasn't gotten the message that sync replication was enabled. More study of the code is needed.\r\n* Quorum engine manages transitioning DCS and sync state between \"safe\" states. If somebody manually modifies state into something we don't think of as valid, there currently isn't any code to automatically recover. Should be simple enough, I just didn't get to it.\r\n* State handling when promoting could be improved. I think we want to keep synchronous replication running with mostly the same settings as before failover, but remove any nodes that we know are down based on DCS information. Speeds up being able to accept commits.\r\n* Testsuite is obviously broken. Needs fixing up of old tests and new tests to exercise corner cases of the new stuff.\r\n* patronictl UI needs updating. How do we want to display the information?\r\n* There is an impedance mismatch between quorum engine and `synchronous_standby_names` in that quorum engine thinks of leader as just one additional synchronous replica. Currently the boundary transition between two worlds is in postgresql.py. Maybe some other place would work better.\r\n* flake8 is probably going to have a fit on this code. A bunch of cleanup is needed.","created_at":"2018-05-01T11:54:39Z","updated_at":"2018-05-08T08:53:54Z","closed_at":null,"merged_at":null,"merge_commit_sha":"67bd6ee90f3923067fb8f7b9740c85bd8b498986","assignee":null,"assignees":[],"requested_reviewers":[],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/zalando/patroni/pulls/672/commits","review_comments_url":"https://api.github.com/repos/zalando/patroni/pulls/672/comments","review_comment_url":"https://api.github.com/repos/zalando/patroni/pulls/comments{/number}","comments_url":"https://api.github.com/repos/zalando/patroni/issues/672/comments","statuses_url":"https://api.github.com/repos/zalando/patroni/statuses/0ae0cb0602a82b85a6f51b45d64f2097f6d11c72","head":{"label":"ants:feature/quorum","ref":"feature/quorum","sha":"0ae0cb0602a82b85a6f51b45d64f2097f6d11c72","user":{"login":"ants","id":64938,"avatar_url":"https://avatars2.githubusercontent.com/u/64938?v=4","gravatar_id":"","url":"https://api.github.com/users/ants","html_url":"https://github.com/ants","followers_url":"https://api.github.com/users/ants/followers","following_url":"https://api.github.com/users/ants/following{/other_user}","gists_url":"https://api.github.com/users/ants/gists{/gist_id}","starred_url":"https://api.github.com/users/ants/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ants/subscriptions","organizations_url":"https://api.github.com/users/ants/orgs","repos_url":"https://api.github.com/users/ants/repos","events_url":"https://api.github.com/users/ants/events{/privacy}","received_events_url":"https://api.github.com/users/ants/received_events","type":"User","site_admin":false},"repo":{"id":65907385,"name":"patroni","full_name":"ants/patroni","owner":{"login":"ants","id":64938,"avatar_url":"https://avatars2.githubusercontent.com/u/64938?v=4","gravatar_id":"","url":"https://api.github.com/users/ants","html_url":"https://github.com/ants","followers_url":"https://api.github.com/users/ants/followers","following_url":"https://api.github.com/users/ants/following{/other_user}","gists_url":"https://api.github.com/users/ants/gists{/gist_id}","starred_url":"https://api.github.com/users/ants/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ants/subscriptions","organizations_url":"https://api.github.com/users/ants/orgs","repos_url":"https://api.github.com/users/ants/repos","events_url":"https://api.github.com/users/ants/events{/privacy}","received_events_url":"https://api.github.com/users/ants/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/ants/patroni","description":"A template for PostgreSQL High Availability with ZooKeeper, etcd or Consul","fork":true,"url":"https://api.github.com/repos/ants/patroni","forks_url":"https://api.github.com/repos/ants/patroni/forks","keys_url":"https://api.github.com/repos/ants/patroni/keys{/key_id}","collaborators_url":"https://api.github.com/repos/ants/patroni/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/ants/patroni/teams","hooks_url":"https://api.github.com/repos/ants/patroni/hooks","issue_events_url":"https://api.github.com/repos/ants/patroni/issues/events{/number}","events_url":"https://api.github.com/repos/ants/patroni/events","assignees_url":"https://api.github.com/repos/ants/patroni/assignees{/user}","branches_url":"https://api.github.com/repos/ants/patroni/branches{/branch}","tags_url":"https://api.github.com/repos/ants/patroni/tags","blobs_url":"https://api.github.com/repos/ants/patroni/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/ants/patroni/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/ants/patroni/git/refs{/sha}","trees_url":"https://api.github.com/repos/ants/patroni/git/trees{/sha}","statuses_url":"https://api.github.com/repos/ants/patroni/statuses/{sha}","languages_url":"https://api.github.com/repos/ants/patroni/languages","stargazers_url":"https://api.github.com/repos/ants/patroni/stargazers","contributors_url":"https://api.github.com/repos/ants/patroni/contributors","subscribers_url":"https://api.github.com/repos/ants/patroni/subscribers","subscription_url":"https://api.github.com/repos/ants/patroni/subscription","commits_url":"https://api.github.com/repos/ants/patroni/commits{/sha}","git_commits_url":"https://api.github.com/repos/ants/patroni/git/commits{/sha}","comments_url":"https://api.github.com/repos/ants/patroni/comments{/number}","issue_comment_url":"https://api.github.com/repos/ants/patroni/issues/comments{/number}","contents_url":"https://api.github.com/repos/ants/patroni/contents/{+path}","compare_url":"https://api.github.com/repos/ants/patroni/compare/{base}...{head}","merges_url":"https://api.github.com/repos/ants/patroni/merges","archive_url":"https://api.github.com/repos/ants/patroni/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/ants/patroni/downloads","issues_url":"https://api.github.com/repos/ants/patroni/issues{/number}","pulls_url":"https://api.github.com/repos/ants/patroni/pulls{/number}","milestones_url":"https://api.github.com/repos/ants/patroni/milestones{/number}","notifications_url":"https://api.github.com/repos/ants/patroni/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/ants/patroni/labels{/name}","releases_url":"https://api.github.com/repos/ants/patroni/releases{/id}","deployments_url":"https://api.github.com/repos/ants/patroni/deployments","created_at":"2016-08-17T12:53:28Z","updated_at":"2016-08-17T12:53:29Z","pushed_at":"2018-05-01T11:52:32Z","git_url":"git://github.com/ants/patroni.git","ssh_url":"git@github.com:ants/patroni.git","clone_url":"https://github.com/ants/patroni.git","svn_url":"https://github.com/ants/patroni","homepage":"","size":3809,"stargazers_count":0,"watchers_count":0,"language":"Python","has_issues":false,"has_projects":true,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":1,"mirror_url":null,"archived":false,"open_issues_count":1,"license":{"key":"mit","name":"MIT License","spdx_id":"MIT","url":"https://api.github.com/licenses/mit"},"forks":1,"open_issues":1,"watchers":0,"default_branch":"master"}},"base":{"label":"zalando:master","ref":"master","sha":"3eeb4ed97922ff1535ffabc49d5a7c35248bfdf8","user":{"login":"zalando","id":1564818,"avatar_url":"https://avatars0.githubusercontent.com/u/1564818?v=4","gravatar_id":"","url":"https://api.github.com/users/zalando","html_url":"https://github.com/zalando","followers_url":"https://api.github.com/users/zalando/followers","following_url":"https://api.github.com/users/zalando/following{/other_user}","gists_url":"https://api.github.com/users/zalando/gists{/gist_id}","starred_url":"https://api.github.com/users/zalando/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zalando/subscriptions","organizations_url":"https://api.github.com/users/zalando/orgs","repos_url":"https://api.github.com/users/zalando/repos","events_url":"https://api.github.com/users/zalando/events{/privacy}","received_events_url":"https://api.github.com/users/zalando/received_events","type":"Organization","site_admin":false},"repo":{"id":38744670,"name":"patroni","full_name":"zalando/patroni","owner":{"login":"zalando","id":1564818,"avatar_url":"https://avatars0.githubusercontent.com/u/1564818?v=4","gravatar_id":"","url":"https://api.github.com/users/zalando","html_url":"https://github.com/zalando","followers_url":"https://api.github.com/users/zalando/followers","following_url":"https://api.github.com/users/zalando/following{/other_user}","gists_url":"https://api.github.com/users/zalando/gists{/gist_id}","starred_url":"https://api.github.com/users/zalando/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/zalando/subscriptions","organizations_url":"https://api.github.com/users/zalando/orgs","repos_url":"https://api.github.com/users/zalando/repos","events_url":"https://api.github.com/users/zalando/events{/privacy}","received_events_url":"https://api.github.com/users/zalando/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/zalando/patroni","description":"A template for PostgreSQL High Availability with ZooKeeper, etcd, or Consul","fork":false,"url":"https://api.github.com/repos/zalando/patroni","forks_url":"https://api.github.com/repos/zalando/patroni/forks","keys_url":"https://api.github.com/repos/zalando/patroni/keys{/key_id}","collaborators_url":"https://api.github.com/repos/zalando/patroni/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/zalando/patroni/teams","hooks_url":"https://api.github.com/repos/zalando/patroni/hooks","issue_events_url":"https://api.github.com/repos/zalando/patroni/issues/events{/number}","events_url":"https://api.github.com/repos/zalando/patroni/events","assignees_url":"https://api.github.com/repos/zalando/patroni/assignees{/user}","branches_url":"https://api.github.com/repos/zalando/patroni/branches{/branch}","tags_url":"https://api.github.com/repos/zalando/patroni/tags","blobs_url":"https://api.github.com/repos/zalando/patroni/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/zalando/patroni/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/zalando/patroni/git/refs{/sha}","trees_url":"https://api.github.com/repos/zalando/patroni/git/trees{/sha}","statuses_url":"https://api.github.com/repos/zalando/patroni/statuses/{sha}","languages_url":"https://api.github.com/repos/zalando/patroni/languages","stargazers_url":"https://api.github.com/repos/zalando/patroni/stargazers","contributors_url":"https://api.github.com/repos/zalando/patroni/contributors","subscribers_url":"https://api.github.com/repos/zalando/patroni/subscribers","subscription_url":"https://api.github.com/repos/zalando/patroni/subscription","commits_url":"https://api.github.com/repos/zalando/patroni/commits{/sha}","git_commits_url":"https://api.github.com/repos/zalando/patroni/git/commits{/sha}","comments_url":"https://api.github.com/repos/zalando/patroni/comments{/number}","issue_comment_url":"https://api.github.com/repos/zalando/patroni/issues/comments{/number}","contents_url":"https://api.github.com/repos/zalando/patroni/contents/{+path}","compare_url":"https://api.github.com/repos/zalando/patroni/compare/{base}...{head}","merges_url":"https://api.github.com/repos/zalando/patroni/merges","archive_url":"https://api.github.com/repos/zalando/patroni/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/zalando/patroni/downloads","issues_url":"https://api.github.com/repos/zalando/patroni/issues{/number}","pulls_url":"https://api.github.com/repos/zalando/patroni/pulls{/number}","milestones_url":"https://api.github.com/repos/zalando/patroni/milestones{/number}","notifications_url":"https://api.github.com/repos/zalando/patroni/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/zalando/patroni/labels{/name}","releases_url":"https://api.github.com/repos/zalando/patroni/releases{/id}","deployments_url":"https://api.github.com/repos/zalando/patroni/deployments","created_at":"2015-07-08T09:27:19Z","updated_at":"2018-05-07T23:55:11Z","pushed_at":"2018-05-01T11:54:40Z","git_url":"git://github.com/zalando/patroni.git","ssh_url":"git@github.com:zalando/patroni.git","clone_url":"https://github.com/zalando/patroni.git","svn_url":"https://github.com/zalando/patroni","homepage":"","size":3607,"stargazers_count":1634,"watchers_count":1634,"language":"Python","has_issues":true,"has_projects":false,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":149,"mirror_url":null,"archived":false,"open_issues_count":54,"license":{"key":"mit","name":"MIT License","spdx_id":"MIT","url":"https://api.github.com/licenses/mit"},"forks":149,"open_issues":54,"watchers":1634,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/zalando/patroni/pulls/672"},"html":{"href":"https://github.com/zalando/patroni/pull/672"},"issue":{"href":"https://api.github.com/repos/zalando/patroni/issues/672"},"comments":{"href":"https://api.github.com/repos/zalando/patroni/issues/672/comments"},"review_comments":{"href":"https://api.github.com/repos/zalando/patroni/pulls/672/comments"},"review_comment":{"href":"https://api.github.com/repos/zalando/patroni/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/zalando/patroni/pulls/672/commits"},"statuses":{"href":"https://api.github.com/repos/zalando/patroni/statuses/0ae0cb0602a82b85a6f51b45d64f2097f6d11c72"}},"author_association":"CONTRIBUTOR"}}
{"actor":{"display_login":"ants"}}