Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

Hello, attached is the new version v8.

At Tue, 26 Apr 2016 11:02:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp> wrote in
<20160426.110225.35506931.horiguchi.kyotaro@lab.ntt.co.jp>
> At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <476.1461420723@sss.pgh.pa.us>
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > The main point for this improvement is that the handling for guc s_s_names
> > > is not similar to what we do for other somewhat similar guc's and which
> > > causes in-efficiency in non-hot code path (less used code).
> > 
> > This is not about efficiency, this is about correctness.  The proposed
> > v7 patch is flat out not acceptable, not now and not for 9.7 either,
> > because it introduces a GUC assign hook that can easily fail (eg, through
> > out-of-memory for the copy step).  Assign hook functions need to be
> > incapable of failure.  I do not see any good reason why this one cannot
> > satisfy that requirement, either.  It just needs to make use of the
> > "extra" mechanism to pass back an already-suitably-long-lived result from
> > check_synchronous_standby_names.  See check_timezone_abbreviations/
> > assign_timezone_abbreviations for a model to follow. 
> 
> I had already seen there before the v7 and had the same feeling
> below in mind but packing in a blob needs to use other than List
> to hold the name list (just should be an array) and it is
> followed by the necessity of many changes in where the list is
> accessed. But the result is hopeless as you mentioned :(
> 
> > You are going to
> > need to find a way to package the parse result into a single malloc'd
> > blob, though, because that's as much as guc.c can keep track of for an
> > "extra" value.
> 
> Ok, I'll post the v8 with the blob solution sooner.

Hmm. It was way easier than I thought. The attached v8 patch does,

- Changed SyncRepConfigData from a struct using liked list to a blob. Since the former struct is useful in parsing, it
isstill used and converted into the latter form in check_s_s_names.
 

- Make assign_s_s_names not to do nothing other than just assigning SyncRepConfig.

- Change SyncRepGetSyncStandbys to read the latter form of configuration.

- SyncRepFreeConfig is removed since it is no longer needed.

It passes both make check and recovery/make check.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..376fe51 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -361,11 +361,6 @@ SyncRepInitConfig(void){    int            priority;
-    /* Update the config data of synchronous replication */
-    SyncRepFreeConfig(SyncRepConfig);
-    SyncRepConfig = NULL;
-    SyncRepUpdateConfig();
-    /*     * Determine if we are a potential sync standby and remember the result     * for handling replies from
standby.
@@ -575,7 +570,7 @@ SyncRepGetSyncStandbys(bool *am_sync)    if (am_sync != NULL)        *am_sync = false;
-    lowest_priority = list_length(SyncRepConfig->members);
+    lowest_priority = SyncRepConfig->nmembers;    next_highest_priority = lowest_priority + 1;    /*
@@ -730,9 +725,7 @@ SyncRepGetSyncStandbys(bool *am_sync)static intSyncRepGetStandbyPriority(void){
-    List       *members;
-    ListCell   *l;
-    int            priority = 0;
+    int            priority;    bool        found = false;    /*
@@ -745,12 +738,9 @@ SyncRepGetStandbyPriority(void)    if (!SyncStandbysDefined())        return 0;
-    members = SyncRepConfig->members;
-    foreach(l, members)
+    for (priority = 1 ; priority <= SyncRepConfig->nmembers ; priority++)    {
-        char       *standby_name = (char *) lfirst(l);
-
-        priority++;
+        char  *standby_name = SyncRepConfig->members[priority - 1];        if (pg_strcasecmp(standby_name,
application_name)== 0 ||            pg_strcasecmp(standby_name, "*") == 0)
 
@@ -867,50 +857,6 @@ SyncRepUpdateSyncStandbysDefined(void)    }}
-/*
- * Parse synchronous_standby_names and update the config data
- * of synchronous standbys.
- */
-void
-SyncRepUpdateConfig(void)
-{
-    int    parse_rc;
-
-    if (!SyncStandbysDefined())
-        return;
-
-    /*
-     * check_synchronous_standby_names() verifies the setting value of
-     * synchronous_standby_names before this function is called. So
-     * syncrep_yyparse() must not cause an error here.
-     */
-    syncrep_scanner_init(SyncRepStandbyNames);
-    parse_rc = syncrep_yyparse();
-    syncrep_scanner_finish();
-
-    if (parse_rc != 0)
-        ereport(ERROR,
-                (errcode(ERRCODE_SYNTAX_ERROR),
-                 errmsg_internal("synchronous_standby_names parser returned %d",
-                                 parse_rc)));
-
-    SyncRepConfig = syncrep_parse_result;
-    syncrep_parse_result = NULL;
-}
-
-/*
- * Free a previously-allocated config data of synchronous replication.
- */
-void
-SyncRepFreeConfig(SyncRepConfigData *config)
-{
-    if (!config)
-        return;
-
-    list_free_deep(config->members);
-    pfree(config);
-}
-#ifdef USE_ASSERT_CHECKINGstatic boolSyncRepQueueIsOrderedByLSN(int mode)
@@ -956,9 +902,16 @@ boolcheck_synchronous_standby_names(char **newval, void **extra, GucSource source){    int
parse_rc;
+    SyncRepConfigData *pconf;
+    int i;
+    ListCell *lc;    if (*newval != NULL && (*newval)[0] != '\0')    {
+        /*
+         * syncrep_yyparse generates a result on the current memory context as
+         * the side effect and points it using syncrep_prase_result.
+         */        syncrep_scanner_init(*newval);        parse_rc = syncrep_yyparse();
syncrep_scanner_finish();
@@ -1016,18 +969,35 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
     errhint("Specify more names of potential synchronous standbys in synchronous_standby_names.")));        }
 
-        /*
-         * syncrep_yyparse sets the global syncrep_parse_result as side effect.
-         * But this function is required to just check, so frees it
-         * after parsing the parameter.
-         */
-        SyncRepFreeConfig(syncrep_parse_result);
-    }
+        /* Convert SyncRepConfig into the packed struct fit to guc extra */
+        pconf = (SyncRepConfigData *)
+            malloc(SizeOfSyncRepConfig(
+                       list_length(syncrep_parse_result->members)));
+        pconf->num_sync = syncrep_parse_result->num_sync;
+        pconf->nmembers = list_length(syncrep_parse_result->members);
+        i = 0;
+        foreach (lc, syncrep_parse_result->members)
+        {
+            strncpy(pconf->members[i], (char*) lfirst (lc), NAMEDATALEN - 1);
+            pconf->members[i][NAMEDATALEN - 1] = 0;
+            i++;
+        }
+        *extra = (void *)pconf;
+
+        /* No further need for syncrep_parse_result */
+        syncrep_parse_result = NULL;
+    }    return true;}void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+    SyncRepConfig = (SyncRepConfigData *) extra;
+}
+
+voidassign_synchronous_commit(int newval, void *extra){    switch (newval)
diff --git a/src/backend/replication/syncrep_gram.y b/src/backend/replication/syncrep_gram.y
index 380fedc..932fa9d 100644
--- a/src/backend/replication/syncrep_gram.y
+++ b/src/backend/replication/syncrep_gram.y
@@ -19,9 +19,9 @@#include "utils/formatting.h"/* Result of the parsing is returned here */
-SyncRepConfigData    *syncrep_parse_result;
+SyncRepParseData    *syncrep_parse_result;
-static SyncRepConfigData *create_syncrep_config(char *num_sync, List *members);
+static SyncRepParseData *create_syncrep_config(char *num_sync, List *members);/* * Bison doesn't allocate anything
thatneeds to live across parser calls,
 
@@ -43,7 +43,7 @@ static SyncRepConfigData *create_syncrep_config(char *num_sync, List *members);{    char       *str;
 List       *list;
 
-    SyncRepConfigData  *config;
+    SyncRepParseData  *config;}%token <str> NAME NUM
@@ -72,11 +72,11 @@ standby_name:;%%
-static SyncRepConfigData *
+static SyncRepParseData *create_syncrep_config(char *num_sync, List *members){
-    SyncRepConfigData *config =
-        (SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+    SyncRepParseData *config =
+        (SyncRepParseData *) palloc(sizeof(SyncRepParseData));    config->num_sync = atoi(num_sync);
config->members= members;
 
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 81d3d28..20d23d5 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -2780,23 +2780,12 @@ pg_stat_get_wal_senders(PG_FUNCTION_ARGS)    MemoryContextSwitchTo(oldcontext);    /*
-     * Allocate and update the config data of synchronous replication,
-     * and then get the currently active synchronous standbys.
+     * Get the currently active synchronous standbys.     */
-    SyncRepUpdateConfig();    LWLockAcquire(SyncRepLock, LW_SHARED);    sync_standbys = SyncRepGetSyncStandbys(NULL);
 LWLockRelease(SyncRepLock);
 
-    /*
-     * Free the previously-allocated config data because a backend
-     * no longer needs it. The next call of this function needs to
-     * allocate and update the config data newly because the setting
-     * of sync replication might be changed between the calls.
-     */
-    SyncRepFreeConfig(SyncRepConfig);
-    SyncRepConfig = NULL;
-    for (i = 0; i < max_wal_senders; i++)    {        WalSnd *walsnd = &WalSndCtl->walsnds[i];
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 60856dd..cccc8eb 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -3484,7 +3484,7 @@ static struct config_string ConfigureNamesString[] =        },        &SyncRepStandbyNames,
"",
 
-        check_synchronous_standby_names, NULL, NULL
+        check_synchronous_standby_names, assign_synchronous_standby_names, NULL    },    {
diff --git a/src/include/replication/syncrep.h b/src/include/replication/syncrep.h
index 14b5664..6197308 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -33,16 +33,30 @@#define SYNC_REP_WAIT_COMPLETE        2/*
+ * Struct for parsing synchronous_standby_names
+ */
+typedef struct SyncRepParseData
+{
+    int    num_sync;    /* number of sync standbys that we need to wait for */
+    List    *members;    /* list of names of potential sync standbys */
+} SyncRepParseData;
+
+/* * Struct for the configuration of synchronous replication. */typedef struct SyncRepConfigData{    int    num_sync;
 /* number of sync standbys that we need to wait for */
 
-    List    *members;    /* list of names of potential sync standbys */
+    int    nmembers;    /* number of members in the following list */
+    char members[FLEXIBLE_ARRAY_MEMBER][NAMEDATALEN];/* list of names of
+                                                      * potential sync
+                                                      * standbys */} SyncRepConfigData;
-extern SyncRepConfigData *syncrep_parse_result;
-extern SyncRepConfigData *SyncRepConfig;
+#define SizeOfSyncRepConfig(n) \
+    (offsetof(SyncRepConfigData, members) + (n) * NAMEDATALEN)
+
+extern SyncRepParseData *syncrep_parse_result;/* user-settable parameters for synchronous replication */extern char
*SyncRepStandbyNames;
@@ -59,13 +73,12 @@ extern void SyncRepReleaseWaiters(void);/* called by wal sender and user backend */extern List
*SyncRepGetSyncStandbys(bool*am_sync);
 
-extern void SyncRepUpdateConfig(void);
-extern void SyncRepFreeConfig(SyncRepConfigData *config);/* called by checkpointer */extern void
SyncRepUpdateSyncStandbysDefined(void);externbool check_synchronous_standby_names(char **newval, void **extra,
GucSourcesource);
 
+extern void assign_synchronous_standby_names(const char *newval, void *extra);extern void
assign_synchronous_commit(intnewval, void *extra);/*