Hello, attached is the new version v8.
At Tue, 26 Apr 2016 11:02:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI
<horiguchi.kyot...@lab.ntt.co.jp> wrote in
<20160426.110225.35506931.horiguchi.kyot...@lab.ntt.co.jp>
> At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote in
> <476.1461420...@sss.pgh.pa.us>
> > Amit Kapila <amit.kapil...@gmail.com> writes:
> > > The main point for this improvement is that the handling for guc s_s_names
> > > is not similar to what we do for other somewhat similar guc's and which
> > > causes in-efficiency in non-hot code path (less used code).
> >
> > This is not about efficiency, this is about correctness. The proposed
> > v7 patch is flat out not acceptable, not now and not for 9.7 either,
> > because it introduces a GUC assign hook that can easily fail (eg, through
> > out-of-memory for the copy step). Assign hook functions need to be
> > incapable of failure. I do not see any good reason why this one cannot
> > satisfy that requirement, either. It just needs to make use of the
> > "extra" mechanism to pass back an already-suitably-long-lived result from
> > check_synchronous_standby_names. See check_timezone_abbreviations/
> > assign_timezone_abbreviations for a model to follow.
>
> I had already seen there before the v7 and had the same feeling
> below in mind but packing in a blob needs to use other than List
> to hold the name list (just should be an array) and it is
> followed by the necessity of many changes in where the list is
> accessed. But the result is hopeless as you mentioned :(
>
> > You are going to
> > need to find a way to package the parse result into a single malloc'd
> > blob, though, because that's as much as guc.c can keep track of for an
> > "extra" value.
>
> Ok, I'll post the v8 with the blob solution sooner.
Hmm. It was way easier than I thought. The attached v8 patch does,
- Changed SyncRepConfigData from a struct using liked list to a
blob. Since the former struct is useful in parsing, it is still
used and converted into the latter form in check_s_s_names.
- Make assign_s_s_names not to do nothing other than just
assigning SyncRepConfig.
- Change SyncRepGetSyncStandbys to read the latter form of
configuration.
- SyncRepFreeConfig is removed since it is no longer needed.
It passes both make check and recovery/make check.
regards,
--
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..376fe51 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -361,11 +361,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.
@@ -575,7 +570,7 @@ SyncRepGetSyncStandbys(bool *am_sync)
if (am_sync != NULL)
*am_sync = false;
- lowest_priority = list_length(SyncRepConfig->members);
+ lowest_priority = SyncRepConfig->nmembers;
next_highest_priority = lowest_priority + 1;
/*
@@ -730,9 +725,7 @@ SyncRepGetSyncStandbys(bool *am_sync)
static int
SyncRepGetStandbyPriority(void)
{
- List *members;
- ListCell *l;
- int priority = 0;
+ int priority;
bool found = false;
/*
@@ -745,12 +738,9 @@ SyncRepGetStandbyPriority(void)
if (!SyncStandbysDefined())
return 0;
- members = SyncRepConfig->members;
- foreach(l, members)
+ for (priority = 1 ; priority <= SyncRepConfig->nmembers ; priority++)
{
- char *standby_name = (char *) lfirst(l);
-
- priority++;
+ char *standby_name = SyncRepConfig->members[priority - 1];
if (pg_strcasecmp(standby_name, application_name) == 0 ||
pg_strcasecmp(standby_name, "*") == 0)
@@ -867,50 +857,6 @@ 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)
-{
- if (!config)
- return;
-
- list_free_deep(config->members);
- pfree(config);
-}
-
#ifdef USE_ASSERT_CHECKING
static bool
SyncRepQueueIsOrderedByLSN(int mode)
@@ -956,9 +902,16 @@ bool
check_synchronous_standby_names(char **newval, void **extra, GucSource source)
{
int parse_rc;
+ SyncRepConfigData *pconf;
+ int i;
+ ListCell *lc;
if (*newval != NULL && (*newval)[0] != '\0')
{
+ /*
+ * syncrep_yyparse generates a result on the current memory context as
+ * the side effect and points it using syncrep_prase_result.
+ */
syncrep_scanner_init(*newval);
parse_rc = syncrep_yyparse();
syncrep_scanner_finish();
@@ -1016,18 +969,35 @@ check_synchronous_standby_names(char **newval, void **extra, GucSource source)
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);
- }
+ /* Convert SyncRepConfig into the packed struct fit to guc extra */
+ pconf = (SyncRepConfigData *)
+ malloc(SizeOfSyncRepConfig(
+ list_length(syncrep_parse_result->members)));
+ pconf->num_sync = syncrep_parse_result->num_sync;
+ pconf->nmembers = list_length(syncrep_parse_result->members);
+ i = 0;
+ foreach (lc, syncrep_parse_result->members)
+ {
+ strncpy(pconf->members[i], (char*) lfirst (lc), NAMEDATALEN - 1);
+ pconf->members[i][NAMEDATALEN - 1] = 0;
+ i++;
+ }
+ *extra = (void *)pconf;
+
+ /* No further need for syncrep_parse_result */
+ syncrep_parse_result = NULL;
+ }
return true;
}
void
+assign_synchronous_standby_names(const char *newval, void *extra)
+{
+ SyncRepConfig = (SyncRepConfigData *) extra;
+}
+
+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..932fa9d 100644
--- a/src/backend/replication/syncrep_gram.y
+++ b/src/backend/replication/syncrep_gram.y
@@ -19,9 +19,9 @@
#include "utils/formatting.h"
/* Result of the parsing is returned here */
-SyncRepConfigData *syncrep_parse_result;
+SyncRepParseData *syncrep_parse_result;
-static SyncRepConfigData *create_syncrep_config(char *num_sync, List *members);
+static SyncRepParseData *create_syncrep_config(char *num_sync, List *members);
/*
* Bison doesn't allocate anything that needs to live across parser calls,
@@ -43,7 +43,7 @@ static SyncRepConfigData *create_syncrep_config(char *num_sync, List *members);
{
char *str;
List *list;
- SyncRepConfigData *config;
+ SyncRepParseData *config;
}
%token <str> NAME NUM
@@ -72,11 +72,11 @@ standby_name:
;
%%
-static SyncRepConfigData *
+static SyncRepParseData *
create_syncrep_config(char *num_sync, List *members)
{
- SyncRepConfigData *config =
- (SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+ SyncRepParseData *config =
+ (SyncRepParseData *) palloc(sizeof(SyncRepParseData));
config->num_sync = atoi(num_sync);
config->members = members;
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 60856dd..cccc8eb 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..6197308 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -33,16 +33,30 @@
#define SYNC_REP_WAIT_COMPLETE 2
/*
+ * Struct for parsing synchronous_standby_names
+ */
+typedef struct SyncRepParseData
+{
+ int num_sync; /* number of sync standbys that we need to wait for */
+ List *members; /* list of names of potential sync standbys */
+} SyncRepParseData;
+
+/*
* Struct for the configuration of synchronous replication.
*/
typedef struct SyncRepConfigData
{
int num_sync; /* number of sync standbys that we need to wait for */
- List *members; /* list of names of potential sync standbys */
+ int nmembers; /* number of members in the following list */
+ char members[FLEXIBLE_ARRAY_MEMBER][NAMEDATALEN];/* list of names of
+ * potential sync
+ * standbys */
} SyncRepConfigData;
-extern SyncRepConfigData *syncrep_parse_result;
-extern SyncRepConfigData *SyncRepConfig;
+#define SizeOfSyncRepConfig(n) \
+ (offsetof(SyncRepConfigData, members) + (n) * NAMEDATALEN)
+
+extern SyncRepParseData *syncrep_parse_result;
/* user-settable parameters for synchronous replication */
extern char *SyncRepStandbyNames;
@@ -59,13 +73,12 @@ 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);
/* 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
