Re: Sync Rep v17

On Mon, Feb 21, 2011 at 6:06 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> I've read about a tenth of the patch, so I'll submit another comments
> about the rest later.

Here are another comments:

SyncRepReleaseWaiters should be called when walsender exits. Otherwise,
if the standby crashes while a transaction is waiting for replication,
it waits infinitely.

sync_rep_service and potential_sync_standby are not required to be in the
WalSnd shmem because only walsender accesses them.

+static bool
+SyncRepServiceAvailable(void)
+{
+	bool	 result = false;
+
+	SpinLockAcquire(&WalSndCtl->ctlmutex);
+	result = WalSndCtl->sync_rep_service_available;
+	SpinLockRelease(&WalSndCtl->ctlmutex);
+
+	return result;
+}

volatile pointer needs to be used to prevent code rearrangement.

+	slock_t		ctlmutex;		/* locks shared variables shown above */

cltmutex should be initialized by calling SpinLockInit.

+			/*
+			 * Stop providing the sync rep service, even if there are
+			 * waiting backends.
+			 */
+			{
+				SpinLockAcquire(&WalSndCtl->ctlmutex);
+				WalSndCtl->sync_rep_service_available = false;
+				SpinLockRelease(&WalSndCtl->ctlmutex);
+			}

sync_rep_service_available should be set to false only when
there is no synchronous walsender.

+	/*
+	 * When we first start replication the standby will be behind the primary.
+	 * For some applications, for example, synchronous replication, it is
+	 * important to have a clear state for this initial catchup mode, so we
+	 * can trigger actions when we change streaming state later. We may stay
+	 * in this state for a long time, which is exactly why we want to be
+	 * able to monitor whether or not we are still here.
+	 */
+	WalSndSetState(WALSNDSTATE_CATCHUP);
+

The above has already been committed. Please remove that from the patch.

I don't like calling SyncRepReleaseWaiters for each feedback because
I guess that it's too frequent. How about receiving all the feedbacks available
from the socket, and then calling SyncRepReleaseWaiters as well as
walreceiver does?

+	bool		ownLatch;		/* do we own the above latch? */

We can just remove the ownLatch flag.


I've read about two-tenths of the patch, so I'll submit another comments
about the rest later. Sorry for the slow reviewing...

Regards,

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