Re: Quorum commit for multiple synchronous replication.

On Mon, Oct 17, 2016 at 4:00 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> Attached latest patch.
> Please review it.

Okay, so let's move on with this patch...

+ <para>
+ The keyword <literal>ANY</> is omissible, but note that there is
+ not compatibility between <productname>PostgreSQL</> version 10 and
+ 9.6 or before. For example, <literal>1 (s1, s2)</> is the same as the
+ configuration with <literal>FIRST</> and <replaceable
class="parameter">
+ num_sync</replaceable> equal to 1 in <productname>PostgreSQL</> 9.6
+ or before. On the other hand, It's the same as the configuration with
+ <literal>ANY</> and <replaceable
class="parameter">num_sync</> equal to
+ 1 in <productname>PostgreSQL</> 10 or later.
+ </para>
This paragraph could be reworded:
If FIRST or ANY are not specified, this parameter behaves as ANY. Note
that this grammar is incompatible with PostgreSQL 9.6, where no
keyword specified is equivalent as if FIRST was specified.
In short, there is no real need to specify num_sync as this behavior
does not have changed, as well as it is not necessary to mention
pre-9.6 versions as the multi-sync grammar has been added in 9.6.

- Specifying more than one standby name can allow very high availability.
Why removing this sentence?

+ The keyword <literal>ANY</>, coupeld with an interger number N,
s/coupeld/coupled/ and s/interger/integer/, for a double hit in one
line, still...

+ The keyword <literal>ANY</>, coupeld with an interger number N,
+ chooses N standbys in a set of standbys with the same, lowest,
+ priority and makes transaction commit when WAL records are received
+ those N standbys.
This could be reworded more simply, for example: The keyword ANY,
coupled with an integer number N, makes transaction commits wait until
WAL records are received from N connected standbys among those defined
in the list of synchronous_standby_names.

+ <literal>s2</> and <literal>s3</> wil be considered as synchronous standby
s/wil/will/

+ when standby is considered as a condidate of quorum commit.</entry>
s/condidate/candidate/

[... stopping here ...] Please be more careful with the documentation
and comment grammar. There are other things in the patch..

A bunch of comments at the top of syncrep.c need to be updated.

+extern List *SyncRepGetSyncStandbysPriority(bool *am_sync);
+extern List *SyncRepGetSyncStandbysQuorum(bool *am_sync);
Those two should be static routines in syncrep.c, let's keep the whole
logic about quorum and higher-priority definition only there and not
bother the callers of them about that.

+ if (SyncRepConfig->sync_method == SYNC_REP_PRIORITY)
+ return SyncRepGetSyncStandbysPriority(am_sync);
+ else /* SYNC_REP_QUORUM */
+ return SyncRepGetSyncStandbysQuorum(am_sync);
Both routines share the same logic to detect if a WAL sender can be
selected as a candidate for sync evaluation or not, still per the
selection they do I agree that it is better to keep them as separate.

+ /* In quroum method, all sync standby priorities are always 1 */
+ if (found && SyncRepConfig->sync_method == SYNC_REP_QUORUM)
+ priority = 1;
Honestly I don't understand why you are enforcing that. Priority can
be important for users willing to switch from ANY to FIRST to have a
look immediately at what are the standbys that would become sync or
potential.

else if (list_member_int(sync_standbys, i))
- values[7] = CStringGetTextDatum("sync");
+ values[7] = SyncRepConfig->sync_method == SYNC_REP_PRIORITY ?
+ CStringGetTextDatum("sync") : CStringGetTextDatum("quorum");
The comment at the top of this code block needs to be refreshed.

The representation given to the user in pg_stat_replication is not
enough IMO. For example, imagine a cluster with 4 standbys:
=# select application_name, sync_priority, sync_state from pg_stat_replication ;
application_name | sync_priority | sync_state
------------------+---------------+------------
node_5433 | 0 | async
node_5434 | 0 | async
node_5435 | 0 | async
node_5436 | 0 | async
(4 rows)

If FIRST N is used, is it easy for the user to understand what are the
nodes in sync:
=# alter system set synchronous_standby_names = 'FIRST 2 (node_5433,
node_5434, node_5435)';
ALTER SYSTEM
=# select pg_reload_conf();
pg_reload_conf
----------------
t
(1 row)
=# select application_name, sync_priority, sync_state from pg_stat_replication ;
application_name | sync_priority | sync_state
------------------+---------------+------------
node_5433 | 1 | sync
node_5434 | 2 | sync
node_5435 | 3 | potential
node_5436 | 0 | async
(4 rows)

In this case it is easy to understand that two nodes are required to be in sync.

When using ANY similarly for three nodes, here is what
pg_stat_replication tells:
=# select application_name, sync_priority, sync_state from pg_stat_replication ;
application_name | sync_priority | sync_state
------------------+---------------+------------
node_5433 | 1 | quorum
node_5434 | 1 | quorum
node_5435 | 1 | quorum
node_5436 | 0 | async
(4 rows)

It is not possible to guess from how many standbys this needs to wait
for. One idea would be to mark the sync_state not as "quorum", but
"quorum-N", or just add a new column to indicate how many in the set
need to give a commit confirmation.

The patch is going into the right direction in my opinion.
--
Michael