Re: Support for N synchronous standby servers - take 2

On Sun, Feb 28, 2016 at 8:04 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached latest version patch.
>
> The changes from previous version are,
> - Fix parser, lexer bugs.
> - Add regression test patch based on patch Suraji submitted.
>
> Please review it.
>
> [000_multi_sync_replication_v13.patch]

Hi Masahiko,

Hi,

I have a couple of small suggestions for the documentation and comments:

+ Specifies a standby names that can support
<firstterm>synchronous replication</> using
+ either two types of syntax; comma-separated list or dedicated
language, as
+ described in <xref linkend="synchronous-replication">.
+ Transcations waiting for commit will be allowed to proceed after the
+ specified number of standby servers confirms receipt of their data.

Suggestion: Specifies the standby names that can support
<firstterm>synchronous replication</> using either of two syntaxes: a
comma-separated list, or a more flexible syntax described in <xref
linkend="synchronous-replication">. Transactions waiting for commit
will be allowed to proceed after a configurable subset of standby
servers confirms receipt of their data. For the simple
comma-separated list syntax, it is one server.

+ If the current any of synchronous standbys disconnects for
whatever reason,

s/the current any of/any of the current/

+ no mechanism to enforce uniqueness. For each specified standby name,
+ only the specified count of standbys will be chosen to be synchronous
+ standbys, though exactly which one is indeterminate, the rest will
+ represent potential synchronous standbys.

s/one/ones/
s/indeterminate, the/indeterminate. The/

+ made by a transcation have been transferred to one or more
synchronous standby
+ server. This extends that standard levelof durability

s/transcation/transaction/
s/that standard levelof/the standard level of/

offered by a transaction commit. This level of protection is referred
to as 2-safe replication in computer science theory.

Is this still called "2-safe" or does this patch make it "N-safe",
"group-safe", or something else?

- The minimum wait time is the roundtrip time between primary to standby.
+ The minimum wait time is the roundtrip time between primary to standbys.

Suggestion: The minimum wait time is the roundtrip time between the
primary and the slowest synchronous standby.

+ Multiple synchronous replication is set up by setting <xref
linkend="guc-synchronous-standby-names">
+ using dedicated language. The syntax of dedicated language is following.

Suggestion: Multiple synchronous replication is set up by setting
<xref linkend="guc-synchronous-standby-names"> using the following
syntax.

+ Using dedicated language, we can define a synchronous group with
a number N.
+ synchronous group can have some members which are consdiered as
synchronous standby using comma-separated list.
+ Any standby name is accepted at any position of its list, but '*'
is accepted at only tailing of the standby list.
+ The leading N is a number which specifies that how many standbys
the master server waits to commit for. This number
+ must be less than actual number of members of its group.
+ The listed standby are given highest priority from left defined
starting with 1.

Suggestion: This syntax allows us to define a synchronous group that
will wait for at least N standbys, and a comma-separated list of group
members. The special value <literal>*</> is accepted at the tail of
the member list, and matches any standby. The number N must not be
greater than the number of members listed in the group, unless
<literal>*</> is used. Priority is given to servers in the order that
they appear in the list. The first named server has the highest
priority.

+ All ASCII characters except for special characters(',', '&quot',
'[', ']', ' ') are allowed as standby name.
+ When these special characters are used as standby name, whole
standby name string need to be written in
+ double-quoted representation.

Suggestion: ... are allowed in unquoted standby names. To use these
special characters, the standby name should be enclosed in double
quotes.

+ * In 9.5 we support the possibility to have multiple synchronous standbys,

s/9.5/9.6/

+ * as defined in synchronous_standby_names. Before on standby can become a

s/ on / a /

+ * Waiters will be released from the queue once the number of standbys
+ * specified in synchronous_standby_names have caught.

s/caught/processed the commit record/

+ * Check whether specified standby is active, which means not only having
+ * pid but also having any priority.

s/having any priority/having a non-zero priority (meaning it is
configured as potential sync standby)./

- announce_next_takeover = true;

By removing this, haven't we lost the ability to announce takeover
more than once per walsender? I'm not sure exactly where this should
go now but the walsender needs to detect its own transition from
potential to sync state. Also, that message, where it appears below
should probably be tweaked slightly s/the/a/, so "standby \"%s\" is
now a synchronous standby with priority %u", not "... the synchronous
standby ...".

/*
+ * Return true if we have enough synchrononized standbys and the 'safe' written
+ * flushed LSNs, which are LSNs assured in all standbys considered should be
+ * synchronized.
+ */

Suggestion: Return true if we have enough synchronous standbys. If
true, also store the 'safe' write and flush position in the output
parameters write_pos and flush_pos, but only if the standby managed by
this walsender is one of the standbys that has reached each safe
position respectively.

+ /* Check whether each LSN has advanced to */

Suggestion: /* Check whether this standby has reached the safe positions. */

+/*
+ * Decide synced LSNs at this moment using priority method.
+ * If there are not active standbys enough to determine LSNs, return false.

s/not active standbys enough/not enough active standbys/

+/*
+ * Return the positions of the first group->wait_num synchronized standbys
+ * in group->member list into sync_list. sync_list is assumed to have enough
+ * space for at least group->wait_num elements.
+ */

s/Return/Write/
s/sychronized/synchronous/
Then add: "Return the number found."

+int
+SyncRepGetSyncStandbysUsingPriority(SyncGroupNode *group, int *sync_list)
+{
+ int target_priority = 1; /* lowest priority is 1 */

1 is actually the *highest* priority standby.

+ /*
+ * Select low priority standbys from walsnds array. If there are same
+ * priority standbys, first defined standby is selected. It's possible
+ * to have same priority different standbys, so we can not break loop
+ * even when standby having target_prioirty priority is found.

s/target_prioirty/target_priority/

+ /* Got enough synchronous stnadby */

s/stnadby/standbys/

+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ (errmsg_internal("The number of group memebers must be less than its
group waits."))));

I'm not sure what the right error code is, but this isn't an syntax
error. Maybe ERRCODE_CONFIG_FILE_ERROR or
ERRCODE_INVALID_PARAMETER_VALUE? Suggestion for the message: "the
configured number of synchronous standbys exceeds the length of the
group of standby names: %d"

+ /*
+ * syncgroup_yyparse sets the global SyncRepStandbys as side effect.
+ * But this function is required to just check, so frees SyncRepStandbyNanes

s/SyncRepStandbyNanes/SyncRepStandbys/ ???

+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ (errmsg_internal("Invalid syntax. synchronous_standby_names parse
returned %d",
+ parse_rc))));

Looking at other error messages I see that they always start with
lower case and then put extra details after ':' rather than using a
'.'. Maybe this could be "could not parse synchronous_standby_names:
error code %d"?

+#define MAX_WALSENDER_NAME 8192

Seems to be unused.

Thanks!

--
Thomas Munro
http://www.enterprisedb.com