Commits don't block for synchronous replication
By setting up synchronized replication between primary and standby, commits
on primary should be blocked by function `SyncRepWaitForLSN()` until its
effects replicated to the standby.
Currently, `SyncRepWaitForLSN()` fast exits if the `SyncStandbysDefined()`
is false, which means GUC `synchronous_standby_names` is empty.
There is a race condition where the GUC is set but not reflected by some
backends. However, the `pg_stat_replication` `sync_state` is `sync`, and
the `state` is `streaming`, which means the commit should block and get
replicated to the mirror before it returns.
The proposed fix is to NOT do the fast exit based on the GUC, but only rely
on `WalSndCtl->sync_standbys_defined`.
Here is a quick repro:
- setup async replication
- create a backend
- set breakpoint at SyncRepWaitForLSN()
- Issue a ddl like `create table foo(c int)`
- Then the backend will be break at breakpoint SyncRepWaitForLSN()
- Set the GUC `synchronous_standby_names` to '*' in `postgresql.conf` to
trigger the synchronous replication.
- `pg_ctl` reload to signal the backend to reload the conf
- Check the `pg_stat_replication` to ensure `state` is `streaming` and
`sync_state` is `sync`.
- In this case, we expect the DDL is blocked.
- Then, step through the breakpoint until line:
```
-> 163 if (!SyncRepRequested() || !SyncStandbysDefined())
164 return;
```
- Check the content of the GUC as well as the
`WalSndCtl->sync_standbys_defined`
```
(lldb) p SyncRepStandbyNames
(char *) $1 = 0x00007fae31c03a80 ""
(lldb) p WalSndCtl->sync_standbys_defined
(bool) $2 = '\x01'
(lldb)
```
- As you can see the GUC is still not reflect with new content '*', but
the `sync_standbys_defined` is already changed to `1` by checkpointer
process.
- If we continue, the function will return, means the foo table will only
be created on primary, not on the standby.
If primary crashed at that moment, and failover to standby, the foo table
is lost, even though the replication is synced according to
`pg_stat_replication` view.
This is the patch we suggest as the fix:
```
diff --git a/src/backend/replication/syncrep.c
b/src/backend/replication/syncrep.c
index 8677235411..962772ef8f 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -156,11 +156,9 @@ SyncRepWaitForLSN(XLogRecPtr lsn, bool commit)
mode = Min(SyncRepWaitMode, SYNC_REP_WAIT_FLUSH); /*
- * Fast exit if user has not requested sync replication, or there
are no
- * sync replication standby names defined. Note that those standbys
don't
- * need to be connected.
+ * Fast exit if user has not requested sync replication.
*/
- if (!SyncRepRequested() || !SyncStandbysDefined())
+ if (!SyncRepRequested())
return; Assert(SHMQueueIsDetached(&(MyProc->syncRepLinks)));
```
A separate question, is the `pg_stat_replication` view the reliable way to
find when to failover to a standby, or there are some other ways to ensure
the standby is in-sync with the primary?
Thanks,
Xin Zhang, Ashwin Agrawal, and Asim R Praveen
On Tue, Sep 19, 2017 at 7:50 AM, Xin Zhang <xzhang@pivotal.io> wrote:
If primary crashed at that moment, and failover to standby, the foo table is
lost, even though the replication is synced according to
`pg_stat_replication` view.
GUC parameters are reloaded each time a query is run, and so
SyncRepConfig is filled with the parsed data of SyncRepStandbyNames
once the parameter is reloaded for the process. Still, here, a commit
is waiting for a signal from a WAL sender that the wanted LSN has been
correctly flushed on a standby so this code path does not care about
the state of SyncRepConfig saved in the context of the process, we
want to know what the checkpointer thinks about it. Hence using WAL
sender data or sync_standbys_defined as a source of truth looks like a
correct concept to me, making the problem of this bug legit.
The check with SyncRepRequested() still holds truth: max_wal_senders
needs a restart to be updated. Also, the other caller of
SyncStandbysDefined() requires SyncRepConfig to be set, so this caller
is fine.
I have looked at your patch and tested it, but found no problems
associated with it. A backpatch would be required, so I have added an
entry in the next commit fest with status set to "ready for committer"
so as this bug does not fall into the cracks.
A separate question, is the `pg_stat_replication` view the reliable way to
find when to failover to a standby, or there are some other ways to ensure
the standby is in-sync with the primary?
It shows at SQL level what is currently present in shared memory by
scanning all the WAL sender entries, so this report uses the same data
as the backend themselves, so that's a reliable source. In Postgres
10, pg_stat_activity is also able to show to users what are the
backends waiting for a change to be flushed/applied on the standby
using the wait event called SyncRep. You could make some use of that
as well.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 2:26 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Sep 19, 2017 at 7:50 AM, Xin Zhang <xzhang@pivotal.io> wrote:
If primary crashed at that moment, and failover to standby, the foo table is
lost, even though the replication is synced according to
`pg_stat_replication` view.GUC parameters are reloaded each time a query is run, and so
SyncRepConfig is filled with the parsed data of SyncRepStandbyNames
once the parameter is reloaded for the process. Still, here, a commit
is waiting for a signal from a WAL sender that the wanted LSN has been
correctly flushed on a standby so this code path does not care about
the state of SyncRepConfig saved in the context of the process, we
want to know what the checkpointer thinks about it. Hence using WAL
sender data or sync_standbys_defined as a source of truth looks like a
correct concept to me, making the problem of this bug legit.
I agree with the analysis and the approach of this patch.
The check with SyncRepRequested() still holds truth: max_wal_senders
needs a restart to be updated. Also, the other caller of
SyncStandbysDefined() requires SyncRepConfig to be set, so this caller
is fine.
Yeah, after reloaded the config there might be some wal senders that
don't reflect it yet but I think it cannot be a problem.
I have looked at your patch and tested it, but found no problems
associated with it. A backpatch would be required, so I have added an
entry in the next commit fest with status set to "ready for committer"
so as this bug does not fall into the cracks.
Also I found no problems in the patch.
A separate question, is the `pg_stat_replication` view the reliable way to
find when to failover to a standby, or there are some other ways to ensure
the standby is in-sync with the primary?It shows at SQL level what is currently present in shared memory by
scanning all the WAL sender entries, so this report uses the same data
as the backend themselves, so that's a reliable source. In Postgres
10, pg_stat_activity is also able to show to users what are the
backends waiting for a change to be flushed/applied on the standby
using the wait event called SyncRep. You could make some use of that
as well.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
https://commitfest.postgresql.org/15/1297/
Am I missing something or not looking at right place, this is marked as
committed but don't see the change in latest master ?
On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
https://commitfest.postgresql.org/15/1297/
Am I missing something or not looking at right place, this is marked as
committed but don't see the change in latest master ?
Good thing you double-checked. This has been marked as committed
eleven day ago by Simon (added in CC), but no commit has happened. I
am switching back the status as "ready for committer".
--
Michael
On 15 November 2017 at 10:07, Michael Paquier <michael.paquier@gmail.com> wrote:
On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
https://commitfest.postgresql.org/15/1297/
Am I missing something or not looking at right place, this is marked as
committed but don't see the change in latest master ?Good thing you double-checked. This has been marked as committed
eleven day ago by Simon (added in CC), but no commit has happened. I
am switching back the status as "ready for committer".
The patch has been applied - look at the code. Marking back to committed.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Nov 22, 2017 at 9:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 15 November 2017 at 10:07, Michael Paquier <michael.paquier@gmail.com>
wrote:On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal <aagrawal@pivotal.io>
wrote:
https://commitfest.postgresql.org/15/1297/
Am I missing something or not looking at right place, this is marked as
committed but don't see the change in latest master ?Good thing you double-checked. This has been marked as committed
eleven day ago by Simon (added in CC), but no commit has happened. I
am switching back the status as "ready for committer".The patch has been applied - look at the code. Marking back to committed.
I have no idea which magical place this is being committed, atleast don't
see on master unless checking something wrong, please can you post the
commit here ?
On Thu, Nov 23, 2017 at 4:32 AM, Ashwin Agrawal <aagrawal@pivotal.io> wrote:
On Wed, Nov 22, 2017 at 9:57 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 15 November 2017 at 10:07, Michael Paquier <michael.paquier@gmail.com>
wrote:On Wed, Nov 15, 2017 at 7:28 AM, Ashwin Agrawal <aagrawal@pivotal.io>
wrote:https://commitfest.postgresql.org/15/1297/
Am I missing something or not looking at right place, this is marked as
committed but don't see the change in latest master ?Good thing you double-checked. This has been marked as committed
eleven day ago by Simon (added in CC), but no commit has happened. I
am switching back the status as "ready for committer".The patch has been applied - look at the code. Marking back to committed.
I have no idea which magical place this is being committed, atleast don't
see on master unless checking something wrong, please can you post the
commit here ?
I am afraid that I have to agree with Ashwin here, and would like to
know the commit number where you applied it.
The code on HEAD (and back-branches) in syncrep.c, does that, in
SyncRepWaitForLSN():
/*
* Fast exit if user has not requested sync replication, or there are no
* sync replication standby names defined. Note that those
standbys don't
* need to be connected.
*/
if (!SyncRepRequested() || !SyncStandbysDefined())
return;
And the change proposed by Ashwin & co to address what is a bug is that:
/*
- * Fast exit if user has not requested sync replication, or there are no
- * sync replication standby names defined. Note that those
standbys don't
- * need to be connected.
+ * Fast exit if user has not requested sync replication.
*/
- if (!SyncRepRequested() || !SyncStandbysDefined())
+ if (!SyncRepRequested())
return;On top of that the last commit from a certain Simon Riggs on syncrep.c
is this one:
commit: e05f6f75dbe00a7349dccf1116b5ed983b4728c0
author: Simon Riggs <simon@2ndQuadrant.com>
date: Fri, 12 Aug 2016 12:43:45 +0100
Code cleanup in SyncRepWaitForLSN()
This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.
--
Michael
On 23 November 2017 at 11:11, Michael Paquier <michael.paquier@gmail.com> wrote:
This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.
Yes, that looks like it's my mistake. Thanks for rechecking.
Will commit and backpatch when I get home.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Nov 24, 2017 at 11:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 23 November 2017 at 11:11, Michael Paquier <michael.paquier@gmail.com> wrote:
This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.Yes, that looks like it's my mistake. Thanks for rechecking.
Will commit and backpatch when I get home.
Ping. Simon, are you planning to look at this patch, and potentially commit it?
I am moving this item to next CF for now.
--
Michael
On 1 December 2017 at 02:50, Michael Paquier <michael.paquier@gmail.com> wrote:
On Fri, Nov 24, 2017 at 11:38 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On 23 November 2017 at 11:11, Michael Paquier <michael.paquier@gmail.com> wrote:
This is older than the bug report of this thread. All those
indications point out that the patch has *not* been committed. So it
seems to me that you perhaps committed it to your local repository,
but forgot to push it to the remote. I am switching back the patch
status to what looks correct to me "Ready for committer". Thanks.Yes, that looks like it's my mistake. Thanks for rechecking.
Will commit and backpatch when I get home.
Ping. Simon, are you planning to look at this patch, and potentially commit it?
I am moving this item to next CF for now.
Applied, thanks.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Dec 29, 2017 at 02:45:09PM +0000, Simon Riggs wrote:
Applied, thanks.
Thanks, Simon.
--
Michael
On Sat, Dec 30, 2017 at 9:25 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Fri, Dec 29, 2017 at 02:45:09PM +0000, Simon Riggs wrote:
Applied, thanks.
Thank you!
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center