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