Re: Support for N synchronous standby servers - take 2

At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoCOL6BCC+FWNCZH_XPgtWc_otnvShMx6_uAcU7Bwb16Rw@mail.gmail.com>
> >> How about if we do all the parsing stuff in temporary context and then copy
> >> the results using TopMemoryContext?  I don't think it will be a leak in
> >> TopMemoryContext, because next time we try to check/assign s_s_names, it
> >> will free the previous result.
> >
> > I agree with you. A temporary context for the parser seems
> > reasonable. TopMemoryContext is created very early in main() so
> > palloc on it is effectively the same with malloc.
> > One problem is that only the top memory block is assumed to be
> > free()'d, not pfree()'d by guc_set_extra. It makes this quite
> > ugly..
> >
> > Maybe we shouldn't use the extra for this purpose.
> >
> > Thoughts?
> >
> 
> How about if check_hook just parses parameter in
> CurrentMemoryContext(i.g., T_AllocSetContext), and then the
> assign_hook copies syncrep_parse_result to TopMemoryContext.
> Because syncrep_parse_result is a global variable, these hooks can see it.

Hmm. Somewhat uneasy but should work. The attached patch does it.

> Here are some comments.
> 
> -SyncRepUpdateConfig(void)
> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext cxt)
> 
> Sorry, it's my bad. itself variables is no longer needed because
> SyncRepFreeConfig is called by only one function.
> 
> -void
> -SyncRepFreeConfig(SyncRepConfigData *config)
> +SyncRepConfigData *
> +SyncRepCopyConfig(SyncRepConfigData *oldconfig, MemoryContext targetcxt)
> 
> I'm not sure targetcxt argument is necessary.

Yes, these are just for signalling so removal doesn't harm.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..3d68fb5 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -68,6 +68,7 @@#include "storage/proc.h"#include "tcop/tcopprot.h"#include "utils/builtins.h"
+#include "utils/memutils.h"#include "utils/ps_status.h"/* User-settable parameters for sync rep */
@@ -361,11 +362,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.
@@ -868,47 +864,50 @@ SyncRepUpdateSyncStandbysDefined(void)}/*
- * Parse synchronous_standby_names and update the config data
- * of synchronous standbys.
+ * Free a previously-allocated config data of synchronous replication. */void
-SyncRepUpdateConfig(void)
+SyncRepFreeConfig(SyncRepConfigData *config){
-    int    parse_rc;
-
-    if (!SyncStandbysDefined())
+    if (!config)        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;
+    list_free_deep(config->members);
+    pfree(config);}/*
- * Free a previously-allocated config data of synchronous replication.
+ * Returns a copy of a replication config data into the TopMemoryContext. */
-void
-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepConfigData *
+SyncRepCopyConfig(SyncRepConfigData *oldconfig){
-    if (!config)
-        return;
+    MemoryContext        oldcxt;
+    SyncRepConfigData  *newconfig;
+    ListCell           *lc;
-    list_free_deep(config->members);
-    pfree(config);
+    if (!oldconfig)
+        return NULL;
+
+    oldcxt = MemoryContextSwitchTo(TopMemoryContext);
+
+    newconfig = (SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+    newconfig->num_sync = oldconfig->num_sync;
+    newconfig->members = list_copy(oldconfig->members);
+
+    /*
+     * The new members list is a combination of list cells on the new context
+     * and data pointed from each cell on the old context. So we explicitly
+     * copy the data.
+     */
+    foreach (lc, newconfig->members)
+    {
+        lfirst(lc) = pstrdup((char *) lfirst(lc));
+    }
+
+    MemoryContextSwitchTo(oldcxt);
+
+    return newconfig;}#ifdef USE_ASSERT_CHECKING
@@ -957,6 +956,8 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source){    int
parse_rc;
+    Assert(syncrep_parse_result == NULL);
+    if (*newval != NULL && (*newval)[0] != '\0')    {        syncrep_scanner_init(*newval);
@@ -965,6 +966,7 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)        if (parse_rc
!=0)        {
 
+            syncrep_parse_result = NULL;            GUC_check_errcode(ERRCODE_SYNTAX_ERROR);
GUC_check_errdetail("synchronous_standby_namesparser returned %d",                                parse_rc);
 
@@ -1017,17 +1019,39 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)        }
/*
-         * 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.
+         * We leave syncrep_parse_result for the use in
+         * assign_synchronous_standby_names.         */
-        SyncRepFreeConfig(syncrep_parse_result);    }    return true;}void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+    /* Free the old SyncRepConfig if exists */
+    if (SyncRepConfig)
+        SyncRepFreeConfig(SyncRepConfig);
+
+    SyncRepConfig = NULL;
+
+    /* Copy the parsed config into TopMemoryContext if exists */
+    if (syncrep_parse_result)
+    {
+        SyncRepConfig = SyncRepCopyConfig(syncrep_parse_result);
+
+        /*
+         * this memory block will be freed as a part of the memory contxt for
+         * config file processing.
+         */
+        syncrep_parse_result = NULL;
+    }
+
+    return;
+}
+
+voidassign_synchronous_commit(int newval, void *extra){    switch (newval)
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 fb091bc..3ce83bf 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..9a1eb2f 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -59,13 +59,14 @@ 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);
+extern SyncRepConfigData *SyncRepCopyConfig(SyncRepConfigData *oldconfig);/* 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);/*