Re: [COMMITTERS] pgsql: Fix various possible problems with synchronous replication.

Fix various possible problems with synchronous replication.

1. Don't ignore query cancel interrupts. Instead, if the user asks to
cancel the query after we've already committed it, but before it's on
the standby, just emit a warning and let the COMMIT finish.

2. Don't ignore die interrupts (pg_terminate_backend or fast shutdown).
Instead, emit a warning message and close the connection without
acknowledging the commit. Other backends will still see the effect of
the commit, but there's no getting around that; it's too late to abort
at this point, and ignoring die interrupts altogether doesn't seem like
a good idea.

3. If synchronous_standby_names becomes empty, wake up all backends
waiting for synchronous replication to complete. Without this, someone
attempting to shut synchronous replication off could easily wedge the
entire system instead.

4. Avoid depending on the assumption that if a walsender updates
MyProc->syncRepState, we'll see the change even if we read it without
holding the lock. The window for this appears to be quite narrow (and
probably doesn't exist at all on machines with strong memory ordering)
but protecting against it is practically free, so do that.

5. Remove useless state SYNC_REP_MUST_DISCONNECT, which isn't needed and
doesn't actually do anything.

There's still some further work needed here to make the behavior of fast
shutdown plausible, but that looks complex, so I'm leaving it for a
separate commit. Review by Fujii Masao.

Branch
------
master

Details
-------
http://git.postgresql.org/pg/commitdiff/9a56dc3389b9470031e9ef8e45c95a680982e01a

Modified Files
--------------
doc/src/sgml/config.sgml | 3 +-
src/backend/postmaster/walwriter.c | 6 +
src/backend/replication/syncrep.c | 302 ++++++++++++++++++++++-------------
src/backend/tcop/postgres.c | 6 +
src/include/replication/syncrep.h | 4 +-
src/include/replication/walsender.h | 7 +
6 files changed, 214 insertions(+), 114 deletions(-)

On 17 March 2011 17:12, Robert Haas <rhaas(at)postgresql(dot)org> wrote:
> Fix various possible problems with synchronous replication.
>
> 1. Don't ignore query cancel interrupts.  Instead, if the user asks to
> cancel the query after we've already committed it, but before it's on
> the standby, just emit a warning and let the COMMIT finish.
>
> 2. Don't ignore die interrupts (pg_terminate_backend or fast shutdown).
> Instead, emit a warning message and close the connection without
> acknowledging the commit.  Other backends will still see the effect of
> the commit, but there's no getting around that; it's too late to abort
> at this point, and ignoring die interrupts altogether doesn't seem like
> a good idea.
>
> 3. If synchronous_standby_names becomes empty, wake up all backends
> waiting for synchronous replication to complete.  Without this, someone
> attempting to shut synchronous replication off could easily wedge the
> entire system instead.
>
> 4. Avoid depending on the assumption that if a walsender updates
> MyProc->syncRepState, we'll see the change even if we read it without
> holding the lock.  The window for this appears to be quite narrow (and
> probably doesn't exist at all on machines with strong memory ordering)
> but protecting against it is practically free, so do that.
>
> 5. Remove useless state SYNC_REP_MUST_DISCONNECT, which isn't needed and
> doesn't actually do anything.
>
> There's still some further work needed here to make the behavior of fast
> shutdown plausible, but that looks complex, so I'm leaving it for a
> separate commit.  Review by Fujii Masao.
>
> Branch
> ------
> master
>
> Details
> -------
> http://git.postgresql.org/pg/commitdiff/9a56dc3389b9470031e9ef8e45c95a680982e01a
>
> Modified Files
> --------------
> doc/src/sgml/config.sgml            |    3 +-
> src/backend/postmaster/walwriter.c  |    6 +
> src/backend/replication/syncrep.c   |  302 ++++++++++++++++++++++-------------
> src/backend/tcop/postgres.c         |    6 +
> src/include/replication/syncrep.h   |    4 +-
> src/include/replication/walsender.h |    7 +
> 6 files changed, 214 insertions(+), 114 deletions(-)

errmsg("canceling the wait for replication and terminating connection
due to administrator command")
errmsg("canceling wait for synchronous replication due to user request")

Should that first one then also say "synchronous replication"?

errdetail("The transaction has already been committed locally but
might have not been replicated to the standby.")));
errdetail("The transaction has committed locally, but may not have
replicated to the standby.")));

Could we have these saying precisely the same thing?

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 17, 2011 at 1:24 PM, Thom Brown <thom(at)linux(dot)com> wrote:
> errmsg("canceling the wait for replication and terminating connection
> due to administrator command")
> errmsg("canceling wait for synchronous replication due to user request")
>
> Should that first one then also say "synchronous replication"?

I could go either way. Clearly if it's asynchronous replication, we
wouldn't be waiting. But you're certainly right that we should be
consistent.

> errdetail("The transaction has already been committed locally but
> might have not been replicated to the standby.")));
> errdetail("The transaction has committed locally, but may not have
> replicated to the standby.")));
>
> Could we have these saying precisely the same thing?

Yeah. Which is better?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 17 March 2011 17:55, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 17, 2011 at 1:24 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>> errdetail("The transaction has already been committed locally but
>> might have not been replicated to the standby.")));
>> errdetail("The transaction has committed locally, but may not have
>> replicated to the standby.")));
>>
>> Could we have these saying precisely the same thing?
>
> Yeah.  Which is better?

Personally I prefer the 2nd. It reads better somehow.

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Mar 17, 2011 at 1:59 PM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 17 March 2011 17:55, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Thu, Mar 17, 2011 at 1:24 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>>> errdetail("The transaction has already been committed locally but
>>> might have not been replicated to the standby.")));
>>> errdetail("The transaction has committed locally, but may not have
>>> replicated to the standby.")));
>>>
>>> Could we have these saying precisely the same thing?
>>
>> Yeah.  Which is better?
>
> Personally I prefer the 2nd.  It reads better somehow.

I hacked on this a bit more and ended up with a hybrid of the two.
Hope you like it; but anyway it's consistent.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 18 March 2011 14:23, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Mar 17, 2011 at 1:59 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>> On 17 March 2011 17:55, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Mar 17, 2011 at 1:24 PM, Thom Brown <thom(at)linux(dot)com> wrote:
>>>> errdetail("The transaction has already been committed locally but
>>>> might have not been replicated to the standby.")));
>>>> errdetail("The transaction has committed locally, but may not have
>>>> replicated to the standby.")));
>>>>
>>>> Could we have these saying precisely the same thing?
>>>
>>> Yeah.  Which is better?
>>
>> Personally I prefer the 2nd.  It reads better somehow.
>
> I hacked on this a bit more and ended up with a hybrid of the two.
> Hope you like it; but anyway it's consistent.

Yes, cheers :)

--
Thom Brown
Twitter: @darkixion
IRC (freenode): dark_ixion
Registered Linux user: #516935

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company