At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <sawada.m...@gmail.com>
wrote in <cad21aocol6bcc+fwnczh_xpgtwc_otnvshmx6_uacu7bwb1...@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_names parser 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;
+}
+
+void
assign_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);
extern bool check_synchronous_standby_names(char **newval, void **extra, GucSource source);
+extern void assign_synchronous_standby_names(const char *newval, void *extra);
extern void assign_synchronous_commit(int newval, void *extra);
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
