Comments to Synchronous replication patch v3

"Fujii Masao" <masao.fujii@gmail.com> wrote:
> On Fri, Nov 14, 2008 at 7:15 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
> > Attached is the latest version of Synch Rep patch. Sorry for my late posting.
> > I fixed some bugs in v1patch and integrated walreceiver into core.

I have some comments about v3 patch.

[1] Should walsender database be real or not?
[2] User-configurable replication_timeout is dangerous
[3] How to configure wal_buffers and wal_sender_delay?
[4] XLogArchivingActive on replication mode
[5] Usage of XLog Send-Flush-Wait
[6] Bit-OR or Logical-OR

----
[1] Should walsender database be real or not?
Index: bin/initdb/initdb.c
+        "CREATE DATABASE walsender;\n",

You create walsender as a *real* database, but is it really needed?
Can we make it a virtual database only for walreceiver?

And backend process crashes when I login the walsender database:
$ psql walsender
psql: server closed the connection unexpectedly      This probably means the server terminated abnormally      before
orwhile processing the request.
 

Even if we need to have the database in real, I'd like to use another
name for it. The name 'walsender' seems to be an internal module name
but it should be a feature name (ex. 'replication').

----
[2] User-configurable replication_timeout is dangerous
Index: backend/utils/misc/guc.c
+        {"replication_timeout", PGC_USERSET, WAL_REPLICATION,

You export replication_timeout as a PGC_USERSET variable, but it is
dangerous. It allows non-superusers to kill servers easily by setting it
too low value. Walsender dies with FATAL on timeout.

BTW, how about adding an option to choose FATAL or ERROR on timeout?
I think FATAL is too strong for some cases.

----
[3] How to configure wal_buffers and wal_sender_delay?
The parameter wal_buffers might be more important in synch rep,
but we don't have a mean to know whether wal_buffers is enough or not.
Should we have a new system view 'pg_stat_xlog' to find shortage
of wal_buffers and wal_sender_delay?

----
[4] XLogArchivingActive on replication mode
XLogArchivingActive is not modified in the patch, but is it safe?
For example, we might need to disable COPY-without-wal optimization
when replication is enabled.

----
[5] Usage of XLog Send-Flush-Wait
There are many following sequences:   RequestXLogSend(WriteRqstPtr, true);   XLogFlush(WriteRqstPtr);
WaitXLogSend(WriteRqstPtr,false, true);
 

It introduces additional complexities to use XLogFlush.
Is it possible to push RequestXLogSend and WaitXLogSend into XLogFlush?
It might require a new flag argument in XLogFlush.

----
[6] Bit-OR or Logical-ORWaitXLogSend(XactLastRecEnd, false, forceSyncCommit | haveNonTemp);
should beWaitXLogSend(XactLastRecEnd, false, forceSyncCommit || haveNonTemp);
| is bit OR and || is logical OR.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center