Re: Support for N synchronous standby servers - take 2

Hello,

At Wed, 10 Feb 2016 15:22:44 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqRk4ZjoQfs4rmF6Di1zp=b4eA=hk0L4GFzUj47GwhgM7g@mail.gmail.com>
> 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.

Mmmm. I found that application_name can contain
commas. Furthermore, there seems to be no limitation for
character in the name.

postgres=# set application_name='ho,ge';
postgres=# select application_name from pg_stat_activity;application_name 
------------------ho,ge

check_application_name() allows all characters in the range
between 32 to 126 in ascii. All other characters are replaced
with '?'.

> > +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?

-- 
Kyotaro Horiguchi
NTT Open Source Software Center