Thread

Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-03T10:53:08Z

Latest version of Sync Rep, which includes substantial internal changes
and simplifications from previous version. (25-30 changes).

Includes all outstanding technical comments, typos and docs. I will
continue to work on self review and test myself, though actively
encourage others to test and report issues.

Interesting changes

* docs updated

* names listed in synchronous_standby_names are now in priority order

* synchronous_standby_names = "*" matches all standby names

* pg_stat_replication now shows standby priority - this is an ordinal
number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
standby".


The only *currently* outstanding point of discussion is the "when to
wait" debate, which we aren't moving quickly towards consensus on at
this stage. I see that as a "How should it work?" debate and something
we can chew over during Alpha/Beta, not as an immediate blocker to
commit.

Please comment on the patch and also watch changes to the repo
git://github.com/simon2ndQuadrant/postgres.git

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-03T15:02:43Z

On Thu, Mar 3, 2011 at 7:53 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Latest version of Sync Rep, which includes substantial internal changes
> and simplifications from previous version. (25-30 changes).
>
> Includes all outstanding technical comments, typos and docs. I will
> continue to work on self review and test myself, though actively
> encourage others to test and report issues.

Thanks for the patch!

> * synchronous_standby_names = "*" matches all standby names

Using '*' as the default seems to lead the performance degradation by
being connected from unexpected synchronous standby.

> * pg_stat_replication now shows standby priority - this is an ordinal
> number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> standby".

monitoring.sgml should be updated.

Though I've not read whole of the patch yet, here is the current comment:

Using MyProc->lwWaiting and lwWaitLink for backends to wait for replication
looks fragile. Since they are used also by lwlock, the value of them can be
changed unexpectedly. Instead, how about defining dedicated variables for
replication?

+		else if (WaitingForSyncRep)
+		{
+			/*
+			 * This must NOT be a FATAL message. We want the state of the
+			 * transaction being aborted to be indeterminate to ensure that
+			 * the transaction completion guarantee is never broken.
+			 */

The backend can reach this code path after returning the commit to the client.
Instead, how about doing this in EndCommand, to close the connection before
returning the commit?

+		LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
+		sync_priority = walsnd->sync_standby_priority;
+		LWLockRelease(SyncRepLock);

LW_SHARE can be used here, instead.

+			/*
+			 * Wait no longer if we have already reached our LSN
+			 */
+			if (XLByteLE(XactCommitLSN, queue->lsn))
+			{
+				/* No need to wait */
+				LWLockRelease(SyncRepLock);
+				return;
+			}

It might take long to acquire SyncRepLock, so how about comparing
our LSN with WalSnd->flush before here?

replication_timeout_client depends on GetCurrentTransactionStopTimestamp().
In COMMIT case, it's OK. But In PREPARE TRANSACTION, COMMIT PREPARED
and ROLLBACK PREPARED cases, it seems problematic because they don't call
SetCurrentTransactionStopTimestamp().

In SyncRepWaitOnQueue, the backend can theoretically call WaitLatch() again
after the wake-up from the latch. In this case, the "timeout" should
be calculated
again. Otherwise, it would take unexpectedly very long to cause the timeout.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-03T16:09:03Z

On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote: 
> > * synchronous_standby_names = "*" matches all standby names
> 
> Using '*' as the default seems to lead the performance degradation by
> being connected from unexpected synchronous standby.

You can configure it however you wish. It seemed better to have an out
of the box setting that was useful.

> > * pg_stat_replication now shows standby priority - this is an ordinal
> > number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> > standby".
> 
> monitoring.sgml should be updated.

Didn't think it needed to be, but I've added a few lines to explain.

> Though I've not read whole of the patch yet, here is the current comment:
> 
> Using MyProc->lwWaiting and lwWaitLink for backends to wait for replication
> looks fragile. Since they are used also by lwlock, the value of them can be
> changed unexpectedly. Instead, how about defining dedicated variables for
> replication?

Yes, I think the queue stuff needs a rewrite now.

> +		else if (WaitingForSyncRep)
> +		{
> +			/*
> +			 * This must NOT be a FATAL message. We want the state of the
> +			 * transaction being aborted to be indeterminate to ensure that
> +			 * the transaction completion guarantee is never broken.
> +			 */
> 
> The backend can reach this code path after returning the commit to the client.
> Instead, how about doing this in EndCommand, to close the connection before
> returning the commit?

OK, will look.

> +		LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
> +		sync_priority = walsnd->sync_standby_priority;
> +		LWLockRelease(SyncRepLock);
> 
> LW_SHARE can be used here, instead.

Seemed easier to keep it simple and have all lockers use LW_EXCLUSIVE.
But I've changed it for you.

> +			/*
> +			 * Wait no longer if we have already reached our LSN
> +			 */
> +			if (XLByteLE(XactCommitLSN, queue->lsn))
> +			{
> +				/* No need to wait */
> +				LWLockRelease(SyncRepLock);
> +				return;
> +			}
> 
> It might take long to acquire SyncRepLock, so how about comparing
> our LSN with WalSnd->flush before here?

If we're not the sync standby and we need to takeover the role of sync
standby we may need to issue a wakeup even though our standby reached
that LSN some time before. So we need to check each time.

> replication_timeout_client depends on GetCurrentTransactionStopTimestamp().
> In COMMIT case, it's OK. But In PREPARE TRANSACTION, COMMIT PREPARED
> and ROLLBACK PREPARED cases, it seems problematic because they don't call
> SetCurrentTransactionStopTimestamp().

Shame on them!

Seems reasonable that they should call
SetCurrentTransactionStopTimestamp().

I don't want to make a special case there for prepared transactions.

> In SyncRepWaitOnQueue, the backend can theoretically call WaitLatch() again
> after the wake-up from the latch. In this case, the "timeout" should
> be calculated
> again. Otherwise, it would take unexpectedly very long to cause the timeout.

That was originally modelled on on the way the statement_timeout timer
works. If it gets nudged and wakes up too early it puts itself back to
sleep to wakeup at the same time again.

I've renamed the variables to make that clearer and edited slightly.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-03-03T17:51:11Z

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote: 
>> > * synchronous_standby_names = "*" matches all standby names
>> 
>> Using '*' as the default seems to lead the performance degradation by
>> being connected from unexpected synchronous standby.
>
> You can configure it however you wish. It seemed better to have an out
> of the box setting that was useful.

Well the HBA still needs some opening before anyone can claim to be a
standby.  I guess the default line would be commented out and no standby
would be accepted as synchronous by default, assuming this GUC is sighup.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-03T18:14:06Z

On Thu, 2011-03-03 at 18:51 +0100, Dimitri Fontaine wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote: 
> >> > * synchronous_standby_names = "*" matches all standby names
> >> 
> >> Using '*' as the default seems to lead the performance degradation by
> >> being connected from unexpected synchronous standby.
> >
> > You can configure it however you wish. It seemed better to have an out
> > of the box setting that was useful.
> 
> Well the HBA still needs some opening before anyone can claim to be a
> standby.  I guess the default line would be commented out and no standby
> would be accepted as synchronous by default, assuming this GUC is sighup.

The patch sets "*" as the default, so all standbys are synchronous by
default.

Would you prefer it if it was blank, meaning no standbys are
synchronous, by default?

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-03T18:23:48Z

On Thu, Mar 3, 2011 at 1:14 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, 2011-03-03 at 18:51 +0100, Dimitri Fontaine wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>> > On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote:
>> >> > * synchronous_standby_names = "*" matches all standby names
>> >>
>> >> Using '*' as the default seems to lead the performance degradation by
>> >> being connected from unexpected synchronous standby.
>> >
>> > You can configure it however you wish. It seemed better to have an out
>> > of the box setting that was useful.
>>
>> Well the HBA still needs some opening before anyone can claim to be a
>> standby.  I guess the default line would be commented out and no standby
>> would be accepted as synchronous by default, assuming this GUC is sighup.
>
> The patch sets "*" as the default, so all standbys are synchronous by
> default.
>
> Would you prefer it if it was blank, meaning no standbys are
> synchronous, by default?

I think * is a reasonable default.

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

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-03T19:17:53Z

On Fri, 2011-03-04 at 00:02 +0900, Fujii Masao wrote:
> +               else if (WaitingForSyncRep)
> +               {
> +                       /*
> +                        * This must NOT be a FATAL message. We want
> the state of the
> +                        * transaction being aborted to be
> indeterminate to ensure that
> +                        * the transaction completion guarantee is
> never broken.
> +                        */
> 
> The backend can reach this code path after returning the commit to the
> client.
> Instead, how about doing this in EndCommand, to close the connection
> before
> returning the commit?

I don't really understand this comment.

You can't get there after returning the COMMIT message. Once we have
finished waiting we set WaitingForSyncRep = false, before we return to
RecordTransactionCommit() and continue from there.

Anyway, this is code in the interrupt handler and only gets executed
when we receive SIGTERM for a fast shutdown.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-03T21:27:35Z

On 2011-03-03 11:53, Simon Riggs wrote:
> Latest version of Sync Rep, which includes substantial internal changes
> and simplifications from previous version. (25-30 changes).
>
> Includes all outstanding technical comments, typos and docs. I will
> continue to work on self review and test myself, though actively
> encourage others to test and report issues.
>
> Interesting changes
>
> * docs updated
>
> * names listed in synchronous_standby_names are now in priority order
>
> * synchronous_standby_names = "*" matches all standby names
>
> * pg_stat_replication now shows standby priority - this is an ordinal
> number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> standby".
Some initial remarks:

1) this works nice:
application_name not in synchronous_standby_names -> sync_priority = 0 (OK)
change synchronous_standby_names to default *, reload conf -> 
sync_priority = 1 (OK)

message in log file
LOG:  00000: standby "walreceiver" is now the synchronous standby with 
priority 1

2) priorities
I have to get used to mapping the integers to synchronous replication 
meaning.
0 -> asynchronous
1 -> the synchronous standby that is waited for
2 and higher -> potential syncs

Could it be hidden from the user? I liked asynchronous / synchronous / 
potential synchronous

then the log message could be
LOG:  00000: standby "walreceiver" is now the synchronous standby

3) walreceiver is the default application name - could there be problems 
when a second standby with that name connects (ofcourse the same 
question holds for two the same nondefault application_names)?

regards
Yeb Havinga

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-03T21:49:07Z

On Thu, 2011-03-03 at 22:27 +0100, Yeb Havinga wrote:
> On 2011-03-03 11:53, Simon Riggs wrote:
> > Latest version of Sync Rep, which includes substantial internal changes
> > and simplifications from previous version. (25-30 changes).
> >
> > Includes all outstanding technical comments, typos and docs. I will
> > continue to work on self review and test myself, though actively
> > encourage others to test and report issues.
> >
> > Interesting changes
> >
> > * docs updated
> >
> > * names listed in synchronous_standby_names are now in priority order
> >
> > * synchronous_standby_names = "*" matches all standby names
> >
> > * pg_stat_replication now shows standby priority - this is an ordinal
> > number so "1" means 1st, "2" means 2nd etc, though 0 means "not a sync
> > standby".
> Some initial remarks:
> 
> 1) this works nice:
> application_name not in synchronous_standby_names -> sync_priority = 0 (OK)
> change synchronous_standby_names to default *, reload conf -> 
> sync_priority = 1 (OK)
> 
> message in log file
> LOG:  00000: standby "walreceiver" is now the synchronous standby with 
> priority 1
> 
> 2) priorities
> I have to get used to mapping the integers to synchronous replication 
> meaning.
> 0 -> asynchronous
> 1 -> the synchronous standby that is waited for
> 2 and higher -> potential syncs
> 
> Could it be hidden from the user? I liked asynchronous / synchronous / 
> potential synchronous

Yes, that sounds good. I will leave it as it is now to gain other
comments since this need not delay commit.

> then the log message could be
> LOG:  00000: standby "walreceiver" is now the synchronous standby

The priority is mentioned in the LOG message, so you can understand what
happens when multiple standbys connect.

e.g.

if you have synchronous_standby_names = 'a, b, c'

and then the standbys connect in the order b, c, a then you will see log
messages

LOG:  standby "b" is now the synchronous standby with priority 2
LOG:  standby "a" is now the synchronous standby with priority 1

It's designed so no matter which order standbys arrive in it is the
highest priority standby that makes it to the front in the end.

> 3) walreceiver is the default application name - could there be problems 
> when a second standby with that name connects (ofcourse the same 
> question holds for two the same nondefault application_names)?

That's documented: in that case which standby is sync is indeterminate.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Tom Lane <tgl@sss.pgh.pa.us> — 2011-03-03T22:01:07Z

Simon Riggs <simon@2ndQuadrant.com> writes:
> Anyway, this is code in the interrupt handler and only gets executed
> when we receive SIGTERM for a fast shutdown.

I trust it's not getting *directly* executed from the interrupt handler,
at least not without ImmediateInterruptOK.

			regards, tom lane

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T04:27:50Z

On Fri, Mar 4, 2011 at 7:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Anyway, this is code in the interrupt handler and only gets executed
>> when we receive SIGTERM for a fast shutdown.
>
> I trust it's not getting *directly* executed from the interrupt handler,
> at least not without ImmediateInterruptOK.

Yes, the backend waits for replication while cancel/die interrupt is
being blocked, i.e., InterruptHoldoffCount > 0. So SIGTERM doesn't
lead the waiting backend to there directly. The backend reaches there
after returning the result.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T04:35:17Z

On Fri, Mar 4, 2011 at 1:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Mar 4, 2011 at 7:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>> Anyway, this is code in the interrupt handler and only gets executed
>>> when we receive SIGTERM for a fast shutdown.
>>
>> I trust it's not getting *directly* executed from the interrupt handler,
>> at least not without ImmediateInterruptOK.
>
> Yes, the backend waits for replication while cancel/die interrupt is
> being blocked, i.e., InterruptHoldoffCount > 0. So SIGTERM doesn't
> lead the waiting backend to there directly. The backend reaches there
> after returning the result.

BTW, this is true in COMMIT and PREPARE cases, and false in
COMMIT PREPARED and ROLLBACK PREPARED cases. In the
latter cases, HOLD_INTERRUPT() is not called before waiting for
replication.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T06:16:08Z

