Re: Sync Rep v17

On Mon, Feb 21, 2011 at 6:06 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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