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(dot)fujii(at)gmail(dot)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