On Fri, 2011-03-04 at 13:35 +0900, Fujii Masao wrote:
> On Fri, Mar 4, 2011 at 1:27 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > On Fri, Mar 4, 2011 at 7:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Simon Riggs <simon@2ndQuadrant.com> writes:
> >>> Anyway, this is code in the interrupt handler and only gets executed
> >>> when we receive SIGTERM for a fast shutdown.
> >>
> >> I trust it's not getting *directly* executed from the interrupt handler,
> >> at least not without ImmediateInterruptOK.
> >
> > Yes, the backend waits for replication while cancel/die interrupt is
> > being blocked, i.e., InterruptHoldoffCount > 0. So SIGTERM doesn't
> > lead the waiting backend to there directly. The backend reaches there
> > after returning the result.
> 
> BTW, this is true in COMMIT and PREPARE cases,

CommitTransaction() calls HOLD_INTERRUPT() and then RESUME_INTERRUPTS(),
which was reasonable before we started waiting for syncrep. The
interrupt does occur *before* we send the message back, but doesn't work
effectively at interrupting the wait in the way you would like.

If we RESUME_INTERRUPTS() prior to waiting and then HOLD again that
would allow all signals not just SIGTERM. We would need to selectively
reject everything except SIGTERM messages.

Ideas?

Alter ProcessInterrupts() to accept an interrupt if ProcDiePending &&
WaitingForSyncRep and InterruptHoldoffCount > 0. That looks a little
scary, but looks like it will work.

>  and false in
> COMMIT PREPARED and ROLLBACK PREPARED cases. In the
> latter cases, HOLD_INTERRUPT() is not called before waiting for
> replication.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T07:42:11Z

On Fri, Mar 4, 2011 at 12:02 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Though I've not read whole of the patch yet, here is the current comment:

Here are another comments:

+#replication_timeout_client = 120   # 0 means wait forever

Typo: s/replication_timeout_client/sync_replication_timeout

+			else if (timeout > 0 &&
+				TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
+											wait_start, timeout))

If SetCurrentTransactionStopTimestamp() is called before (i.e., COMMIT case),
the return value of GetCurrentTransactionStopTimestamp() is the same as
"wait_start". So, in this case, the timeout never expires.

+				strcpy(new_status + len, " waiting for sync rep");
+				set_ps_display(new_status, false);

How about changing the message to something like "waiting for %X/%X"
(%X/%X indicates the LSN which the backend is waiting for)?

Please initialize MyProc->procWaitLink to NULL in InitProcess() as well as
do MyProc->lwWaitLink.

+	/*
+	 * We're a potential sync standby. Release waiters if we are the
+	 * highest priority standby. We do this even if the standby is not yet
+	 * caught up, in case this is a restart situation and
+	 * there are backends waiting for us. That allows backends to exit the
+	 * wait state even if new backends cannot yet enter the wait state.
+	 */

I don't think that it's good idea to switch the high priority standby which has
not caught up, to the sync one, especially when there is already another
sync standby. Because that degrades replication from sync to async for
a while, even though there is sync standby which has caught up.

+		if (walsnd->pid != 0 &&
+			walsnd->sync_standby_priority > 0 &&
+			(priority == 0 ||
+			 priority < walsnd->sync_standby_priority))
+		{
+			 priority = walsnd->sync_standby_priority;
+			 syncWalSnd = walsnd;
+		}

According to the code, the last named standby has highest priority. But the
document says the opposite.

ISTM the waiting backends can be sent the wake-up signal by the
walsender multiple times since the walsender doesn't remove any
entry from the queue. Isn't this unsafe? waste of the cycle?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T08:34:08Z

On Fri, Mar 4, 2011 at 3:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> CommitTransaction() calls HOLD_INTERRUPT() and then RESUME_INTERRUPTS(),
> which was reasonable before we started waiting for syncrep. The
> interrupt does occur *before* we send the message back, but doesn't work
> effectively at interrupting the wait in the way you would like.
>
> If we RESUME_INTERRUPTS() prior to waiting and then HOLD again that
> would allow all signals not just SIGTERM. We would need to selectively
> reject everything except SIGTERM messages.
>
> Ideas?
>
> Alter ProcessInterrupts() to accept an interrupt if ProcDiePending &&
> WaitingForSyncRep and InterruptHoldoffCount > 0. That looks a little
> scary, but looks like it will work.

If shutdown is requested before WaitingForSyncRep is set to TRUE and
after HOLD_INTERRUPT() is called, the waiting backends cannot be
interrupted.

SIGTERM can be sent by pg_terminate_backend(). So we should check
whether shutdown is requested before emitting WARNING and closing
the connection. If it's not requested yet, I think that it's safe to return the
success indication to the client.

I think that it's safer to close the connection and terminate the backend
after cleaning all the resources. So, as I suggested before, how about
doing that in EndCommand()?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T10:21:37Z

On Fri, 2011-03-04 at 17:34 +0900, Fujii Masao wrote:
> On Fri, Mar 4, 2011 at 3:16 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > CommitTransaction() calls HOLD_INTERRUPT() and then RESUME_INTERRUPTS(),
> > which was reasonable before we started waiting for syncrep. The
> > interrupt does occur *before* we send the message back, but doesn't work
> > effectively at interrupting the wait in the way you would like.
> >
> > If we RESUME_INTERRUPTS() prior to waiting and then HOLD again that
> > would allow all signals not just SIGTERM. We would need to selectively
> > reject everything except SIGTERM messages.
> >
> > Ideas?
> >
> > Alter ProcessInterrupts() to accept an interrupt if ProcDiePending &&
> > WaitingForSyncRep and InterruptHoldoffCount > 0. That looks a little
> > scary, but looks like it will work.
> 
> If shutdown is requested before WaitingForSyncRep is set to TRUE and
> after HOLD_INTERRUPT() is called, the waiting backends cannot be
> interrupted.
> 
> SIGTERM can be sent by pg_terminate_backend(). So we should check
> whether shutdown is requested before emitting WARNING and closing
> the connection. If it's not requested yet, I think that it's safe to return the
> success indication to the client.

I'm not sure if that matters. Nobody apart from the postmaster knows
about a shutdown. All the other processes know is that they received
SIGTERM, which as you say could have been a specific user action aimed
at an individual process.

We need a way to end the wait state explicitly, so it seems easier to
make SIGTERM the initiating action, no matter how it is received.

The alternative is to handle it this way
1) set something in shared memory
2) set latch of all backends
3) have the backends read shared memory and then end the wait

Who would do (1) and (2)? Not the backend, its sleeping, not the
postmaster its shm, nor a WALSender cos it might not be there.

Seems like a lot of effort to avoid SIGTERM. Do we have a good reason
why we need that? Might it introduce other issues?

> I think that it's safer to close the connection and terminate the backend
> after cleaning all the resources. So, as I suggested before, how about
> doing that in EndCommand()?

Yes, if we don't use SIGTERM then we would use EndCommand()

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T10:51:03Z

On Fri, 2011-03-04 at 16:42 +0900, Fujii Masao wrote:
> On Fri, Mar 4, 2011 at 12:02 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > Though I've not read whole of the patch yet, here is the current comment:
> 
> Here are another comments:
> 
> +#replication_timeout_client = 120   # 0 means wait forever
> 
> Typo: s/replication_timeout_client/sync_replication_timeout

Done

> +			else if (timeout > 0 &&
> +				TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
> +											wait_start, timeout))
> 
> If SetCurrentTransactionStopTimestamp() is called before (i.e., COMMIT case),
> the return value of GetCurrentTransactionStopTimestamp() is the same as
> "wait_start". So, in this case, the timeout never expires.

Don't understand (still)

> +				strcpy(new_status + len, " waiting for sync rep");
> +				set_ps_display(new_status, false);
> 
> How about changing the message to something like "waiting for %X/%X"
> (%X/%X indicates the LSN which the backend is waiting for)?

Done

> Please initialize MyProc->procWaitLink to NULL in InitProcess() as well as
> do MyProc->lwWaitLink.

I'm rewriting that aspect now.

> +	/*
> +	 * We're a potential sync standby. Release waiters if we are the
> +	 * highest priority standby. We do this even if the standby is not yet
> +	 * caught up, in case this is a restart situation and
> +	 * there are backends waiting for us. That allows backends to exit the
> +	 * wait state even if new backends cannot yet enter the wait state.
> +	 */
> 
> I don't think that it's good idea to switch the high priority standby which has
> not caught up, to the sync one, especially when there is already another
> sync standby. Because that degrades replication from sync to async for
> a while, even though there is sync standby which has caught up.

OK, that wasn't really my intention. Changed.

> +		if (walsnd->pid != 0 &&
> +			walsnd->sync_standby_priority > 0 &&
> +			(priority == 0 ||
> +			 priority < walsnd->sync_standby_priority))
> +		{
> +			 priority = walsnd->sync_standby_priority;
> +			 syncWalSnd = walsnd;
> +		}
> 
> According to the code, the last named standby has highest priority. But the
> document says the opposite.

Priority is a difficult word here since "1" is the highest priority. I
deliberately avoided using the word "highest" in the code for that
reason.

The code above finds the lowest non-zero standby, which is correct as
documented.

> ISTM the waiting backends can be sent the wake-up signal by the
> walsender multiple times since the walsender doesn't remove any
> entry from the queue. Isn't this unsafe? waste of the cycle?

It's ok to set a latch that isn't set. It's unlikely to wake someone
twice before they can remove themselves.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T11:08:17Z

On Fri, 2011-03-04 at 10:51 +0000, Simon Riggs wrote:

> > +			else if (timeout > 0 &&
> > +				TimestampDifferenceExceeds(GetCurrentTransactionStopTimestamp(),
> > +											wait_start, timeout))
> > 
> > If SetCurrentTransactionStopTimestamp() is called before (i.e., COMMIT case),
> > the return value of GetCurrentTransactionStopTimestamp() is the same as
> > "wait_start". So, in this case, the timeout never expires.
> 
> Don't understand (still)

OK, coffee has seeped into brain now, thanks.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-04T11:24:06Z

On 2011-03-03 11:53, Simon Riggs wrote:
> Latest version of Sync Rep, which includes substantial internal changes
> and simplifications from previous version. (25-30 changes).
Testing more with the post v19 version from github with HEAD

commit 009875662e1b47012e1f4b7d30eb9e238d1937f6
Author: Simon Riggs <simon@2ndquadrant.com>
Date:   Fri Mar 4 06:13:43 2011 +0000

     Allow SIGTERM messages in ProcessInterrupts() even when interrupts are
     held, if WaitingForSyncRep

1) unexpected behaviour
- master has synchronous_standby_names = 'standby1,standby2,standby3'
- standby with 'standby2' connects first.
- LOG:  00000: standby "standby2" is now the synchronous standby with 
priority 2

I'm still confused by the priority numbers. At first I thought that 
priority 1 meant: this is the one that is currently waited for. Now I'm 
not sure if this is the first potential standby that is not used, or 
that it is actually the one waited for.
What I expected was that it would be connected with priority 1. And then 
if the standby1 connect, it would become the one with prio1 and standby2 
with prio2.

2) unexpected behaviour
- continued from above
- standby with 'asyncone' name connects next
- no log message on master

I expected a log message along the lines 'standby "asyncone" is now an 
asynchronous standby'

3) more about log messages
- didn't get a log message that the asyncone standby stopped
- didn't get a log message that standby1 connected with priority 1
- after stop / start master, again only got a log that standby2 
connectied with priority 2
- pg_stat_replication showed both standb1 and standby2 with correct prio#

4) More about the priority stuff. At this point I figured out prio 2 can 
also be 'the real sync'. Still I'd prefer in pg_stat_replication a 
boolean that clearly shows 'this is the one', with a source that is 
intimately connected to the syncrep implemenation, instead of a 
different implementation of 'if lowest connected priority and > 0, then 
sync is true. If there are two different implementations, there is room 
for differences, which doesn't feel right.

5) performance.
Seems to have dropped a a few dozen %. With v17 I earlier got ~650 tps 
and after some more tuning over 900 tps. Now with roughly the same setup 
I get ~ 550 tps. Both versions on the same hardware, both compiled 
without debugging, and I used the same postgresql.conf start config.

I'm currently thinking about a failure test that would check if a commit 
has really waited for the standby. What's the worst thing to do to a 
master server? Ideas are welcome :-)

#!/bin/sh
psql -c "create a big table with generate_series"
echo 1 > /proc/sys/kernel/sysrq ; echo b > /proc/sysrq-trigger

regards,
Yeb Havinga

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T11:45:41Z

On Fri, 2011-03-04 at 12:24 +0100, Yeb Havinga wrote:
> On 2011-03-03 11:53, Simon Riggs wrote:
> > Latest version of Sync Rep, which includes substantial internal changes
> > and simplifications from previous version. (25-30 changes).
> Testing more with the post v19 version from github with HEAD

Thanks

> commit 009875662e1b47012e1f4b7d30eb9e238d1937f6
> Author: Simon Riggs <simon@2ndquadrant.com>
> Date:   Fri Mar 4 06:13:43 2011 +0000
> 
>      Allow SIGTERM messages in ProcessInterrupts() even when interrupts are
>      held, if WaitingForSyncRep
> 
> 
> 1) unexpected behaviour
> - master has synchronous_standby_names = 'standby1,standby2,standby3'
> - standby with 'standby2' connects first.
> - LOG:  00000: standby "standby2" is now the synchronous standby with 
> priority 2
> 
> I'm still confused by the priority numbers. At first I thought that 
> priority 1 meant: this is the one that is currently waited for. Now I'm 
> not sure if this is the first potential standby that is not used, or 
> that it is actually the one waited for.
> What I expected was that it would be connected with priority 1. And then 
> if the standby1 connect, it would become the one with prio1 and standby2 
> with prio2.

The priority refers to the order in which that standby is listed in
synchronous_standby_names. That is not dependent upon who is currently
connected. It doesn't mean the order in which the currently connected
standbys will become the sync standby.

