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

At Wed, 20 Apr 2016 11:51:09 +0900, Masahiko Sawada <sawada.mshk@gmail.com> wrote in
<CAD21AoC5rrWSk-V79xjVfYr2UqQYrrCKsXkSxZrN9p5YAaeKJA@mail.gmail.com>
> On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
> <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > 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 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.
..
> Thank you for updating the patch.
> 
> Here are some comments.
> 
> +       Assert(syncrep_parse_result == NULL);
> +
> 
> Why do we need Assert at this point?
> It's possible that syncrep_parse_result is not NULL after setting
> s_s_names by ALTER SYSTEM.

Thank you for pointing it out. It is just a trace of an
assumption no longer useful.

> +               /*
> +                * this memory block will be freed as a part of the
> memory contxt for
> +                * config file processing.
> +                */
> 
> s/contxt/context/

Thanks. I removed whole the comment and the corresponding code
since it's meaningless.

assign_s_s_names causes SEGV when it is called without calling
check_s_s_names. I think that's not the case for this varialbe
because it is unresettable amid a session. It is very uneasy for
me but I don't see a proper means to reset
syncrep_parse_result. MemoryContext deletion hook would work but
it seems to be an overkill for this single use.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..bdd6de0 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
@@ -952,13 +951,30 @@ SyncRepQueueIsOrderedByLSN(int mode) *
===========================================================*/
 
+/*
+ * check_synchronous_standby_names and assign_synchronous_standby_names are to
+ * be used from guc.c. The former generates a result pointed by
+ * syncrep_parse_result in the current memory context as the side effect and
+ * the latter reads it. This won't be a problem as long as the guc variable
+ * synchronous_standby_names cannot be set during a session.
+ */
+boolcheck_synchronous_standby_names(char **newval, void **extra, GucSource source){    int    parse_rc;
+    syncrep_parse_result = NULL;
+    if (*newval != NULL && (*newval)[0] != '\0')    {
+        /*
+         * syncrep_yyparse generates a result on the current memory context as
+         * the side effect and points it using the global
+         * syncrep_prase_result.  We don't clear the pointer even after the
+         * result is invalidated by discarding the context so make sure not to
+         * use it after invalidation.
+         */        syncrep_scanner_init(*newval);        parse_rc = syncrep_yyparse();
syncrep_scanner_finish();
@@ -1015,19 +1031,28 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
             syncrep_parse_result->num_sync, list_length(syncrep_parse_result->members)),
errhint("Specifymore 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);    }    return true;}void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+    /* Free the old SyncRepConfig if exists */
+    if (SyncRepConfig)
+        SyncRepFreeConfig(SyncRepConfig);
+
+    /* Copy the parsed config into TopMemoryContext if exists */
+    if (syncrep_parse_result)
+        SyncRepConfig = SyncRepCopyConfig(syncrep_parse_result);
+    else
+        SyncRepConfig = 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);/*