Re: Sync Rep v19

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