Re: Comments to Synchronous replication patch v3

On Tue, Nov 25, 2008 at 11:42 AM, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
> "Fujii Masao" <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, Nov 14, 2008 at 7:15 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.

Hi, Itagaki-san. Thanks for taking time to review the 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?

I think that the real database is better, because it's simpler and the
user can re-create it easily even if it is lost accidentally. Of course,
if we introduce the new API to handle a virtual database, we can
virtualize it. Is it worth introducing it?

>
> 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 or while processing the request.

Obviously undesirable behavior :(
I will fix it.

>
> 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').

Agreed. The name 'replication' is more suitable, I also think.
Any other ideas?

>
> ----
> [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.

OK, I will add the new GUC option for specifying the behavior when the
timeout occurs. I think the valid values are ERROR, FATAL and PANIC.

* ERROR only cancels that the backend waits for replication. The backends
(including the canceled one) and walsender continue processing. But,
synchronous replication might be broken.

* FATAL terminates walsender. The backends continue processing without
replication.

* PANIC terminates all processes. In previous discussion, someone wanted
this behavior.

Or, should I define replication_timeout as PGC_SUSET?

>
> ----
> [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?

Couting the number of times which the buffer page is replaced in
AdvanceXLInsertBuffer might be help to configure them. Of course,
this kind of feature is very useful. But, is it essential for Synch Rep?
If not, I'd like to postpone it to 8.5.

>
> ----
> [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.

You are right! I will fix it.

>
> ----
> [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.

OK, I will push them into XLogFlush.

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

Oops! I will fix it.

Regards,

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