On Mon, Oct 17, 2016 at 4:00 PM, Masahiko Sawada <sawada.m...@gmail.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 = CStringGetTextDatum("sync"); + values = 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 -- Sent via pgsql-hackers mailing list (firstname.lastname@example.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers