On Mon, Apr 11, 2016 at 8:47 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
> On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada <sawada.m...@gmail.com>
> wrote:
>> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao <masao.fu...@gmail.com> wrote:
>>> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada <sawada.m...@gmail.com>
>>> wrote:
>>>> 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.
>>>
>>> Thanks for the patch!
>>>
>>> When I emptied s_s_names, reloaded the configration file, set it to
>>> 'standby1'
>>> and reloaded the configuration file again, the master crashed with
>>> the following error.
>>>
>>> *** glibc detected *** postgres: wal sender process postgres [local]
>>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>>> 0x00000000024d9a40 ***
>>> ======= Backtrace: =========
>>> *** glibc detected *** postgres: wal sender process postgres [local]
>>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>>> 0x00000000024d9a40 ***
>>> /lib64/libc.so.6[0x3be8e75f4e]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>>> ======= Backtrace: =========
>>> /lib64/libc.so.6[0x3be8e75f4e]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x97dae2]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(set_config_option+0x12cb)[0x982242]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(SetConfigOption+0x4b)[0x9827ff]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x988b4e]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x98af40]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(ProcessConfigFile+0x9f)[0x98a98b]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b50fd]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7b359c]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(exec_replication_command+0x1a7)[0x7b47b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostgresMain+0x772)[0x8141b6]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x7896f7]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x788e62]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x785544]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x6ce12e]
>>> /lib64/libc.so.6(__libc_start_main+0xfd)[0x3be8e1ed5d]
>>> postgres: wal sender process postgres [local] streaming
>>> 0/3015F18(PostmasterMain+0x1134)[0x784c08]
>>> postgres: wal sender process postgres [local] streaming 0/3015F18[0x467e99]
>>>
>>
>> Thank you for reviewing.
>>
>> SyncRepUpdateConfig() seems to be no longer necessary.
>
> Really? I was thinking that something like that function needs to
> be called at the beginning of a backend and walsender in
> EXEC_BACKEND case. No?
>
Hmm, in EXEC_BACKEND case, I guess that each child process calls
read_nondefault_variables that parses and validates these
configuration parameters in SubPostmasterMain.
Previous patch didn't apply to HEAD cleanly, attached updated version.
Regards,
--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..1eb3ec5 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,18 @@ SyncRepUpdateSyncStandbysDefined(void)
}
/*
- * Parse synchronous_standby_names and update the config data
- * of synchronous standbys.
- */
-void
-SyncRepUpdateConfig(void)
-{
- int parse_rc;
-
- if (!SyncStandbysDefined())
- 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;
-}
-
-/*
* 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
@@ -959,6 +926,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();
@@ -1017,17 +988,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..e1706f6 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -59,13 +59,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