So the log message allows you to work out that "standby2" is connected
and will operate as sync standby until something mentioned earlier in
synchronous_standby_names, in this case standby1, connects.

> 2) unexpected behaviour
> - continued from above
> - standby with 'asyncone' name connects next
> - no log message on master
> 
> I expected a log message along the lines 'standby "asyncone" is now an 
> asynchronous standby'

That would introduce messages where there currently aren't any, so I
left that out. I'll put it in for clarity.

> 3) more about log messages
> - didn't get a log message that the asyncone standby stopped
OK
> - didn't get a log message that standby1 connected with priority 1
Bad
> - after stop / start master, again only got a log that standby2 
> connectied with priority 2
Bad
> - pg_stat_replication showed both standb1 and standby2 with correct prio#
Good

Please send me log output at DEBUG3 offline.

> 4) More about the priority stuff. At this point I figured out prio 2 can 
> also be 'the real sync'. Still I'd prefer in pg_stat_replication a 
> boolean that clearly shows 'this is the one', with a source that is 
> intimately connected to the syncrep implemenation, instead of a 
> different implementation of 'if lowest connected priority and > 0, then 
> sync is true. If there are two different implementations, there is room 
> for differences, which doesn't feel right.

OK

> 5) performance.
> Seems to have dropped a a few dozen %. With v17 I earlier got ~650 tps 
> and after some more tuning over 900 tps. Now with roughly the same setup 
> I get ~ 550 tps. Both versions on the same hardware, both compiled 
> without debugging, and I used the same postgresql.conf start config.

Will need to re-look at performance after commit

> I'm currently thinking about a failure test that would check if a commit 
> has really waited for the standby. What's the worst thing to do to a 
> master server? Ideas are welcome :-)
> 
> #!/bin/sh
> psql -c "create a big table with generate_series"
> echo 1 > /proc/sys/kernel/sysrq ; echo b > /proc/sysrq-trigger
> 
> regards,
> Yeb Havinga
> 

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-04T13:57:46Z

On 2011-03-04 12:24, Yeb Havinga wrote:
> I'm currently thinking about a failure test that would check if a 
> commit has really waited for the standby. What's the worst thing to do 
> to a master server? Ideas are welcome :-)
>
> #!/bin/sh
> psql -c "create a big table with generate_series"
> echo 1 > /proc/sys/kernel/sysrq ; echo b > /proc/sysrq-trigger
Did that with both a sync and async standby server, then promoted both 
replicas.

Both replicas had the complete big table. Maybe the async server was 
somehow 'saved' by the master waiting for the sync server? Test repeated 
with only the async one connected.

The master then shows this at restart
LOG:  00000: record with zero length at 4/B2CD3598
LOG:  00000: redo done at 4/B2CD3558
LOG:  00000: last completed transaction was at log time 2011-03-04 
14:43:31.02041+01

The async promoted server
LOG:  00000: record with zero length at 4/B2CC9260
LOG:  00000: redo done at 4/B2CC9220
LOG:  00000: last completed transaction was at log time 2011-03-04 
14:43:31.018444+01

Even though the async server had the complete relation I created, 
something was apparently done just before the reboot.

Test repeated with only 1 sync standby

Then on master at recovery
LOG:  00000: record with zero length at 4/D1051C88
LOG:  00000: redo done at 4/D1051C48
LOG:  00000: last completed transaction was at log time 2011-03-04 
14:52:11.035188+01

on the sync promoted server
LOG:  00000: redo done at 4/D1051C48
LOG:  00000: last completed transaction was at log time 2011-03-04 
14:52:11.035188+01

Nice!

regards,
Yeb Havinga

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T14:15:50Z

On Fri, Mar 4, 2011 at 7:51 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> +             if (walsnd->pid != 0 &&
>> +                     walsnd->sync_standby_priority > 0 &&
>> +                     (priority == 0 ||
>> +                      priority < walsnd->sync_standby_priority))
>> +             {
>> +                      priority = walsnd->sync_standby_priority;
>> +                      syncWalSnd = walsnd;
>> +             }
>>
>> According to the code, the last named standby has highest priority. But the
>> document says the opposite.
>
> Priority is a difficult word here since "1" is the highest priority. I
> deliberately avoided using the word "highest" in the code for that
> reason.
>
> The code above finds the lowest non-zero standby, which is correct as
> documented.

Hmm.. that seems to find the highest standby. And, I could confirm
that in my box. Please see the following. The priority (= 2) of
synchronous standby (its sync_state is SYNC) is higher than that (= 1)
of potential one (its sync_state is POTENTIAL).

postgres=# SHOW synchronous_standby_names ;
 synchronous_standby_names
---------------------------
 one, two
(1 row)

postgres=# SELECT application_name, state, sync_priority, sync_state
FROM pg_stat_replication;
 application_name |   state   | sync_priority | sync_state
------------------+-----------+---------------+------------
 one              | STREAMING |             1 | POTENTIAL
 two              | STREAMING |             2 | SYNC
(2 rows)

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T15:18:38Z

On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:

> postgres=# SELECT application_name, state, sync_priority, sync_state
> FROM pg_stat_replication;
>  application_name |   state   | sync_priority | sync_state
> ------------------+-----------+---------------+------------
>  one              | STREAMING |             1 | POTENTIAL
>  two              | STREAMING |             2 | SYNC
> (2 rows)

Bug! Thanks.

Fixed

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-04T20:04:48Z

On Fri, Mar 4, 2011 at 7:21 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> SIGTERM can be sent by pg_terminate_backend(). So we should check
>> whether shutdown is requested before emitting WARNING and closing
>> the connection. If it's not requested yet, I think that it's safe to return the
>> success indication to the client.
>
> I'm not sure if that matters. Nobody apart from the postmaster knows
> about a shutdown. All the other processes know is that they received
> SIGTERM, which as you say could have been a specific user action aimed
> at an individual process.
>
> We need a way to end the wait state explicitly, so it seems easier to
> make SIGTERM the initiating action, no matter how it is received.
>
> The alternative is to handle it this way
> 1) set something in shared memory
> 2) set latch of all backends
> 3) have the backends read shared memory and then end the wait
>
> Who would do (1) and (2)? Not the backend, its sleeping, not the
> postmaster its shm, nor a WALSender cos it might not be there.
>
> Seems like a lot of effort to avoid SIGTERM. Do we have a good reason
> why we need that? Might it introduce other issues?

On the second thought...

I was totally wrong. Preventing the backend from returning the commit
when shutdown is requested doesn't help to avoid the data loss at all.
Without shutdown, the following simple scenario can cause data loss.

1. Replication connection is closed because of network outage.
2. Though replication has not been completed, the waiting backend is
    released since the timeout expires. Then it returns the success to
    the client.
3. The primary crashes, and then the clusterware promotes the standby
    which doesn't have the latest change on the primary to new primary.
    Data lost happens!

In the first place, there are two kinds of data loss:

(A) Pysical data loss
This is the case where we can never retrieve the committed data
physically. For example, if the storage of the standalone server gets
corrupted, we would lose some data forever. To avoid this type of
data loss, we would have to choose the "wait-forever" behavior. But
as I said in upthread, we can decrease the risk of this data loss to
a certain extent by spending much money on the storage. So, if that
cost is less than the cost which we have to pay when down-time
happens, we don't need to choose the "wait-forever" option.

(B) Logical data loss
This is the case where we think wrongly that the committed data
has been lost while we can actually retrieve it physically. For example,
in the above three-steps scenario, we can read all the committed data
from two servers physically even after failover. But since the client
attempts to read data only from new primary, some data looks lost to
the client. The "wait-forever" behavior can help also to avoid this type
of data loss. And, another way is to STONITH the standby before the
timeout releases any waiting backend. If so, we can completely prevent
the outdated standby from being brought up, and can avoid logical data
loss. According to my quick research, in DRBD, the "dopd (DRBD
outdate-peer daemon)" plays that role.

What I'd like to avoid is (B). Though (A) is more serious problem than (B),
we already have some techniques to decrease the risk of (A). But not
(B), I think.

The "wait-forever" might be a straightforward approach against (B). But
this option prevents transactions from running not only when the
synchronous standby goes away, but also when the primary is invoked
first or when the standby is promoted at failover. Since the availability
of the database service decreases very much, I don't want to use that.

Keeping transactions waiting in the latter two cases would be required
to avoid (A), but not (B). So I think that we can relax the "wait-forever"
option so that it allows not-replicated transactions to complete only in
those cases. IOW, when we initially start the primary, the backends
don't wait at all for new standby to connect. And, while new primary is
running alone after failover, the backends don't wait at all, too. Only
when replication connection is closed while streaming WAL to sync
standby, the backends wait until new sync standby has connected and
replication has been completed. Even in this case, if we want to
improve the service availability, we have only to make something like
dopd to STONITH the outdated standby, and then request the primary
to release the waiting backends. So I think that the interface to
request that release should be implemented.

Fortunately, that partial "wait-forever" behavior has already been
implemented in Simon's patch with the client timeout = 0 (disable).
If he implements the interface to release the waiting backends,
I'm OK with his design about when to release the backends for 9.1
(unless I'm missing something).

Thought?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-04T20:27:07Z

On Fri, Mar 4, 2011 at 3:04 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> The "wait-forever" might be a straightforward approach against (B). But
> this option prevents transactions from running not only when the
> synchronous standby goes away, but also when the primary is invoked
> first or when the standby is promoted at failover. Since the availability
> of the database service decreases very much, I don't want to use that.

I continue to think that wait-forever is the most sensible option.  If
you want all of your data on the disks of two machines before the
commit is ack'd, I think you probably want that all the time.  The
second scenario you mentioned ("when the standby is promoted at
failover") is quite easy to handle.  If you don't want synchronous
replication after a standby promotion, then configure the master to do
synchronous replication and the slave not to do synchronous
replication.  Similarly, if you've got an existing machine that is not
doing synchronous replication and you want to start, fire up the
standby in asynchronous mode and switch to synchronous replication
after it has fully caught up.  It seems to me that we're bent on
providing a service that does synchronous replication except when it
first starts up or when the timeout expires or when the phase of the
moon is waxing gibbons, and I don't get the point of that.  If I ask
for synchronous replication, I want it to be synchronous until I
explicitly turn it off.  Otherwise, when I fail over, how do I know if
I've got all my transactions, or not?

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

Re: Sync Rep v19

Jaime Casanova <jaime@2ndquadrant.com> — 2011-03-04T21:18:39Z

On Thu, Mar 3, 2011 at 1:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>
>> The patch sets "*" as the default, so all standbys are synchronous by
>> default.
>>
>> Would you prefer it if it was blank, meaning no standbys are
>> synchronous, by default?
>
> I think * is a reasonable default.
>

Actually i would prefer to have standbys asynchronous by default...
though is true that there will be no waits until i set
synchronous_replication to on... 1) it could be confusing to see a
SYNC standby in pg_stat_replication by default when i wanted all of
them to be async, 2) also * will give priority 1 to all standbys so it
doesn't seem like a very useful out-of-the-box configuration, better
to make the dba to write the standby names in the order they want

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-04T21:53:59Z

On Fri, Mar 4, 2011 at 4:18 PM, Jaime Casanova <jaime@2ndquadrant.com> wrote:
> On Thu, Mar 3, 2011 at 1:23 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>>
>>> The patch sets "*" as the default, so all standbys are synchronous by
>>> default.
>>>
>>> Would you prefer it if it was blank, meaning no standbys are
>>> synchronous, by default?
>>
>> I think * is a reasonable default.
>>
>
> Actually i would prefer to have standbys asynchronous by default...
> though is true that there will be no waits until i set
> synchronous_replication to on... 1) it could be confusing to see a
> SYNC standby in pg_stat_replication by default when i wanted all of
> them to be async, 2) also * will give priority 1 to all standbys so it
> doesn't seem like a very useful out-of-the-box configuration, better
> to make the dba to write the standby names in the order they want

Mmm, good points.

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

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-04T22:20:14Z

On 2011-03-04 22:18, Jaime Casanova wrote:
> On Thu, Mar 3, 2011 at 1:23 PM, Robert Haas<robertmhaas@gmail.com>  wrote:
>>> The patch sets "*" as the default, so all standbys are synchronous by
>>> default.
>>>
>>> Would you prefer it if it was blank, meaning no standbys are
>>> synchronous, by default?
>> I think * is a reasonable default.
>>
> Actually i would prefer to have standbys asynchronous by default...
> though is true that there will be no waits until i set
> synchronous_replication to on... 1) it could be confusing to see a
> SYNC standby in pg_stat_replication by default when i wanted all of
> them to be async,
I see no problem with * for synchronous_standby names, such that *if* 
synchronous_replication = on, then all standbys are sync. Also for the 
beginning experimenter with sync rep: what would you expect after only 
turning 'synchronous_replication' = on? ISTM better than : you need to 
change two parameters from their default to get a replica in sync mode.
>   2) also * will give priority 1 to all standbys so it
> doesn't seem like a very useful out-of-the-box configuration, better
> to make the dba to write the standby names in the order they want
>
As somebody with a usecase for two hardware-wise equal sync replicas for 
the same master (and a single async replica), the whole ordering of sync 
standbys is too much feature anyway, since it will cause unneccesary 
'which is the sync replica' switching. Besides that, letting all syncs 
have the same priority sounds like the only thing the server can do, if 
the dba has not specified it explicitly. I would see it as improvement 
if order in standby_names doesn't mean priority, and that priority could 
be specified with another parameter (and default: all sync priorities 
the same)

regards,
Yeb Havinga

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-04T22:28:42Z

