On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane <t...@sss.pgh.pa.us> wrote:
> Jeff Janes <jeff.ja...@gmail.com> writes:
>> When I compile now without cassert, I get the compiler warning:
>
>> syncrep.c: In function 'SyncRepUpdateConfig':
>> syncrep.c:878:6: warning: variable 'parse_rc' set but not used
>> [-Wunused-but-set-variable]
>
> If there's a good reason for that to be an Assert, I don't see it.
> There are no callers of SyncRepUpdateConfig that look like they
> need to, or should expect not to have to, tolerate errors.
> I think the way to fix this is to turn the Assert into a plain
> old test-and-ereport-ERROR.
>

I've changed the draft patch Amit implemented so that it doesn't parse
twice(check_hook and assign_hook).
So assertion that was in assign_hook is no longer necessary.

Please find attached.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index cb6b5e5..4733dc1 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 */
@@ -362,7 +363,7 @@ SyncRepInitConfig(void)
 	int			priority;
 
 	/* Update the config data of synchronous replication */
-	SyncRepFreeConfig(SyncRepConfig);
+	SyncRepFreeConfig(SyncRepConfig, true);
 	SyncRepConfig = NULL;
 	SyncRepUpdateConfig();
 
@@ -897,13 +898,16 @@ SyncRepUpdateConfig(void)
  * Free a previously-allocated config data of synchronous replication.
  */
 void
-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
 {
 	if (!config)
 		return;
 
 	list_free_deep(config->members);
-	pfree(config);
+
+	if (itself)
+		free(config);
+
 }
 
 #ifdef USE_ASSERT_CHECKING
@@ -954,6 +958,10 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
 
 	if (*newval != NULL && (*newval)[0] != '\0')
 	{
+		MemoryContext oldcontext;
+
+		oldcontext = MemoryContextSwitchTo(TopMemoryContext);
+
 		syncrep_scanner_init(*newval);
 		parse_rc = syncrep_yyparse();
 		syncrep_scanner_finish();
@@ -1012,17 +1020,36 @@ 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 *)syncrep_parse_result;
+
+		MemoryContextSwitchTo(oldcontext);
 	}
 
 	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.
+	 */
+	if (SyncRepConfig)
+		SyncRepFreeConfig(SyncRepConfig, false);
+
+	SyncRepConfig = myextra;
+
+	return;
+}
+
+void
 assign_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..de25a40 100644
--- a/src/backend/replication/syncrep_gram.y
+++ b/src/backend/replication/syncrep_gram.y
@@ -76,7 +76,7 @@ static SyncRepConfigData *
 create_syncrep_config(char *num_sync, List *members)
 {
 	SyncRepConfigData *config =
-		(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+		(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
 
 	config->num_sync = atoi(num_sync);
 	config->members = members;
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index e4a0119..a9c982b 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..dadbd23 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -60,12 +60,13 @@ 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);
 
 /* 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

Reply via email to