At Wed, 20 Apr 2016 11:51:09 +0900, Masahiko Sawada <[email protected]>
wrote in <cad21aoc5rrwsk-v79xjvfyr2uqqyrrcksxksxzrn9p5yaae...@mail.gmail.com>
> On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
> <[email protected]> wrote:
> > At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada <[email protected]>
> > wrote in
> > <cad21aocol6bcc+fwnczh_xpgtwc_otnvshmx6_uacu7bwb1...@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.
+ */
+
bool
check_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("Specify more 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;
+}
+
+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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