On Sat, 2011-03-05 at 05:04 +0900, Fujii Masao wrote:
> On Fri, Mar 4, 2011 at 7:21 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> >> SIGTERM can be sent by pg_terminate_backend(). So we should check
> >> whether shutdown is requested before emitting WARNING and closing
> >> the connection. If it's not requested yet, I think that it's safe to return the
> >> success indication to the client.
> >
> > I'm not sure if that matters. Nobody apart from the postmaster knows
> > about a shutdown. All the other processes know is that they received
> > SIGTERM, which as you say could have been a specific user action aimed
> > at an individual process.
> >
> > We need a way to end the wait state explicitly, so it seems easier to
> > make SIGTERM the initiating action, no matter how it is received.
> >
> > The alternative is to handle it this way
> > 1) set something in shared memory
> > 2) set latch of all backends
> > 3) have the backends read shared memory and then end the wait
> >
> > Who would do (1) and (2)? Not the backend, its sleeping, not the
> > postmaster its shm, nor a WALSender cos it might not be there.
> >
> > Seems like a lot of effort to avoid SIGTERM. Do we have a good reason
> > why we need that? Might it introduce other issues?
> 
> On the second thought...
> 
> I was totally wrong. Preventing the backend from returning the commit
> when shutdown is requested doesn't help to avoid the data loss at all.
> Without shutdown, the following simple scenario can cause data loss.
> 
> 1. Replication connection is closed because of network outage.
> 2. Though replication has not been completed, the waiting backend is
>     released since the timeout expires. Then it returns the success to
>     the client.
> 3. The primary crashes, and then the clusterware promotes the standby
>     which doesn't have the latest change on the primary to new primary.
>     Data lost happens!

Yes, that can happen. As people will no doubt observe, this seems to be
an argument for wait-forever. What we actually need is a wait that lasts
longer than it takes for us to decide to failover, if the standby is
actually up and this is some kind of split brain situation. That way the
clients are still waiting when failover occurs. WAL is missing, but
since we didn't acknowledge the client we are OK to treat that situation
as if it were an abort.

