Re: Support for N synchronous standby servers - take 2

On Wed, Feb 10, 2016 at 3:13 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
> On Wed, Feb 10, 2016 at 11:25 AM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
> 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.

+#define MAX_WALSENDER_NAME 8192
+typedef enum WalSndState{    WALSNDSTATE_STARTUP = 0,
@@ -62,6 +64,11 @@ typedef struct WalSnd     * SyncRepLock.     */    int            sync_standby_priority;
+
+    /*
+     * Corresponding standby's application_name.
+     */
+    const char       name[MAX_WALSENDER_NAME];} WalSnd;
NAMEDATALEN instead?
-- 
Michael