Re: Support for N synchronous standby servers - Mailing list pgsql-hackers

On Thu, Aug 14, 2014 at 8:34 PM, Fujii Masao <masao.fujii@gmail.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?

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?

> +    for (i = 0; i < num_sync; i++)
> +    {
> +        volatile WalSnd *walsndloc = &WalSndCtl->walsnds[sync_standbys[i]];
> +
> +        if (min_write_pos > walsndloc->write)
> +            min_write_pos = walsndloc->write;
> +        if (min_flush_pos > walsndloc->flush)
> +            min_flush_pos = walsndloc->flush;
> +    }
>
> I don't think that it's safe to see those shared values without spinlock.
Looking at walsender.c you are right. I have updated the code to use
the mutex lock of the walsender whose values are being read from.

Regards,
--
Michael

On Thu, Aug 14, 2014 at 4:34 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
> On Wed, Aug 13, 2014 at 4:10 PM, Michael Paquier
> <michael.paquier@gmail.com> wrote:
>> On Wed, Aug 13, 2014 at 2:10 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>>> I sent the SIGSTOP signal to the walreceiver process in one of sync standbys,
>>> and then ran write transactions again. In this case, they must not be completed
>>> because their WAL cannot be replicated to the standby that its walreceiver
>>> was stopped. But they were successfully completed.
>>
>> At the end of SyncRepReleaseWaiters, SYNC_REP_WAIT_WRITE and
>> SYNC_REP_WAIT_FLUSH in walsndctl were able to update with only one wal
>> sender in sync, making backends wake up even if other standbys did not
>> catch up. But we need to scan all the synchronous wal senders and find
>> the minimum write and flush positions and update walsndctl with those
>> values. Well that's a code path I forgot to cover.
>>
>> Attached is an updated patch fixing the problem you reported.
>
> +        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.
>
> +        for commit will be allowed to proceed after those standby servers
> +        confirms receipt of their data. The synchronous standbys will be
>
> Typo: confirms -> confirm
>
> +       <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.
>
> 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.
>
> When s_s_num is set to larger value than max_wal_senders, we should warn that?
>
> +    for (i = 0; i < num_sync; i++)
> +    {
> +        volatile WalSnd *walsndloc = &WalSndCtl->walsnds[sync_standbys[i]];
> +
> +        if (min_write_pos > walsndloc->write)
> +            min_write_pos = walsndloc->write;
> +        if (min_flush_pos > walsndloc->flush)
> +            min_flush_pos = walsndloc->flush;
> +    }
>
> I don't think that it's safe to see those shared values without spinlock.
>
> Regards,
>
> --
> Fujii Masao



--
Michael