> In the first place, there are two kinds of data loss:
> 
> (A) Pysical data loss
> This is the case where we can never retrieve the committed data
> physically. For example, if the storage of the standalone server gets
> corrupted, we would lose some data forever. To avoid this type of
> data loss, we would have to choose the "wait-forever" behavior. But
> as I said in upthread, we can decrease the risk of this data loss to
> a certain extent by spending much money on the storage. So, if that
> cost is less than the cost which we have to pay when down-time
> happens, we don't need to choose the "wait-forever" option.
> 
> (B) Logical data loss
> This is the case where we think wrongly that the committed data
> has been lost while we can actually retrieve it physically. For example,
> in the above three-steps scenario, we can read all the committed data
> from two servers physically even after failover. But since the client
> attempts to read data only from new primary, some data looks lost to
> the client. The "wait-forever" behavior can help also to avoid this type
> of data loss. And, another way is to STONITH the standby before the
> timeout releases any waiting backend. If so, we can completely prevent
> the outdated standby from being brought up, and can avoid logical data
> loss. According to my quick research, in DRBD, the "dopd (DRBD
> outdate-peer daemon)" plays that role.
> 
> What I'd like to avoid is (B). Though (A) is more serious problem than (B),
> we already have some techniques to decrease the risk of (A). But not
> (B), I think.
> 
> The "wait-forever" might be a straightforward approach against (B). But
> this option prevents transactions from running not only when the
> synchronous standby goes away, but also when the primary is invoked
> first or when the standby is promoted at failover. Since the availability
> of the database service decreases very much, I don't want to use that.
> 
> Keeping transactions waiting in the latter two cases would be required
> to avoid (A), but not (B). So I think that we can relax the "wait-forever"
> option so that it allows not-replicated transactions to complete only in
> those cases. IOW, when we initially start the primary, the backends
> don't wait at all for new standby to connect. And, while new primary is
> running alone after failover, the backends don't wait at all, too. Only
> when replication connection is closed while streaming WAL to sync
> standby, the backends wait until new sync standby has connected and
> replication has been completed. Even in this case, if we want to
> improve the service availability, we have only to make something like
> dopd to STONITH the outdated standby, and then request the primary
> to release the waiting backends. So I think that the interface to
> request that release should be implemented.
> 
> Fortunately, that partial "wait-forever" behavior has already been
> implemented in Simon's patch with the client timeout = 0 (disable).
> If he implements the interface to release the waiting backends,
> I'm OK with his design about when to release the backends for 9.1
> (unless I'm missing something).

Almost-working patch attached for the above feature. Time to stop for
the day. Patch against current repo version.

Current repo version attached here also (v20), which includes all fixes
to all known technical issues, major polishing etc..

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-05T07:13:11Z

On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Almost-working patch attached for the above feature. Time to stop for
> the day. Patch against current repo version.
>
> Current repo version attached here also (v20), which includes all fixes
> to all known technical issues, major polishing etc..

Thanks for the patch. Now the code about the wait list looks very
simpler than before! Here are the comments:

@@ -1337,6 +1352,31 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
<snip>
+		if (walsnd->pid != 0 && walsnd->state == WALSNDSTATE_STREAMING)
+		{
+			sync_priority[i] = walsnd->sync_standby_priority;

This always reports the priority of walsender in CATCHUP state as 0.
I don't think that priority needs to be reported as 0.

When new standby which has the same priority as current sync standby
connects, that new standby can switch to new sync one even though
current one is still running. This happens when the index of WalSnd slot
which new standby uses is ahead of that which current one uses. People
don't expect such an unexpected switchover, I think.

+		/*
+		 * Assume the queue is ordered by LSN
+		 */
+		if (XLByteLT(walsndctl->lsn, proc->waitLSN))
+			return numprocs;

The code to ensure the assumption needs to be added.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T11:04:45Z

On Sat, 2011-03-05 at 16:13 +0900, Fujii Masao wrote:
> On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > Almost-working patch attached for the above feature. Time to stop for
> > the day. Patch against current repo version.
> >
> > Current repo version attached here also (v20), which includes all fixes
> > to all known technical issues, major polishing etc..
> 
> Thanks for the patch. Now the code about the wait list looks very
> simpler than before! Here are the comments:
> 
> @@ -1337,6 +1352,31 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)
> <snip>
> +		if (walsnd->pid != 0 && walsnd->state == WALSNDSTATE_STREAMING)
> +		{
> +			sync_priority[i] = walsnd->sync_standby_priority;
> 
> This always reports the priority of walsender in CATCHUP state as 0.
> I don't think that priority needs to be reported as 0.

Cosmetic change. We can do this, yes.

> When new standby which has the same priority as current sync standby
> connects, that new standby can switch to new sync one even though
> current one is still running. This happens when the index of WalSnd slot
> which new standby uses is ahead of that which current one uses. People
> don't expect such an unexpected switchover, I think.

It is documented that the selection of standby from a set of similar
priorities is indeterminate. Users don't like it, they can change it.

> +		/*
> +		 * Assume the queue is ordered by LSN
> +		 */
> +		if (XLByteLT(walsndctl->lsn, proc->waitLSN))
> +			return numprocs;
> 
> The code to ensure the assumption needs to be added.

Yes, just need to add the code for traversing list backwards.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-05T11:08:42Z

On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Yes, that can happen. As people will no doubt observe, this seems to be
> an argument for wait-forever. What we actually need is a wait that lasts
> longer than it takes for us to decide to failover, if the standby is
> actually up and this is some kind of split brain situation. That way the
> clients are still waiting when failover occurs. WAL is missing, but
> since we didn't acknowledge the client we are OK to treat that situation
> as if it were an abort.

Oracle Data Guard in the maximum availability mode behaves that way?

I'm sure that you are implementing something like the maximum availability
mode rather than the maximum protection one. So I'd like to know how
the data loss situation I described can be avoided in the maximum availability
mode.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T12:21:26Z

On Sat, 2011-03-05 at 11:04 +0000, Simon Riggs wrote:
> > +             /*
> > +              * Assume the queue is ordered by LSN
> > +              */
> > +             if (XLByteLT(walsndctl->lsn, proc->waitLSN))
> > +                     return numprocs;
> > 
> > The code to ensure the assumption needs to be added.
> 
> Yes, just need to add the code for traversing list backwards.

I've added code to shmqueue.c to allow this.

New version pushed.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-05T12:24:46Z

On Sat, Mar 5, 2011 at 6:04 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> It is documented that the selection of standby from a set of similar
> priorities is indeterminate. Users don't like it, they can change it.

That doesn't seem like a good argument to *change* the synchronous
standby once it's already set.

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

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T12:49:46Z

On Sat, 2011-03-05 at 07:24 -0500, Robert Haas wrote:
> On Sat, Mar 5, 2011 at 6:04 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > It is documented that the selection of standby from a set of similar
> > priorities is indeterminate. Users don't like it, they can change it.
> 
> That doesn't seem like a good argument to *change* the synchronous
> standby once it's already set.

If the order is arbitrary, why does it matter if it changes?

The user has the power to specify a sequence, yet they have not done so.
They are told the results are indeterminate, which is accurate. I can
add the words "and may change as new standbys connect" if that helps.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-05T13:05:31Z

On Sat, Mar 5, 2011 at 7:49 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Sat, 2011-03-05 at 07:24 -0500, Robert Haas wrote:
>> On Sat, Mar 5, 2011 at 6:04 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> > It is documented that the selection of standby from a set of similar
>> > priorities is indeterminate. Users don't like it, they can change it.
>>
>> That doesn't seem like a good argument to *change* the synchronous
>> standby once it's already set.
>
> If the order is arbitrary, why does it matter if it changes?
>
> The user has the power to specify a sequence, yet they have not done so.
> They are told the results are indeterminate, which is accurate. I can
> add the words "and may change as new standbys connect" if that helps.

I just don't think that's very useful behavior.  Suppose I have a
master and two standbys.  Both are local (or both are remote with
equally good connectivity).  When one of the standbys goes down, there
will be a hiccup (i.e. transactions will block trying to commit) until
that guy falls off and the other one takes over.  Now, when he comes
back up again, I don't want the synchronous standby to change again;
that seems like a recipe for another hiccup.  I think "who the current
synchronous standby is" should act as a tiebreak.

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

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-05T13:44:26Z

On Sat, Mar 5, 2011 at 2:05 PM, Robert Haas <robertmhaas@gmail.com> wrote:

> On Sat, Mar 5, 2011 at 7:49 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > If the order is arbitrary, why does it matter if it changes?
> >
> > The user has the power to specify a sequence, yet they have not done so.
> > They are told the results are indeterminate, which is accurate. I can
> > add the words "and may change as new standbys connect" if that helps.
>
> I just don't think that's very useful behavior.  Suppose I have a
> master and two standbys.  Both are local (or both are remote with
> equally good connectivity).  When one of the standbys goes down, there
> will be a hiccup (i.e. transactions will block trying to commit) until
> that guy falls off and the other one takes over.  Now, when he comes
> back up again, I don't want the synchronous standby to change again;
> that seems like a recipe for another hiccup.  I think "who the current
> synchronous standby is" should act as a tiebreak.
>

+1

TLDR part:

The first one might be noticed by users because it takes tens of seconds
before the sync switch. The second hiccup is hardly noticable. However
limiting the # switches of sync standby to the absolute minimum is also good
if e.g. (if there was a hook for it) cluster middleware is notified of the
sync replica change. That might either introduce a race condition or be even
completely unreliable if the notify is sent asynchronous, or it might
introduce a longer lag if the master waits for confirmation of the sync
replica change message. At that point sync replica changes become more
expensive than they are currently.

regards,
Yeb Havinga

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T15:07:45Z

On Sat, 2011-03-05 at 14:44 +0100, Yeb Havinga wrote:
> On Sat, Mar 5, 2011 at 2:05 PM, Robert Haas <robertmhaas@gmail.com>
> wrote:
>         On Sat, Mar 5, 2011 at 7:49 AM, Simon Riggs
>         <simon@2ndquadrant.com> wrote:
>         > If the order is arbitrary, why does it matter if it changes?
>         >
>         > The user has the power to specify a sequence, yet they have
>         not done so.
>         > They are told the results are indeterminate, which is
>         accurate. I can
>         > add the words "and may change as new standbys connect" if
>         that helps.
>         
>         
>         I just don't think that's very useful behavior.  Suppose I
>         have a
>         master and two standbys.  Both are local (or both are remote
>         with
>         equally good connectivity).  When one of the standbys goes
>         down, there
>         will be a hiccup (i.e. transactions will block trying to
>         commit) until
>         that guy falls off and the other one takes over.  Now, when he
>         comes
>         back up again, I don't want the synchronous standby to change
>         again;
>         that seems like a recipe for another hiccup.  I think "who the
>         current
>         synchronous standby is" should act as a tiebreak.
> 
> 
> +1
> 
> TLDR part:
> 
> The first one might be noticed by users because it takes tens of
> seconds before the sync switch. The second hiccup is hardly noticable.
> However limiting the # switches of sync standby to the absolute
> minimum is also good if e.g. (if there was a hook for it) cluster
> middleware is notified of the sync replica change. That might either
> introduce a race condition or be even completely unreliable if the
> notify is sent asynchronous, or it might introduce a longer lag if the
> master waits for confirmation of the sync replica change message. At
> that point sync replica changes become more expensive than they are
> currently.

I'm not in favour.

If the user has a preferred order, they can specify it. If there is no
preferred order, how will we maintain that order?

What are the rules for maintaining this arbitrary order?

No, this is not something we need prior to commit, if ever.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T15:26:35Z

On Sat, 2011-03-05 at 20:08 +0900, Fujii Masao wrote:
> On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > Yes, that can happen. As people will no doubt observe, this seems to be
> > an argument for wait-forever. What we actually need is a wait that lasts
> > longer than it takes for us to decide to failover, if the standby is
> > actually up and this is some kind of split brain situation. That way the
> > clients are still waiting when failover occurs. WAL is missing, but
> > since we didn't acknowledge the client we are OK to treat that situation
> > as if it were an abort.
> 
> Oracle Data Guard in the maximum availability mode behaves that way?
> 
> I'm sure that you are implementing something like the maximum availability
> mode rather than the maximum protection one. So I'd like to know how
> the data loss situation I described can be avoided in the maximum availability
> mode.

This is important, so I am taking time to formulate a full reply.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-05T15:42:20Z

On Sat, Mar 5, 2011 at 9:21 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> I've added code to shmqueue.c to allow this.
>
> New version pushed.

New comments;

It looks odd to report the sync_state of walsender in BACKUP
state as ASYNC.

+SyncRepCleanupAtProcExit(int code, Datum arg)
+{
+	if (WaitingForSyncRep && !SHMQueueIsDetached(&(MyProc->syncrep_links)))
+	{
+		LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
+		SHMQueueDelete(&(MyProc->syncrep_links));
+		LWLockRelease(SyncRepLock);
+	}
+
+	if (MyProc != NULL)
+		DisownLatch(&MyProc->waitLatch);

Can MyProc really be NULL here? If yes, "MyProc != NULL" should be
checked before seeing MyProc->syncrep_links.

Even though postmaster dies, the waiting backend keeps waiting until
the timeout expires. Instead, the backends should periodically check
whether postmaster is alive, and then they should exit immediately
if it's not alive, as well as other process does? If the timeout is
disabled, such backends would get stuck infinitely.

Though I commented about the issue related to shutdown, that was
pointless. So change of ProcessInterrupts is not required unless we
find the need again. Sorry for the noise..

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-05T16:17:29Z

On Sun, Mar 6, 2011 at 12:07 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> I'm not in favour.
>
> If the user has a preferred order, they can specify it. If there is no
> preferred order, how will we maintain that order?
>
> What are the rules for maintaining this arbitrary order?

Probably what Robert, Yeb and I think is to leave the current
sync standby in sync mode until either its connection is closed
or higher priority standby connects. No complicated rule is
required.

To do that, how about tracking which standby is currently in
sync mode? Each walsender checks whether its priority is
higher than that of current sync one, and if yes, it takes over.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-05T16:31:12Z

On Mar 5, 2011, at 11:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Sun, Mar 6, 2011 at 12:07 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> I'm not in favour.
>> 
>> If the user has a preferred order, they can specify it. If there is no
>> preferred order, how will we maintain that order?
>> 
>> What are the rules for maintaining this arbitrary order?
> 
> Probably what Robert, Yeb and I think is to leave the current
> sync standby in sync mode until either its connection is closed
> or higher priority standby connects. No complicated rule is
> required.
> 
> To do that, how about tracking which standby is currently in
> sync mode? Each walsender checks whether its priority is
> higher than that of current sync one, and if yes, it takes over.

That is precisely what I would expect to happen, and IMHO quite useful.

...Robert

Re: Sync Rep v19

Jaime Casanova <jaime@2ndquadrant.com> — 2011-03-05T16:42:13Z

El 05/03/2011 11:18, "Fujii Masao" <masao.fujii@gmail.com> escribió:
>
> On Sun, Mar 6, 2011 at 12:07 AM, Simon Riggs <simon@2ndquadrant.com>
wrote:
> > I'm not in favour.
> >
> > If the user has a preferred order, they can specify it. If there is no
> > preferred order, how will we maintain that order?
> >
> > What are the rules for maintaining this arbitrary order?
>
> Probably what Robert, Yeb and I think is to leave the current
> sync standby in sync mode until either its connection is closed
> or higher priority standby connects. No complicated rule is
> required.
>

It's not better to remove the code to manage * in synchronous_standby_names?
Once we do that there is no chance of having 2 standbys with the same
priority.

After all, most of the times the dba will need to change the * for a real
list of names anyway. At least in IMHO

--
Jaime Casanova                www.2ndQuadrant.com

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T16:49:43Z

On Sun, 2011-03-06 at 01:17 +0900, Fujii Masao wrote:
> On Sun, Mar 6, 2011 at 12:07 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > I'm not in favour.
> >
> > If the user has a preferred order, they can specify it. If there is no
> > preferred order, how will we maintain that order?
> >
> > What are the rules for maintaining this arbitrary order?
> 
> Probably what Robert, Yeb and I think is to leave the current
> sync standby in sync mode until either its connection is closed
> or higher priority standby connects. No complicated rule is
> required.

No, it is complex. The code is intentionally stateless, so unless you
have a rule you cannot work out which one is sync standby.

It is much more important to have robust takeover behaviour.

Changing this will require rethinking how that takeover works. And I'm
not doing that for something that is documented as "indeterminate".

If you care about the sequence then set the supplied parameter, which I
have gone to some trouble to provide.

> To do that, how about tracking which standby is currently in
> sync mode? Each walsender checks whether its priority is
> higher than that of current sync one, and if yes, it takes over.
> 
> Regards,
> 

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T16:51:18Z

On Sat, 2011-03-05 at 11:42 -0500, Jaime Casanova wrote:
> El 05/03/2011 11:18, "Fujii Masao" <masao.fujii@gmail.com> escribió:
> >
> > On Sun, Mar 6, 2011 at 12:07 AM, Simon Riggs <simon@2ndquadrant.com>
> wrote:
> > > I'm not in favour.
> > >
> > > If the user has a preferred order, they can specify it. If there
> is no
> > > preferred order, how will we maintain that order?
> > >
> > > What are the rules for maintaining this arbitrary order?
> >
> > Probably what Robert, Yeb and I think is to leave the current
> > sync standby in sync mode until either its connection is closed
> > or higher priority standby connects. No complicated rule is
> > required.
> >
> 
> It's not better to remove the code to manage * in
> synchronous_standby_names? Once we do that there is no chance of
> having 2 standbys with the same priority.

Yes, there is, because we don't do duplicate name checking.

I've changed the default so it is no longer "*" by default, to avoid
complaints.

> After all, most of the times the dba will need to change the * for a
> real list of names anyway. At least in IMHO
> 
> --
> Jaime Casanova                www.2ndQuadrant.com
> 

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T16:53:20Z

On Sat, 2011-03-05 at 20:08 +0900, Fujii Masao wrote:
> On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > Yes, that can happen. As people will no doubt observe, this seems to be
> > an argument for wait-forever. What we actually need is a wait that lasts
> > longer than it takes for us to decide to failover, if the standby is
> > actually up and this is some kind of split brain situation. That way the
> > clients are still waiting when failover occurs. WAL is missing, but
> > since we didn't acknowledge the client we are OK to treat that situation
> > as if it were an abort.
> 
> Oracle Data Guard in the maximum availability mode behaves that way?
> 
> I'm sure that you are implementing something like the maximum availability
> mode rather than the maximum protection one. So I'd like to know how
> the data loss situation I described can be avoided in the maximum availability
> mode.

It can't. (Oracle or otherwise...)

Once we begin waiting for sync rep, if the transaction or backend ends
then other backends will be able to see the changed data. The only way
to prevent that is to shutdown the database to ensure that no readers or
writers have access to that.

Oracle's protection mechanism is to shutdown the primary if there is no
sync standby available. Maximum Protection. Any other mode must
therefore be less than maximum protection, according to Oracle, and me.
"Available" here means one that has not timed out, via parameter.

Shutting down the main server is cool, as long as you failover to one of
the standbys. If there aren't any standbys, or you don't have a
mechanism for switching quickly, you have availability problems.

What shutting down the server doesn't do is keep the data safe for
transactions that were in their commit-wait phase when the disconnect
occurs. That data exists, yet will not have been transferred to the
standby.

>From now, I also say we should wait forever. It is the safest mode and I
want no argument about whether sync rep is safe or not. We can introduce
a more relaxed mode later with high availability for the primary. That
is possible and in some cases desirable.

Now, when we lose last sync standby we have three choices:

 1. reconnect the standby, or wait for a potential standby to catchup

 2. immediate shutdown of master and failover to one of the standbys

 3. reclassify an async standby as a sync standby

More than likely we would attempt to do (1) for a while, then do (2)

This means that when we startup the primary will freeze for a while
until the sync standbys connect. Which is OK, since they try to
reconnect every 5 seconds and we don't plan on shutting down the primary
much anyway.

I've removed the timeout parameter, plus if we begin waiting we wait
until released, forever if that's how long it takes.

The recommendation to use more than one standby remains.

Fast shutdown will wake backends from their latch and there isn't any
changed interrupt behaviour any more.

synchronous_standby_names = '*' is no longer the default

On a positive note this is one less parameter and will improve
performance as well.

All above changes made.

Ready to commit, barring concrete objections to important behaviour.

I will do one final check tomorrow evening then commit.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-05T16:56:19Z

On Sun, 2011-03-06 at 00:42 +0900, Fujii Masao wrote:
> On Sat, Mar 5, 2011 at 9:21 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> > I've added code to shmqueue.c to allow this.
> >
> > New version pushed.
> 
> New comments;

None of the requested changes are in v21, as yet.

> It looks odd to report the sync_state of walsender in BACKUP
> state as ASYNC.

Cool.

> +SyncRepCleanupAtProcExit(int code, Datum arg)
> +{
> +	if (WaitingForSyncRep && !SHMQueueIsDetached(&(MyProc->syncrep_links)))
> +	{
> +		LWLockAcquire(SyncRepLock, LW_EXCLUSIVE);
> +		SHMQueueDelete(&(MyProc->syncrep_links));
> +		LWLockRelease(SyncRepLock);
> +	}
> +
> +	if (MyProc != NULL)
> +		DisownLatch(&MyProc->waitLatch);
> 
> Can MyProc really be NULL here? If yes, "MyProc != NULL" should be
> checked before seeing MyProc->syncrep_links.

OK

> Even though postmaster dies, the waiting backend keeps waiting until
> the timeout expires. Instead, the backends should periodically check
> whether postmaster is alive, and then they should exit immediately
> if it's not alive, as well as other process does? If the timeout is
> disabled, such backends would get stuck infinitely.

Will wake them every 60 seconds

> Though I commented about the issue related to shutdown, that was
> pointless. So change of ProcessInterrupts is not required unless we
> find the need again. Sorry for the noise..

Yep, all gone now.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-05T16:58:31Z

On Sun, Mar 6, 2011 at 12:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> New comments;

Another one;

+	long		timeout = SyncRepGetWaitTimeout();
<snip>
+			else if (timeout > 0 &&
+				TimestampDifferenceExceeds(wait_start, now, timeout))
+			{

The third argument of TimestampDifferenceExceeds is
expressed in milliseconds. But you wrongly give that the
microseconds.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-05T17:25:31Z

On Sat, Mar 5, 2011 at 5:53 PM, Simon Riggs <simon@2ndquadrant.com> wrote:

>
> On a positive note this is one less parameter and will improve
> performance as well.
>
> All above changes made.
>
> Ready to commit, barring concrete objections to important behaviour.
>
> I will do one final check tomorrow evening then commit.
>

Will retest with new version this evening. Also curious to performance
improvement, since v17 seems to be topscorer in that department.

regardd,
Yeb Havinga

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-05T17:59:50Z

On Sat, Mar 5, 2011 at 11:56 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Even though postmaster dies, the waiting backend keeps waiting until
>> the timeout expires. Instead, the backends should periodically check
>> whether postmaster is alive, and then they should exit immediately
>> if it's not alive, as well as other process does? If the timeout is
>> disabled, such backends would get stuck infinitely.
>
> Will wake them every 60 seconds

I don't really see why sync rep should be responsible for solving this
problem, which is an issue in many other situations as well, only for
itself.  In fact I think I'd prefer that it didn't, and that we wait
for a more general solution that will actually fix this problem for
real.

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

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-05T20:11:36Z

On 2011-03-05 18:25, Yeb Havinga wrote:
> On Sat, Mar 5, 2011 at 5:53 PM, Simon Riggs <simon@2ndquadrant.com 
> <mailto:simon@2ndquadrant.com>> wrote:
>
>
>     On a positive note this is one less parameter and will improve
>     performance as well.
>
>     All above changes made.
>
>     Ready to commit, barring concrete objections to important behaviour.
>
>     I will do one final check tomorrow evening then commit.
>
>
> Will retest with new version this evening. Also curious to performance 
> improvement, since v17 seems to be topscorer in that department.
Summary of preliminary testing:
1) it is confusing to show messages/ contents of stat_replication that 
hints at syncrep, when synchronous_replication is on.
2) should guc settings for synchronous_replication be changed so it can 
only be set in the config file, and hence only change with reload_conf()?
3) speed is comparable to v17 :-)

regards,
Yeb Havinga

So the biggest change is perhaps that you cannot start 'working' 
immediately after a initdb with synchronous_replication=on, without a 
connected standby; I needed to create a role for the repuser to make a 
backup, but the master halted. The initial bootstrapping has to be done 
with synchronous_replication = off. So I did, made backup, started 
standby's while still in not synchronous mode. What followed was confusing:

LOG:  00000: standby "standby2" is now the synchronous standby with 
priority 2

postgres=# show synchronous_replication ; show 
synchronous_standby_names; select application_name,state,sync_state from 
pg_stat_replication;
  synchronous_replication
-------------------------
  off
(1 row)
  synchronous_standby_names
----------------------------
  standby1,standby2,standby3
(1 row)
  application_name |   state   | sync_state
------------------+-----------+------------
  standby2         | STREAMING | SYNC
  asyncone         | STREAMING | ASYNC
(2 rows)

Is it really sync?
pgbench test got 1464 tps.. seems a bit high.

psql postgres, set synchronous_replication = on;
- no errors, and show after disconnect showed this parameter was still 
on. My guess: we have syncrep! A restart or reload config was not necessary.
pgbench test got 1460 tps.

pg_reload_conf(); with syncrep = on in postgresql.conf
pgbench test got 819 tps

So now this is synchronous.
Disabled the asynchronous standby
pgbench test got 920 tps.

I also got a first first > 1000 tps score :-) (yeah you have to believe 
me there really was a sync standby server)

$ pgbench -c 10 -M prepared -T 30 test
starting vacuum...end.
transaction type: TPC-B (sort of)
scaling factor: 50
query mode: prepared
number of clients: 10
number of threads: 1
duration: 30 s
number of transactions actually processed: 30863
tps = 1027.493807 (including connections establishing)
tps = 1028.183618 (excluding connections establishing)

Re: Sync Rep v19

Yeb Havinga <yebhavinga@gmail.com> — 2011-03-05T20:17:56Z

On 2011-03-05 21:11, Yeb Havinga wrote:
> Summary of preliminary testing:
> 1) it is confusing to show messages/ contents of stat_replication that 
> hints at syncrep, when synchronous_replication is on.
s/on/off/

Also forgot to mention these tests are againt the latest v21 syncrep patch.

Y

