Re: Support for N synchronous standby servers - take 2

On Wed, Feb 10, 2016 at 11:25 AM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:
> Yes, I will implement regression test patch and documentation patch as
well.

Cool, now that we have a clear picture of where we want to move, that would
be an excellent thing to have. Having the docs in the place is clearly
mandatory.

> Attached latest version patch supporting s_s_names = '*'.
> Unlike currently behaviour a bit, s_s_names can have only one '*'
character.
> e.g, The following setting will get syntax error.
>
> s_s_names = '*, node1,node2'
> s_s_names = `2[node1, *, node2]`
>
> when we use '*' character as s_s_names element, we must set s_s_names
> like follows.
>
> s_s_names = '*'
> s_s_names = '2[*]'
>
> BTW, we've discussed about mini language syntax.
> IIRC, the syntax uses [] and () like,
> 'N[node1, node2, ...]', to define priority standbys.
> 'N(node1, node2, ...)', to define quorum standbys.
> And current patch behaves so.
>
> Which type of parentheses should be used for this syntax to be more
clarity?
> Or other character should be used such as <>, // ?

I am personally fine with () and [] as you mention, we could even consider
{}, each one of them has a different meaning mathematically..

I am not entered into a detailed review yet (waiting for the docs), but the
patch looks brittle. I have been able to crash the server just by querying
pg_stat_replication:
* thread #1: tid = 0x0000, 0x0000000105eb36c2
postgres`pg_stat_get_wal_senders(fcinfo=0x00007fff5a156290) + 498 at
walsender.c:2783, stop reason = signal SIGSTOP
* frame #0: 0x0000000105eb36c2
postgres`pg_stat_get_wal_senders(fcinfo=0x00007fff5a156290) + 498 at
walsender.c:2783
frame #1: 0x0000000105d4277d
postgres`ExecMakeTableFunctionResult(funcexpr=0x00007fea128f3838,
econtext=0x00007fea128f1b58, argContext=0x00007fea128c8ea8,
expectedDesc=0x00007fea128f4710, randomAccess='\0') + 1005 at
execQual.c:2211
frame #2: 0x0000000105d70c24
postgres`FunctionNext(node=0x00007fea128f2f78) + 180 at
nodeFunctionscan.c:95
* thread #1: tid = 0x0000, 0x0000000105eb36c2
postgres`pg_stat_get_wal_senders(fcinfo=0x00007fff5a156290) + 498 at
walsender.c:2783, stop reason = signal SIGSTOP
frame #0: 0x0000000105eb36c2
postgres`pg_stat_get_wal_senders(fcinfo=0x00007fff5a156290) + 498 at
walsender.c:2783
2780 /*
2781 * Get the currently active synchronous standby.
2782 */
-> 2783 sync_standbys = (int *) palloc(sizeof(int) *
SyncRepStandbyNames->wait_num);
2784 LWLockAcquire(SyncRepLock, LW_SHARED);
2785 num_sync =
SyncRepGetSyncStandbysPriority(SyncRepStandbyNames, sync_standbys);
2786 LWLockRelease(SyncRepLock);
(lldb) p SyncRepStandbyNames
(SyncGroupNode *) $0 = 0x0000000000000000

+sync_node_group:
+ sync_list { $$ = create_group_node(1,
$1); }
+ | sync_element_ast { $$ = create_group_node(1,
$1);}
+ | INT '[' sync_list ']' { $$ = create_group_node($1,
$3);}
+ | INT '[' sync_element_ast ']' { $$ = create_group_node($1,
$3); }
We may want to be careful with the use of '[' in application_name. I am not
much thrilled with forbidding the use of []() in application_name, so we
may want to recommend user to use a backslash when using s_s_names when a
group is defined.

+void
+yyerror(const char *message)
+{
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg_internal("%s", message)));
+}
whitespace errors here.
--
Michael