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

At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila <amit.kapila16@gmail.com> wrote in
<CAA4eK1+Qsw2hLEhrEBvveKC91uZQhDce9i-4dB8VPz87Ciz+OQ@mail.gmail.com>
> On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada <sawada.mshk@gmail.com>
> wrote:
> >
> > On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
> > <horiguchi.kyotaro@lab.ntt.co.jp> wrote:
> > > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao <masao.fujii@gmail.com>
> wrote in <CAHGQGwH7F5gWfdCT71Ucix_w+8ipR1Owzv9k4VnA1fcMYyfr6w@mail.gmail.com
> >
> > >> > Yes, this is what I was trying to explain to Fujii-san upthread and
> I have
> > >> > also verified that the same works on Windows.
> > >>
> > >> Oh, okay, understood. Thanks for explaining that!
> > >>
> > >> > I think one point which we
> > >> > should try to ensure in this patch is whether it is good to use
> > >> > TopMemoryContext to allocate the memory in the check or assign
> function or
> > >> > should we allocate some temporary context (like we do in
> load_tzoffsets())
> > >> > to perform parsing and then delete the same at end.
> > >>
> > >> Seems yes if some memories are allocated by palloc and they are not
> > >> free'd while parsing s_s_names.
> > >>
> > >> Here are another comment for the patch.
> > >>
> > >> -SyncRepFreeConfig(SyncRepConfigData *config)
> > >> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
> > >>
> > >> SyncRepFreeConfig() was extended so that it accepts the second boolean
> > >> argument. But it's always called with the second argument = false. So,
> > >> I just wonder why that second argument is required.
> > >>
> > >>     SyncRepConfigData *config =
> > >> -        (SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
> > >> +        (SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> > >>
> > >> Why should we use malloc instead of palloc here?
> > >>
> > >> *If* we use malloc, its return value must be checked.
> > >
> > > Because it should live irrelevant to any memory context, as guc
> > > values are so. guc.c provides guc_malloc for this purpose, which
> > > is a malloc having some simple error handling, so having
> > > walsender_malloc would be reasonable.
> > >
> > > I don't think it's good to use TopMemoryContext for syncrep
> > > parser. syncrep_scanner.l uses palloc. This basically causes a
> > > memory leak on all postgres processes.
> > >
> > > It might be better if the parser works on the current memory
> > > context and the caller copies the result on the malloc'ed
> > > memory. But some list-creation functions using palloc..
> 
> 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?

> > Changing
> > > SyncRepConfigData.members to be char** would be messier..
> >
> > SyncRepGetSyncStandby logic assumes deeply that the sync standby names
> > are constructed as a list.
> > I think that it would entail a radical change in SyncRepGetStandby
> > Another idea is to prepare the some functions that allocate/free
> > element of list using by malloc, free.
> >
> 
> Yeah, that could be another way of doing it, but seems like much more work.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..3778c94 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,61 @@ 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, bool itself, MemoryContext cxt){
-    int    parse_rc;
+    MemoryContext oldcxt = NULL;
-    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;
+    if (cxt)
+        oldcxt = MemoryContextSwitchTo(cxt);
+    list_free_deep(config->members);
+
+    if(oldcxt)
+        MemoryContextSwitchTo(oldcxt);
+
+    if (itself)
+        free(config);}/*
- * Free a previously-allocated config data of synchronous replication.
+ * Returns a copy of a replication config data in the specified memory
+ * context. Note that only the top block should be malloc'ed, because it is
+ * assumed to be freed by set_ */
-void
-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepConfigData *
+SyncRepCopyConfig(SyncRepConfigData *oldconfig, MemoryContext targetcxt){
-    if (!config)
-        return;
+    MemoryContext        oldcxt;
+    SyncRepConfigData  *newconfig;
+    ListCell           *lc;
-    list_free_deep(config->members);
-    pfree(config);
+    if (!oldconfig)
+        return NULL;
+
+    oldcxt = MemoryContextSwitchTo(targetcxt);
+
+    newconfig = (SyncRepConfigData *) malloc(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 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
@@ -959,12 +969,32 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)    if (*newval !=
NULL&& (*newval)[0] != '\0')    {
 
+        MemoryContext oldcxt;
+        MemoryContext repparse_cxt;
+
+        /*
+         * The result of syncrep_yyparse should live for the lifetime of the
+         * process and syncrep_yyparse may abandon a certain amount of
+         * palloc'ed memory * blocks. So we provide a temporary memory context
+         * for the playground of syncrep_yyparse and copy the result to
+         * TopMmeoryContext.
+         */
+        repparse_cxt = AllocSetContextCreate(CurrentMemoryContext,
+                                             "syncrep parser",
+                                             ALLOCSET_DEFAULT_MINSIZE,
+                                             ALLOCSET_DEFAULT_INITSIZE,
+                                             ALLOCSET_DEFAULT_MAXSIZE);
+        oldcxt = MemoryContextSwitchTo(repparse_cxt);
+        syncrep_scanner_init(*newval);        parse_rc = syncrep_yyparse();        syncrep_scanner_finish();
+        MemoryContextSwitchTo(oldcxt);
+        if (parse_rc != 0)        {
+            MemoryContextDelete(repparse_cxt);            GUC_check_errcode(ERRCODE_SYNTAX_ERROR);
GUC_check_errdetail("synchronous_standby_namesparser returned %d",                                parse_rc);
 
@@ -1017,17 +1047,38 @@ 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.
+         * syncrep_yyparse sets the global syncrep_parse_result.
+         * Save syncrep_parse_result to extra in order to use it in
+         * assign_synchronous_standby_names hook as well.         */
-        SyncRepFreeConfig(syncrep_parse_result);
+        *extra = (void *)SyncRepCopyConfig(syncrep_parse_result,
+                                           TopMemoryContext);
+        MemoryContextDelete(repparse_cxt);    }    return true;}void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+    SyncRepConfigData *myextra = extra;
+
+    /*
+     * Free members of SyncRepConfig if it already refers somewhere, but
+     * SyncRepConfig itself is freed by set_extra_field. The content of
+     * SyncRepConfit is on TopMemoryContext. See
+     * check_synchronous_standby_names.
+     */
+    if (SyncRepConfig)
+        SyncRepFreeConfig(SyncRepConfig, false, TopMemoryContext);
+
+    SyncRepConfig = myextra;
+
+    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..97368f8 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -59,13 +59,16 @@ 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 void SyncRepFreeConfig(SyncRepConfigData *config, bool itself,
+                                     MemoryContext targetcxt);
+extern SyncRepConfigData *SyncRepCopyConfig(SyncRepConfigData *oldconfig,
+                                            MemoryContext targetcxt);/* 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);/*