Re: Sync Rep v19

Jaime Casanova <jaime@2ndquadrant.com> — 2011-03-05T22:10:29Z

On Sat, Mar 5, 2011 at 3:11 PM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>
> Summary of preliminary testing:
> 1) it is confusing to show messages/ contents of stat_replication that hints
> at syncrep, when synchronous_replication is on.

[for the record, Yeb explain he means OFF not on...]

the thing is that once you put a server name in
synchronous_standby_names that standby is declared to be synchronous
and because synchronous_replication can change at any time for any
given backend we need to know priority of any declared sync standby.
if you want pure async standbys remove their names from
synchronous_standby_names

> 2) should guc settings for synchronous_replication be changed so it can only
> be set in the config file, and hence only change with reload_conf()?

no. the thing is that we can determine for which data we want to pay
the price of synchrony and for which data don't

> 3) speed is comparable to v17 :-)
>

yeah... it's a lot better than before, good work Simon :)

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-06T05:27:49Z

On Sun, Mar 6, 2011 at 2:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, Mar 5, 2011 at 11:56 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>>> Even though postmaster dies, the waiting backend keeps waiting until
>>> the timeout expires. Instead, the backends should periodically check
>>> whether postmaster is alive, and then they should exit immediately
>>> if it's not alive, as well as other process does? If the timeout is
>>> disabled, such backends would get stuck infinitely.
>>
>> Will wake them every 60 seconds
>
> I don't really see why sync rep should be responsible for solving this
> problem, which is an issue in many other situations as well, only for
> itself. In fact I think I'd prefer that it didn't, and that we wait
> for a more general solution that will actually fix this problem for
> real.

I agree if such a general solution will be committed together with sync rep.
Otherwise, because of sync rep, the backend can easily get stuck *infinitely*.
When postmaster is not alive, all the existing walsenders exit immediately
and no new walsender can appear. So there is no way to release the
waiting backend. I think that some solutions for this issue which is likely to
happen are required.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-06T07:51:05Z

On Sun, Mar 6, 2011 at 1:53 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Sat, 2011-03-05 at 20:08 +0900, Fujii Masao wrote:
>> On Sat, Mar 5, 2011 at 7:28 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> > Yes, that can happen. As people will no doubt observe, this seems to be
>> > an argument for wait-forever. What we actually need is a wait that lasts
>> > longer than it takes for us to decide to failover, if the standby is
>> > actually up and this is some kind of split brain situation. That way the
>> > clients are still waiting when failover occurs. WAL is missing, but
>> > since we didn't acknowledge the client we are OK to treat that situation
>> > as if it were an abort.
>>
>> Oracle Data Guard in the maximum availability mode behaves that way?
>>
>> I'm sure that you are implementing something like the maximum availability
>> mode rather than the maximum protection one. So I'd like to know how
>> the data loss situation I described can be avoided in the maximum availability
>> mode.
>
> It can't. (Oracle or otherwise...)
>
> Once we begin waiting for sync rep, if the transaction or backend ends
> then other backends will be able to see the changed data. The only way
> to prevent that is to shutdown the database to ensure that no readers or
> writers have access to that.
>
> Oracle's protection mechanism is to shutdown the primary if there is no
> sync standby available. Maximum Protection. Any other mode must
> therefore be less than maximum protection, according to Oracle, and me.
> "Available" here means one that has not timed out, via parameter.
>
> Shutting down the main server is cool, as long as you failover to one of
> the standbys. If there aren't any standbys, or you don't have a
> mechanism for switching quickly, you have availability problems.
>
> What shutting down the server doesn't do is keep the data safe for
> transactions that were in their commit-wait phase when the disconnect
> occurs. That data exists, yet will not have been transferred to the
> standby.
>
> >From now, I also say we should wait forever. It is the safest mode and I
> want no argument about whether sync rep is safe or not. We can introduce
> a more relaxed mode later with high availability for the primary. That
> is possible and in some cases desirable.
>
> Now, when we lose last sync standby we have three choices:
>
>  1. reconnect the standby, or wait for a potential standby to catchup
>
>  2. immediate shutdown of master and failover to one of the standbys
>
>  3. reclassify an async standby as a sync standby
>
> More than likely we would attempt to do (1) for a while, then do (2)
>
> This means that when we startup the primary will freeze for a while
> until the sync standbys connect. Which is OK, since they try to
> reconnect every 5 seconds and we don't plan on shutting down the primary
> much anyway.
>
> I've removed the timeout parameter, plus if we begin waiting we wait
> until released, forever if that's how long it takes.
>
> The recommendation to use more than one standby remains.
>
> Fast shutdown will wake backends from their latch and there isn't any
> changed interrupt behaviour any more.
>
> synchronous_standby_names = '*' is no longer the default
>
> On a positive note this is one less parameter and will improve
> performance as well.
>
> All above changes made.
>
> Ready to commit, barring concrete objections to important behaviour.
>
> I will do one final check tomorrow evening then commit.

I agree with this change.

One comment; what about introducing built-in function to wake up all the
waiting backends? When replication connection is closed, if we STONITH
the standby, we can safely (for not physical data loss but logical one)
switch the primary to standalone mode. But there is no way to wake up
the waiting backends for now. Setting synchronous_replication to OFF
and reloading the configuration file doesn't affect the existing waiting
backends. The attached patch introduces the "pg_wakeup_all_waiters"
(better name?) function which wakes up all the backends on the queue.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-06T07:58:19Z

On Sun, Mar 6, 2011 at 4:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> One comment; what about introducing built-in function to wake up all the
> waiting backends? When replication connection is closed, if we STONITH
> the standby, we can safely (for not physical data loss but logical one)
> switch the primary to standalone mode. But there is no way to wake up
> the waiting backends for now. Setting synchronous_replication to OFF
> and reloading the configuration file doesn't affect the existing waiting
> backends. The attached patch introduces the "pg_wakeup_all_waiters"
> (better name?) function which wakes up all the backends on the queue.

If unfortunately all connection slots are used by backends waiting for
replication, we cannot execute such a function. So it makes more sense
to introduce something like "pg_ctl standalone" command?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-06T08:21:13Z

On Sun, 2011-03-06 at 14:27 +0900, Fujii Masao wrote:
> On Sun, Mar 6, 2011 at 2:59 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> > On Sat, Mar 5, 2011 at 11:56 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
> >>> Even though postmaster dies, the waiting backend keeps waiting until
> >>> the timeout expires. Instead, the backends should periodically check
> >>> whether postmaster is alive, and then they should exit immediately
> >>> if it's not alive, as well as other process does? If the timeout is
> >>> disabled, such backends would get stuck infinitely.
> >>
> >> Will wake them every 60 seconds
> >
> > I don't really see why sync rep should be responsible for solving this
> > problem, which is an issue in many other situations as well, only for
> > itself. In fact I think I'd prefer that it didn't, and that we wait
> > for a more general solution that will actually fix this problem for
> > real.
> 
> I agree if such a general solution will be committed together with sync rep.
> Otherwise, because of sync rep, the backend can easily get stuck *infinitely*.
> When postmaster is not alive, all the existing walsenders exit immediately
> and no new walsender can appear. So there is no way to release the
> waiting backend. I think that some solutions for this issue which is likely to
> happen are required.

Completely agree.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-06T08:26:18Z

On Sun, 2011-03-06 at 16:58 +0900, Fujii Masao wrote:
> On Sun, Mar 6, 2011 at 4:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > One comment; what about introducing built-in function to wake up all the
> > waiting backends? When replication connection is closed, if we STONITH
> > the standby, we can safely (for not physical data loss but logical one)
> > switch the primary to standalone mode. But there is no way to wake up
> > the waiting backends for now. Setting synchronous_replication to OFF
> > and reloading the configuration file doesn't affect the existing waiting
> > backends. The attached patch introduces the "pg_wakeup_all_waiters"
> > (better name?) function which wakes up all the backends on the queue.
> 
> If unfortunately all connection slots are used by backends waiting for
> replication, we cannot execute such a function. So it makes more sense
> to introduce something like "pg_ctl standalone" command?

Well, there is one way to end the wait: shutdown, or use
pg_terminate_backend().

If you simply end the wait you will get COMMIT messages.

What I would like to do is commit the "safe" patch now. We can then
discuss whether it is safe and desirable to relax some aspects of that
during beta.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-06T08:27:01Z

On Sun, 2011-03-06 at 01:58 +0900, Fujii Masao wrote:
> On Sun, Mar 6, 2011 at 12:42 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > New comments;
> 
> Another one;
> 
> +	long		timeout = SyncRepGetWaitTimeout();
> <snip>
> +			else if (timeout > 0 &&
> +				TimestampDifferenceExceeds(wait_start, now, timeout))
> +			{
> 
> The third argument of TimestampDifferenceExceeds is
> expressed in milliseconds. But you wrongly give that the
> microseconds.

Just for the record: that code section is now removed in v21

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-06T14:26:22Z

On Sun, Mar 6, 2011 at 5:26 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Sun, 2011-03-06 at 16:58 +0900, Fujii Masao wrote:
>> On Sun, Mar 6, 2011 at 4:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> > One comment; what about introducing built-in function to wake up all the
>> > waiting backends? When replication connection is closed, if we STONITH
>> > the standby, we can safely (for not physical data loss but logical one)
>> > switch the primary to standalone mode. But there is no way to wake up
>> > the waiting backends for now. Setting synchronous_replication to OFF
>> > and reloading the configuration file doesn't affect the existing waiting
>> > backends. The attached patch introduces the "pg_wakeup_all_waiters"
>> > (better name?) function which wakes up all the backends on the queue.
>>
>> If unfortunately all connection slots are used by backends waiting for
>> replication, we cannot execute such a function. So it makes more sense
>> to introduce something like "pg_ctl standalone" command?
>
> Well, there is one way to end the wait: shutdown, or use
> pg_terminate_backend().

Immediate shutdown can release the wait. But smart and fast shutdown
cannot. Also pg_terminate_backend() cannot. Since a backend is waiting
on the latch and InterruptHoldoffCount is not zero, only SetLatch() or
SIGQUIT can cause it to end.

> If you simply end the wait you will get COMMIT messages.
>
> What I would like to do is commit the "safe" patch now. We can then
> discuss whether it is safe and desirable to relax some aspects of that
> during beta.

OK if changing some aspects is acceptable during beta.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-06T14:44:58Z

On Sun, Mar 6, 2011 at 5:02 PM, Yeb Havinga <yebhavinga@gmail.com> wrote:
> On Sun, Mar 6, 2011 at 8:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>
>> If unfortunately all connection slots are used by backends waiting for
>> replication, we cannot execute such a function. So it makes more sense
>> to introduce something like "pg_ctl standalone" command?
>
> If it is only for shutdown, maybe pg_ctl stop -m standalone?

It's for not only shutdown but also running the primary in standalone mode.
So something like "pg_ctl standalone" is better.

For now I think that pg_ctl command is better than built-in function because
sometimes we might want to wake waiters up even during shutdown in
order to cause shutdown to end. During shutdown, the server doesn't
accept any new connection (even from the standby). So, without something
like "pg_ctl standalone", there is no way to cause shutdown to end.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Jaime Casanova <jaime@2ndquadrant.com> — 2011-03-06T16:14:33Z

El 06/03/2011 03:26, "Simon Riggs" <simon@2ndquadrant.com> escribió:
>
> On Sun, 2011-03-06 at 16:58 +0900, Fujii Masao wrote:

> > If unfortunately all connection slots are used by backends waiting for
> > replication, we cannot execute such a function. So it makes more sense
> > to introduce something like "pg_ctl standalone" command?
>
> Well, there is one way to end the wait: shutdown, or use
> pg_terminate_backend().
>

I disconnected all standbys so the master keeps waiting on commit. Then i
shutdown the master with immediate and got a crash. i wasn't able to trace
that though

--
Jaime Casanova                www.2ndQuadrant.com

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-06T19:54:28Z

On Mar 6, 2011, at 9:44 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Sun, Mar 6, 2011 at 5:02 PM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>> On Sun, Mar 6, 2011 at 8:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> 
>>> If unfortunately all connection slots are used by backends waiting for
>>> replication, we cannot execute such a function. So it makes more sense
>>> to introduce something like "pg_ctl standalone" command?
>> 
>> If it is only for shutdown, maybe pg_ctl stop -m standalone?
> 
> It's for not only shutdown but also running the primary in standalone mode.
> So something like "pg_ctl standalone" is better.
> 
> For now I think that pg_ctl command is better than built-in function because
> sometimes we might want to wake waiters up even during shutdown in
> order to cause shutdown to end. During shutdown, the server doesn't
> accept any new connection (even from the standby). So, without something
> like "pg_ctl standalone", there is no way to cause shutdown to end.

This sounds like an awful hack to work around a bad design. Surely once shutdown reaches a point where new replication connections can no longer be accepted, any standbys hung on commit need to close the connection without responding to the COMMIT, per previous discussion.  It's completely unreasonable for sync rep to break the shutdown sequence.

...Robert

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-06T21:33:38Z

On Sun, 2011-03-06 at 16:51 +0900, Fujii Masao wrote:

> One comment; what about introducing built-in function to wake up all the
> waiting backends? When replication connection is closed, if we STONITH
> the standby, we can safely (for not physical data loss but logical one)
> switch the primary to standalone mode. But there is no way to wake up
> the waiting backends for now. Setting synchronous_replication to OFF
> and reloading the configuration file doesn't affect the existing waiting
> backends. The attached patch introduces the "pg_wakeup_all_waiters"
> (better name?) function which wakes up all the backends on the queue.

Will apply this as a separate commit.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-07T00:37:04Z

On Sat, 2011-03-05 at 21:11 +0100, Yeb Havinga wrote:

> I also got a first first > 1000 tps score

The committed version should be even faster. Would appreciate a retest.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-07T13:33:38Z

On Mon, 2011-03-07 at 14:20 +0100, Yeb Havinga wrote:
> On 2011-03-07 01:37, Simon Riggs wrote:
> > On Sat, 2011-03-05 at 21:11 +0100, Yeb Havinga wrote:
> >
> >> I also got a first first>  1000 tps score
> > The committed version should be even faster. Would appreciate a retest.
> >
> pgbench 5 minute test pgbench -c 10 -M prepared -T 300 test
> dbsize was -s 50, 1Gbit Ethernet
> 
> 1 async standby
> tps = 2475.285931 (excluding connections establishing)
> 
> 2 async standbys
> tps = 2333.670561 (excluding connections establishing)
> 
> 1 sync standby
> tps = 1277.082753 (excluding connections establishing)
> 
> 1 sync, 1 async standby
> tps = 1273.317386 (excluding connections establishing)
> 
> Hard for me to not revert to superlatives right now! :-)

