Re: Support for N synchronous standby servers

On Fri, Aug 15, 2014 at 4:05 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Thu, Aug 14, 2014 at 8:34 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> + At any one time there will be at a number of active
>> synchronous standbys
>> + defined by <varname>synchronous_standby_num</>; transactions waiting
>>
>> It's better to use <xref linkend="guc-synchronous-standby-num">, instead.
> Fixed.
>
>> + for commit will be allowed to proceed after those standby servers
>> + confirms receipt of their data. The synchronous standbys will be
>>
>> Typo: confirms -> confirm
>
> Fixed.
>
>> + <para>
>> + Specifies the number of standbys that support
>> + <firstterm>synchronous replication</>, as described in
>> + <xref linkend="synchronous-replication">, and listed as the first
>> + elements of <xref linkend="guc-synchronous-standby-names">.
>> + </para>
>> + <para>
>> + Default value is 1.
>> + </para>
>>
>> synchronous_standby_num is defined with PGC_SIGHUP. So the following
>> should be added into the document.
>>
>> This parameter can only be set in the postgresql.conf file or on
>> the server command line.
> Fixed.
>
>> The name of the parameter "synchronous_standby_num" sounds to me that
>> the transaction must wait for its WAL to be replicated to s_s_num standbys.
>> But that's not true in your patch. If s_s_names is empty, replication works
>> asynchronously whether the value of s_s_num is. I'm afraid that it's confusing.
>> The description of s_s_num is not sufficient. I'm afraid that users can easily
>> misunderstand that they can use quorum commit feature by using s_s_names
>> and s_s_num. That is, the transaction waits for its WAL to be replicated to
>> any s_s_num standbys listed in s_s_names.
>
> I reworked the docs to mention all that. Yes things are a bit
> different than any quorum commit facility (how to parametrize that
> simply without a parameter mapping one to one the items of
> s_s_names?), as this facility relies on the order of the items of
> s_s_names and the fact that stansbys are connected at a given time.
>
>> When s_s_num is set to larger value than max_wal_senders, we should warn that?
> Actually I have done a bit more than that by forbidding setting
> s_s_num to a value higher than max_wal_senders. Thoughts?

You added check_synchronous_standby_num() as the GUC check function for
synchronous_standby_num, and checked that there. But that seems to be wrong.
You can easily see the following error messages even if synchronous_standby_num
is smaller than max_wal_senders. The point is that synchronous_standby_num
should be located before max_wal_senders in postgresql.conf.

LOG: invalid value for parameter "synchronous_standby_num": 0
DETAIL: synchronous_standby_num cannot be higher than max_wal_senders.

> Now that we discuss the interactions with other parameters. Another
> thing that I am wondering about now is: what should we do if we
> specify s_s_num to a number higher than the elements in s_s_names?
> Currently, the patch gives the priority to s_s_num, in short if we set
> s_s_num to 100, server will wait for 100 servers to confirm commit
> even if there are less than 100 elements in s_s_names. I chose this
> way because it looks saner particularly if s_s_names = '*'. Thoughts
> once again?

I'm fine with this. As you gave an example, the number of entries in s_s_names
can be smaller than the number of actual active sync standbys. For example,
when s_s_names is set to 'hoge', more than one standbys with the name 'hoge'
can connect to the server with sync mode.

I still think that it's strange that replication can be async even when
s_s_num is larger than zero. That is, I think that the transaction must
wait for s_s_num sync standbys whether s_s_names is empty or not.
OTOH, if s_s_num is zero, replication must be async whether s_s_names
is empty or not. At least for me, it's intuitive to use s_s_num primarily
to control the sync mode. Of course, other hackers may have different
thoughts, so we need to keep our ear open for them.

In the above design, one problem is that the number of parameters
that those who want to set up only one sync replication need to change is
incremented by one. That is, they need to change s_s_num additionally.
If we are really concerned about this, we can treat a value of -1 in
s_s_num as the special value, which allows us to control sync replication
only by s_s_names as we do now. That is, if s_s_names is empty,
replication would be async. Otherwise, only one standby with
high-priority in s_s_names becomes sync one. Probably the default of
s_s_num should be -1. Thought?

The source code comments at the top of syncrep.c need to be udpated.
It's worth checking whether there are other comments to be updated.

Regards,

--
Fujii Masao