That looks like good news, thanks.

It shows that sync rep is "fairly fast", but it also shows clearly why
you'd want to mix sync and async replication within an application.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-08T12:05:08Z

On Mon, Mar 7, 2011 at 4:54 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mar 6, 2011, at 9:44 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Sun, Mar 6, 2011 at 5:02 PM, Yeb Havinga <yebhavinga@gmail.com> wrote:
>>> On Sun, Mar 6, 2011 at 8:58 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>
>>>> If unfortunately all connection slots are used by backends waiting for
>>>> replication, we cannot execute such a function. So it makes more sense
>>>> to introduce something like "pg_ctl standalone" command?
>>>
>>> If it is only for shutdown, maybe pg_ctl stop -m standalone?
>>
>> It's for not only shutdown but also running the primary in standalone mode.
>> So something like "pg_ctl standalone" is better.
>>
>> For now I think that pg_ctl command is better than built-in function because
>> sometimes we might want to wake waiters up even during shutdown in
>> order to cause shutdown to end. During shutdown, the server doesn't
>> accept any new connection (even from the standby). So, without something
>> like "pg_ctl standalone", there is no way to cause shutdown to end.
>
> This sounds like an awful hack to work around a bad design. Surely once shutdown reaches a point where new replication connections can no longer be accepted, any standbys hung on commit need to close the connection without responding to the COMMIT, per previous discussion.  It's completely unreasonable for sync rep to break the shutdown sequence.

Yeah, let's think about how shutdown should work. I'd like to propose the
following. Thought?

* Smart shutdown
Smart shutdown should wait for all the waiting backends to be acked, and
should not cause them to forcibly exit. But this leads shutdown to get stuck
infinitely if there is no walsender at that time. To enable them to be acked
even in that situation, we need to change postmaster so that it accepts the
replication connection even during smart shutdown (until we reach
PM_SHUTDOWN_2 state). Postmaster has already accepted the superuser
connection to cancel backup during smart shutdown. So I don't think that
the idea to accept the replication connection during smart shutdown is so
ugly.

* Fast shutdown
I agree with you about fast shutdown. Fast shutdown should cause all the
backends including waiting ones to exit immediately. At that time, the
non-acked backend should not return the success, according to the
definition of sync rep. So we need to change a backend so that it gets rid
of itself from the waiting queue and exits before returning the success,
when it receives SIGTERM. This change leads the waiting backends to
do the same even when pg_terminate_backend is called. But since
they've not been acked yet, it seems to be reasonable to prevent them
from returning the COMMIT.

Comments? I'll create the patch barring objection.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-08T16:58:35Z

On Tue, Mar 8, 2011 at 7:05 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> Yeah, let's think about how shutdown should work. I'd like to propose the
> following. Thought?
>
> * Smart shutdown
> Smart shutdown should wait for all the waiting backends to be acked, and
> should not cause them to forcibly exit. But this leads shutdown to get stuck
> infinitely if there is no walsender at that time. To enable them to be acked
> even in that situation, we need to change postmaster so that it accepts the
> replication connection even during smart shutdown (until we reach
> PM_SHUTDOWN_2 state). Postmaster has already accepted the superuser
> connection to cancel backup during smart shutdown. So I don't think that
> the idea to accept the replication connection during smart shutdown is so
> ugly.
>
> * Fast shutdown
> I agree with you about fast shutdown. Fast shutdown should cause all the
> backends including waiting ones to exit immediately. At that time, the
> non-acked backend should not return the success, according to the
> definition of sync rep. So we need to change a backend so that it gets rid
> of itself from the waiting queue and exits before returning the success,
> when it receives SIGTERM. This change leads the waiting backends to
> do the same even when pg_terminate_backend is called. But since
> they've not been acked yet, it seems to be reasonable to prevent them
> from returning the COMMIT.

The fast shutdown handling seems fine, but why not just handle smart
shutdown the same way?  I don't really like the idea of allowing
replication connections for longer, and the idea that we don't want to
keep waiting for a commit ACK once we're past the point where it's
possible for one to occur seems to apply generically to any shutdown
sequence.

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

Re: Sync Rep v19

Jaime Casanova <jaime@2ndquadrant.com> — 2011-03-09T05:14:01Z

On Tue, Mar 8, 2011 at 11:58 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> The fast shutdown handling seems fine, but why not just handle smart
> shutdown the same way?

currently, smart shutdown means no new connections, wait until
existing ones close normally. for consistency, it should behave the
same for sync rep.

+1 for Fujii's proposal

-- 
Jaime Casanova         www.2ndQuadrant.com
Professional PostgreSQL: Soporte y capacitación de PostgreSQL

Re: Sync Rep v19

Bruce Momjian <bruce@momjian.us> — 2011-03-10T02:21:25Z

Simon Riggs wrote:
> On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:
> 
> > postgres=# SELECT application_name, state, sync_priority, sync_state
> > FROM pg_stat_replication;
> >  application_name |   state   | sync_priority | sync_state
> > ------------------+-----------+---------------+------------
> >  one              | STREAMING |             1 | POTENTIAL
> >  two              | STREAMING |             2 | SYNC
> > (2 rows)
> 
> Bug! Thanks.

Is there a reason these status are all upper-case?

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-10T08:03:51Z

On Wed, 2011-03-09 at 21:21 -0500, Bruce Momjian wrote:
> Simon Riggs wrote:
> > On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:
> > 
> > > postgres=# SELECT application_name, state, sync_priority, sync_state
> > > FROM pg_stat_replication;
> > >  application_name |   state   | sync_priority | sync_state
> > > ------------------+-----------+---------------+------------
> > >  one              | STREAMING |             1 | POTENTIAL
> > >  two              | streaming |             2 | sync
> > > (2 rows)
> > 
> > Bug! Thanks.
> 
> Is there a reason these status are all upper-case?

NOT AS FAR AS I KNOW.

I'll add it to the list of changes for beta.

-- 
 Simon Riggs           http://www.2ndQuadrant.com/books/
 PostgreSQL Development, 24x7 Support, Training and Services

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-10T15:33:04Z

On Wed, Mar 9, 2011 at 9:21 PM, Bruce Momjian <bruce@momjian.us> wrote:
> Simon Riggs wrote:
>> On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:
>>
>> > postgres=# SELECT application_name, state, sync_priority, sync_state
>> > FROM pg_stat_replication;
>> >  application_name |   state   | sync_priority | sync_state
>> > ------------------+-----------+---------------+------------
>> >  one              | STREAMING |             1 | POTENTIAL
>> >  two              | STREAMING |             2 | SYNC
>> > (2 rows)
>>
>> Bug! Thanks.
>
> Is there a reason these status are all upper-case?

Not that I know of.

However, I think that some more fundamental rethinking of the "state"
mechanism may be in order.  When Magnus first committed this, it would
say CATCHUP whenever you were behind (even if only momentarily) and
STREAMING if you were caught up.  Simon then changed it so that it
says CATCHUP until you catch up the first time, and then STREAMING
afterward (even if you fall behind again).  Neither behavior seems
completely adequate to me.  I think we should have a way to know
whether we've ever been caught up, and if so when the most recent time
was.  So you could then say things like "is the most recent time at
which the standby was caught up within the last 30 seconds?", which
would be a useful thing to monitor, and right now there's no way to do
it.  There's also a BACKUP state, but I'm not sure it makes sense to
lump that in with the others.  Some day it might be possible to stream
WAL and take a backup at the same time, over the same connection.
Maybe that should be a separate column or something.

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

Re: Sync Rep v19

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-03-10T19:42:10Z

Robert Haas <robertmhaas@gmail.com> writes:
> was.  So you could then say things like "is the most recent time at
> which the standby was caught up within the last 30 seconds?", which
> would be a useful thing to monitor, and right now there's no way to do

Well in my experience with replication, that's not what I want to
monitor.  If the standby is synchronous, then it's not catching up, it's
streaming.  If it were not, it would not be a synchronous standby.

When a standby is asynchronous then what I want to monitor is its lag.

So the CATCHUP state is useful to see that a synchronous standby
candidate can not yet be a synchronous standby.  When it just lost its
synchronous status (and hopefully another standby is now the sync one),
then it's just asynchronous and I want to know its lag.

> it.  There's also a BACKUP state, but I'm not sure it makes sense to
> lump that in with the others.  Some day it might be possible to stream
> WAL and take a backup at the same time, over the same connection.
> Maybe that should be a separate column or something.

BACKUP is still meaningful if you stream WAL at the same time, because
you're certainly *not* applying them while doing the base backup, are
you?  So you're not yet a standby, that's what BACKUP means.

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-10T19:56:44Z

On Thu, Mar 10, 2011 at 2:42 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> was.  So you could then say things like "is the most recent time at
>> which the standby was caught up within the last 30 seconds?", which
>> would be a useful thing to monitor, and right now there's no way to do
>
> Well in my experience with replication, that's not what I want to
> monitor.  If the standby is synchronous, then it's not catching up, it's
> streaming.  If it were not, it would not be a synchronous standby.
>
> When a standby is asynchronous then what I want to monitor is its lag.
>
> So the CATCHUP state is useful to see that a synchronous standby
> candidate can not yet be a synchronous standby.  When it just lost its
> synchronous status (and hopefully another standby is now the sync one),
> then it's just asynchronous and I want to know its lag.

Yeah, maybe.  The trick is how to measure the lag.  I proposed the
above scheme mostly as a way of giving the user some piece of
information that can be measured in seconds rather than WAL position,
but I'm open to better ideas.  Monitoring is pretty hard to do at all
in 9.0; in 9.1, we'll be able to tell them how many *bytes* behind
they are, but there's no easy way to figure out what that means in
terms of wall-clock time, which I think would be useful.

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

Re: Sync Rep v19

Dimitri Fontaine <dimitri@2ndquadrant.fr> — 2011-03-10T20:29:10Z

Robert Haas <robertmhaas@gmail.com> writes:
> they are, but there's no easy way to figure out what that means in
> terms of wall-clock time, which I think would be useful.

Jan Wieck had a detailed proposal to make that happen at last developper
meeting, but then ran out of time to implement it for 9.1 it seems.  The
idea was basically to have a ticker in core, an SRF that would associate
txid_snapshot with wall clock time.  Lots of good things would come from
that.

  http://archives.postgresql.org/pgsql-hackers/2010-05/msg01209.php

Of course if you think that's important enough for you to implement it
between now and beta, that would be great :)

Regards,
-- 
Dimitri Fontaine
http://2ndQuadrant.fr     PostgreSQL : Expertise, Formation et Support

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-10T20:50:48Z

On Thu, Mar 10, 2011 at 3:29 PM, Dimitri Fontaine
<dimitri@2ndquadrant.fr> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> they are, but there's no easy way to figure out what that means in
>> terms of wall-clock time, which I think would be useful.
>
> Jan Wieck had a detailed proposal to make that happen at last developper
> meeting, but then ran out of time to implement it for 9.1 it seems.  The
> idea was basically to have a ticker in core, an SRF that would associate
> txid_snapshot with wall clock time.  Lots of good things would come from
> that.
>
>  http://archives.postgresql.org/pgsql-hackers/2010-05/msg01209.php
>
> Of course if you think that's important enough for you to implement it
> between now and beta, that would be great :)

I think that's actually something a little different, and more
complicated, but I do think it'd be useful.  I was hoping there was a
simple way to get some kind of time-based information into
pg_stat_replication, but if there isn't, there isn't.

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

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-11T12:08:09Z

On Fri, Mar 11, 2011 at 5:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 10, 2011 at 3:29 PM, Dimitri Fontaine
> <dimitri@2ndquadrant.fr> wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>> they are, but there's no easy way to figure out what that means in
>>> terms of wall-clock time, which I think would be useful.
>>
>> Jan Wieck had a detailed proposal to make that happen at last developper
>> meeting, but then ran out of time to implement it for 9.1 it seems.  The
>> idea was basically to have a ticker in core, an SRF that would associate
>> txid_snapshot with wall clock time.  Lots of good things would come from
>> that.
>>
>>  http://archives.postgresql.org/pgsql-hackers/2010-05/msg01209.php
>>
>> Of course if you think that's important enough for you to implement it
>> between now and beta, that would be great :)
>
> I think that's actually something a little different, and more
> complicated, but I do think it'd be useful.  I was hoping there was a
> simple way to get some kind of time-based information into
> pg_stat_replication, but if there isn't, there isn't.

How about sending the timestamp of last applied transaction
(i.e., this is the return value of pg_last_xact_replay_timestamp)
from the standby to the master, and reporting it in
pg_stat_replication? Then you can see the lag by comparing
it with current_timestamp.

But since the last replay timestamp doesn't advance (but
current timestamp advances) if there is no work on the master,
the calculated lag might be unexpectedly too large. So, to
calculate the exact lag, I'm thinking that we should introduce
new function which returns the timestamp of the last transaction
written in the master.

Thought?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-11T13:02:25Z

On Fri, Mar 11, 2011 at 7:08 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Mar 11, 2011 at 5:50 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Thu, Mar 10, 2011 at 3:29 PM, Dimitri Fontaine
>> <dimitri@2ndquadrant.fr> wrote:
>>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> they are, but there's no easy way to figure out what that means in
>>>> terms of wall-clock time, which I think would be useful.
>>>
>>> Jan Wieck had a detailed proposal to make that happen at last developper
>>> meeting, but then ran out of time to implement it for 9.1 it seems.  The
>>> idea was basically to have a ticker in core, an SRF that would associate
>>> txid_snapshot with wall clock time.  Lots of good things would come from
>>> that.
>>>
>>>  http://archives.postgresql.org/pgsql-hackers/2010-05/msg01209.php
>>>
>>> Of course if you think that's important enough for you to implement it
>>> between now and beta, that would be great :)
>>
>> I think that's actually something a little different, and more
>> complicated, but I do think it'd be useful.  I was hoping there was a
>> simple way to get some kind of time-based information into
>> pg_stat_replication, but if there isn't, there isn't.
>
> How about sending the timestamp of last applied transaction
> (i.e., this is the return value of pg_last_xact_replay_timestamp)
> from the standby to the master, and reporting it in
> pg_stat_replication? Then you can see the lag by comparing
> it with current_timestamp.
>
> But since the last replay timestamp doesn't advance (but
> current timestamp advances) if there is no work on the master,
> the calculated lag might be unexpectedly too large. So, to
> calculate the exact lag, I'm thinking that we should introduce
> new function which returns the timestamp of the last transaction
> written in the master.
>
> Thought?

Hmm... where would we get that value from?  And what if no
transactions are running on the master?

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

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-11T13:21:52Z

On Fri, Mar 11, 2011 at 10:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> How about sending the timestamp of last applied transaction
>> (i.e., this is the return value of pg_last_xact_replay_timestamp)
>> from the standby to the master, and reporting it in
>> pg_stat_replication? Then you can see the lag by comparing
>> it with current_timestamp.
>>
>> But since the last replay timestamp doesn't advance (but
>> current timestamp advances) if there is no work on the master,
>> the calculated lag might be unexpectedly too large. So, to
>> calculate the exact lag, I'm thinking that we should introduce
>> new function which returns the timestamp of the last transaction
>> written in the master.
>>
>> Thought?
>
> Hmm... where would we get that value from?

xl_xact_commit->xact_time (which is set in RecordTransactionCommit)
and xl_xact_abort->xact_time (which is set in RecordTransactionAbort).

> And what if no
> transactions are running on the master?

In that case, the last write WAL timestamp would become equal to the
last replay WAL timestamp. So we can see that there is no lag.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-11T14:03:33Z

On Fri, Mar 11, 2011 at 8:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Fri, Mar 11, 2011 at 10:02 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>>> How about sending the timestamp of last applied transaction
>>> (i.e., this is the return value of pg_last_xact_replay_timestamp)
>>> from the standby to the master, and reporting it in
>>> pg_stat_replication? Then you can see the lag by comparing
>>> it with current_timestamp.
>>>
>>> But since the last replay timestamp doesn't advance (but
>>> current timestamp advances) if there is no work on the master,
>>> the calculated lag might be unexpectedly too large. So, to
>>> calculate the exact lag, I'm thinking that we should introduce
>>> new function which returns the timestamp of the last transaction
>>> written in the master.
>>>
>>> Thought?
>>
>> Hmm... where would we get that value from?
>
> xl_xact_commit->xact_time (which is set in RecordTransactionCommit)
> and xl_xact_abort->xact_time (which is set in RecordTransactionAbort).
>
>> And what if no
>> transactions are running on the master?
>
> In that case, the last write WAL timestamp would become equal to the
> last replay WAL timestamp. So we can see that there is no lag.

Oh, I see (I think).  You're talking about write/replay lag, but I was
thinking of master/slave transmission lag.

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

Re: Sync Rep v19

Ross Reedstrom <reedstrm@rice.edu> — 2011-03-11T19:42:59Z

On Fri, Mar 11, 2011 at 09:03:33AM -0500, Robert Haas wrote:
> On Fri, Mar 11, 2011 at 8:21 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > In that case, the last write WAL timestamp would become equal to the
> > last replay WAL timestamp. So we can see that there is no lag.
> 
> Oh, I see (I think).  You're talking about write/replay lag, but I was
> thinking of master/slave transmission lag.
> 

Which are both useful numbers to know: the first tells you how "stale"
queries from a Hot Standby will be, the second tells you the maximum
data loss from a "meteor hits the master" scenario where that slave is
promoted, if I understand all the interactions correctly.

Ross
-- 
Ross Reedstrom, Ph.D.                                 reedstrm@rice.edu
Systems Engineer & Admin, Research Scientist        phone: 713-348-6166
Connexions                  http://cnx.org            fax: 713-348-3665
Rice University MS-375, Houston, TX 77005
GPG Key fingerprint = F023 82C8 9B0E 2CC6 0D8E  F888 D3AE 810E 88F0 BEDE

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-19T02:25:10Z

On Tue, Mar 8, 2011 at 7:05 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> * Smart shutdown
> Smart shutdown should wait for all the waiting backends to be acked, and
> should not cause them to forcibly exit. But this leads shutdown to get stuck
> infinitely if there is no walsender at that time. To enable them to be acked
> even in that situation, we need to change postmaster so that it accepts the
> replication connection even during smart shutdown (until we reach
> PM_SHUTDOWN_2 state). Postmaster has already accepted the superuser
> connection to cancel backup during smart shutdown. So I don't think that
> the idea to accept the replication connection during smart shutdown is so
> ugly.
>
> * Fast shutdown
> I agree with you about fast shutdown. Fast shutdown should cause all the
> backends including waiting ones to exit immediately. At that time, the
> non-acked backend should not return the success, according to the
> definition of sync rep. So we need to change a backend so that it gets rid
> of itself from the waiting queue and exits before returning the success,
> when it receives SIGTERM. This change leads the waiting backends to
> do the same even when pg_terminate_backend is called. But since
> they've not been acked yet, it seems to be reasonable to prevent them
> from returning the COMMIT.
>
> Comments? I'll create the patch barring objection.

The fast smart shutdown part of this problem has been addressed.  The
smart shutdown case still needs work, and I think the consensus was
that your proposal above was the best way to go with it.

Do you still want to work up a patch for this?  If so, I can review.

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

Re: Sync Rep v19

Robert Haas <robertmhaas@gmail.com> — 2011-03-19T02:28:33Z

On Fri, Mar 18, 2011 at 10:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Mar 8, 2011 at 7:05 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> * Smart shutdown
>> Smart shutdown should wait for all the waiting backends to be acked, and
>> should not cause them to forcibly exit. But this leads shutdown to get stuck
>> infinitely if there is no walsender at that time. To enable them to be acked
>> even in that situation, we need to change postmaster so that it accepts the
>> replication connection even during smart shutdown (until we reach
>> PM_SHUTDOWN_2 state). Postmaster has already accepted the superuser
>> connection to cancel backup during smart shutdown. So I don't think that
>> the idea to accept the replication connection during smart shutdown is so
>> ugly.
>>
>> * Fast shutdown
>> I agree with you about fast shutdown. Fast shutdown should cause all the
>> backends including waiting ones to exit immediately. At that time, the
>> non-acked backend should not return the success, according to the
>> definition of sync rep. So we need to change a backend so that it gets rid
>> of itself from the waiting queue and exits before returning the success,
>> when it receives SIGTERM. This change leads the waiting backends to
>> do the same even when pg_terminate_backend is called. But since
>> they've not been acked yet, it seems to be reasonable to prevent them
>> from returning the COMMIT.
>>
>> Comments? I'll create the patch barring objection.
>
> The fast smart shutdown part of this problem has been addressed.  The

Ugh.  I mean "the fast shutdown", of course, not "the fast smart
shutdown".  Anyway, point is:

fast shutdown now OK
smart shutdown still not OK
do you want to write a patch?

:-)

> smart shutdown case still needs work, and I think the consensus was
> that your proposal above was the best way to go with it.
>
> Do you still want to work up a patch for this?  If so, I can review.

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

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-23T08:53:14Z

On Sat, Mar 19, 2011 at 11:28 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Fri, Mar 18, 2011 at 10:25 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Tue, Mar 8, 2011 at 7:05 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> * Smart shutdown
>>> Smart shutdown should wait for all the waiting backends to be acked, and
>>> should not cause them to forcibly exit. But this leads shutdown to get stuck
>>> infinitely if there is no walsender at that time. To enable them to be acked
>>> even in that situation, we need to change postmaster so that it accepts the
>>> replication connection even during smart shutdown (until we reach
>>> PM_SHUTDOWN_2 state). Postmaster has already accepted the superuser
>>> connection to cancel backup during smart shutdown. So I don't think that
>>> the idea to accept the replication connection during smart shutdown is so
>>> ugly.
>>>
>>> * Fast shutdown
>>> I agree with you about fast shutdown. Fast shutdown should cause all the
>>> backends including waiting ones to exit immediately. At that time, the
>>> non-acked backend should not return the success, according to the
>>> definition of sync rep. So we need to change a backend so that it gets rid
>>> of itself from the waiting queue and exits before returning the success,
>>> when it receives SIGTERM. This change leads the waiting backends to
>>> do the same even when pg_terminate_backend is called. But since
>>> they've not been acked yet, it seems to be reasonable to prevent them
>>> from returning the COMMIT.
>>>
>>> Comments? I'll create the patch barring objection.
>>
>> The fast smart shutdown part of this problem has been addressed.  The
>
> Ugh.  I mean "the fast shutdown", of course, not "the fast smart
> shutdown".  Anyway, point is:
>
> fast shutdown now OK
> smart shutdown still not OK
> do you want to write a patch?
>
> :-)
>
>> smart shutdown case still needs work, and I think the consensus was
>> that your proposal above was the best way to go with it.
>>
>> Do you still want to work up a patch for this?  If so, I can review.

Sure. Will do.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-24T11:17:30Z

On Wed, Mar 23, 2011 at 5:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> Do you still want to work up a patch for this?  If so, I can review.
>
> Sure. Will do.

The attached patch allows standby servers to connect during smart shutdown
in order to wake up backends waiting for sync rep.

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-24T11:34:40Z

On Thu, Mar 24, 2011 at 11:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Mar 23, 2011 at 5:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>> Do you still want to work up a patch for this?  If so, I can review.
>>
>> Sure. Will do.
>
> The attached patch allows standby servers to connect during smart shutdown
> in order to wake up backends waiting for sync rep.

I think that is possibly OK, but the big problem is the lack of a
clear set of comments about how the states are supposed to interact
that allow these changes to be validated.

That state isn't down to you, but I think we need that clear so this
change is more obviously correct than it is now.

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Sync Rep v19

Fujii Masao <masao.fujii@gmail.com> — 2011-03-24T11:53:42Z

On Thu, Mar 24, 2011 at 8:34 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> On Thu, Mar 24, 2011 at 11:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> On Wed, Mar 23, 2011 at 5:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>> Do you still want to work up a patch for this?  If so, I can review.
>>>
>>> Sure. Will do.
>>
>> The attached patch allows standby servers to connect during smart shutdown
>> in order to wake up backends waiting for sync rep.
>
> I think that is possibly OK, but the big problem is the lack of a
> clear set of comments about how the states are supposed to interact
> that allow these changes to be validated.
>
> That state isn't down to you, but I think we need that clear so this
> change is more obviously correct than it is now.

src/backend/replication/README needs to be updated to make that clear?

Regards,

-- 
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Re: Sync Rep v19

Simon Riggs <simon@2ndquadrant.com> — 2011-03-28T19:03:45Z

On Thu, Mar 24, 2011 at 11:53 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Thu, Mar 24, 2011 at 8:34 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> On Thu, Mar 24, 2011 at 11:17 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> On Wed, Mar 23, 2011 at 5:53 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>>>>> Do you still want to work up a patch for this?  If so, I can review.
>>>>
>>>> Sure. Will do.
>>>
>>> The attached patch allows standby servers to connect during smart shutdown
>>> in order to wake up backends waiting for sync rep.
>>
>> I think that is possibly OK, but the big problem is the lack of a
>> clear set of comments about how the states are supposed to interact
>> that allow these changes to be validated.
>>
>> That state isn't down to you, but I think we need that clear so this
>> change is more obviously correct than it is now.
>
> src/backend/replication/README needs to be updated to make that clear?

Not sure where we'd put them.

-- 
 Simon Riggs                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

Re: Sync Rep v19

Bruce Momjian <bruce@momjian.us> — 2011-04-26T15:40:25Z

Simon Riggs wrote:
> On Wed, 2011-03-09 at 21:21 -0500, Bruce Momjian wrote:
> > Simon Riggs wrote:
> > > On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:
> > > 
> > > > postgres=# SELECT application_name, state, sync_priority, sync_state
> > > > FROM pg_stat_replication;
> > > >  application_name |   state   | sync_priority | sync_state
> > > > ------------------+-----------+---------------+------------
> > > >  one              | STREAMING |             1 | POTENTIAL
> > > >  two              | streaming |             2 | sync
> > > > (2 rows)
> > > 
> > > Bug! Thanks.
> > 
> > Is there a reason these status are all upper-case?
> 
> NOT AS FAR AS I KNOW.
> 
> I'll add it to the list of changes for beta.

The attached patch lowercases the labels displayed in the view above. 
(The example above was originally all upper-case.)

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +

Re: Sync Rep v19

Bruce Momjian <bruce@momjian.us> — 2011-04-30T02:29:01Z

Bruce Momjian wrote:
> Simon Riggs wrote:
> > On Wed, 2011-03-09 at 21:21 -0500, Bruce Momjian wrote:
> > > Simon Riggs wrote:
> > > > On Fri, 2011-03-04 at 23:15 +0900, Fujii Masao wrote:
> > > > 
> > > > > postgres=# SELECT application_name, state, sync_priority, sync_state
> > > > > FROM pg_stat_replication;
> > > > >  application_name |   state   | sync_priority | sync_state
> > > > > ------------------+-----------+---------------+------------
> > > > >  one              | STREAMING |             1 | POTENTIAL
> > > > >  two              | streaming |             2 | sync
> > > > > (2 rows)
> > > > 
> > > > Bug! Thanks.
> > > 
> > > Is there a reason these status are all upper-case?
> > 
> > NOT AS FAR AS I KNOW.
> > 
> > I'll add it to the list of changes for beta.
> 
> The attached patch lowercases the labels displayed in the view above. 
> (The example above was originally all upper-case.)

Applied.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

  + It's impossible for everything to be true. +