Re: [HACKERS] Support for N synchronous standby servers - take 2

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> At Wed, 27 Apr 2016 18:05:26 -0400, Tom Lane  wrote in 
> <3167.1461794...@sss.pgh.pa.us>
>> My inclination is to just rip out the warning.

> Is there anyone object to removing the warining?

Hearing no objections, done.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Hello,

At Wed, 27 Apr 2016 18:05:26 -0400, Tom Lane  wrote in 
<3167.1461794...@sss.pgh.pa.us>
> Kyotaro HORIGUCHI  writes:
> > Sorry, I have attached an empty patch. This is another one that should
> > be with content.
> 
> I pushed this after whacking it around some, and cleaning up some
> sort-of-related problems in the syncrep parser/lexer.

Thank you for pushing this (with improvements) and improvements
of synchronous_standby_names. I agree to the discussion that
standby names should have restriction not to break possible
extension to be happen near future.

> There remains a point that I'm not very happy about, which is the code
> in check_synchronous_standby_names to emit a WARNING if the num_sync
> setting is too large.  That's a pretty bad compromise: we should either
> decide that the case is legal or that it is not.  If it's legal, people
> who are correctly using the case will not thank us for logging a WARNING
> every single time the postmaster gets a SIGHUP (and those who aren't using
> it correctly will have their systems freezing up, warning or no warning).
> If it's not legal, we should make it an error not a warning.

This specification makes the code a bit complex and makes the
document a bit less understandable. It seems to me somewhat
suspicious that allowing duplcate (potentially synchronous)
walrecivers is so useful as to justify such disadvantages.

In spite of this, my inclination is also the same as the
following:p rather than making the behavior consistent and clear.

> My inclination is to just rip out the warning.

Is there anyone object to removing the warining?

> But I wonder whether the
> desire to have one doesn't imply that the semantics are poorly chosen
> and should be revisited.

We already have abandoned a bit of backward compatibility in this
feature.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> Sorry, I have attached an empty patch. This is another one that should
> be with content.

I pushed this after whacking it around some, and cleaning up some
sort-of-related problems in the syncrep parser/lexer.

There remains a point that I'm not very happy about, which is the code
in check_synchronous_standby_names to emit a WARNING if the num_sync
setting is too large.  That's a pretty bad compromise: we should either
decide that the case is legal or that it is not.  If it's legal, people
who are correctly using the case will not thank us for logging a WARNING
every single time the postmaster gets a SIGHUP (and those who aren't using
it correctly will have their systems freezing up, warning or no warning).
If it's not legal, we should make it an error not a warning.

My inclination is to just rip out the warning.  But I wonder whether the
desire to have one doesn't imply that the semantics are poorly chosen
and should be revisited.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> Sorry, I have attached an empty patch. This is another one that should
> be with content.

I started to review this, and in passing came across this gem in
syncrep_scanner.l:


/*
  * flex emits a yy_fatal_error() function that it calls in response to
  * critical errors like malloc failure, file I/O errors, and detection of
  * internal inconsistency.  That function prints a message and calls exit().
  * Mutate it to instead call ereport(FATAL), which terminates this process.
  *
  * The process that causes this fatal error should be terminated.
  * Otherwise it has to abandon the new setting value of
  * synchronous_standby_names and keep running with the previous one
  * while the other processes switch to the new one.
  * This inconsistency of the setting that each process is based on
  * can cause a serious problem. Though it's basically not good idea to
  * use FATAL here because it can take down the postmaster,
  * we should do that in order to avoid such an inconsistency.
  */
#undef fprintf
#define fprintf(file, fmt, msg) syncrep_flex_fatal(fmt, msg)

static void
syncrep_flex_fatal(const char *fmt, const char *msg)
{
ereport(FATAL, (errmsg_internal("%s", msg)));
}


This is the faultiest reasoning possible.  There are a hundred reasons why
a process might fail to absorb a GUC setting, and causing just one such
code path to FATAL out is not going to improve system stability one bit.

If you think it is absolutely imperative that all processes in the system
have identical synchronous_standby_names settings, then we need to make
it be PGC_POSTMASTER, not indulge in half-baked non-solutions like this.
But I'd like to know why that is so essential.  It looks to me like what
matters is only whether each individual walsender thinks its client is
a sync standby, and so inconsistent settings between different walsenders
don't really matter.  Which is a good thing, because if it's to remain
SIGHUP, you can't promise that they'll all absorb a new value at the same
instant anyway.

In short, I don't see any good reason not to make this be a plain ERROR
like it is in every other scanner in the backend.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

On Wed, Apr 27, 2016 at 10:14 AM, Kyotaro HORIGUCHI
 wrote:
> I just added a comment in the v9.

Sorry, I have attached an empty patch. This is another one that should
be with content.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


fix_sync_rep_update_conf_v9.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Hello,

At Tue, 26 Apr 2016 09:57:50 +0530, Amit Kapila  wrote 
in 
> > > > Amit Kapila  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.
> 
> 
> It seems to me that similar problem can be there
> for assign_pgstat_temp_directory() as it can also lead to "out of memory"
> error.  However, in general I understand your concern and I think we should
> avoid any such failure in assign functions.

I noticed that forgetting error handling of malloc then searched
for the callers of guc_malloc just now and found the same
thing. This should be addressed as another issue.

> > > > 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,
...
> + /* Convert SyncRepConfig into the packed struct fit to guc extra */
> 
> + pconf = (SyncRepConfigData *)
> 
> + malloc(SizeOfSyncRepConfig(
> 
> +   list_length(syncrep_parse_result->members)));
> 
> I think there should be a check for malloc failure in above code.

Yes, I'm ashamed to have forgotten what I mentioned just
before. Added the same thing with guc_malloc. The error is at
ERROR since parsing GUC files should continue on parse errors
(and seeing check_log_destination).


> + /* No further need for syncrep_parse_result */
> 
> + syncrep_parse_result = NULL;
> 
> Isn't this a memory leak?  Shouldn't we need to free the corresponding
> memory as well.

It is palloc'ed on the current context, which AFAICS would be
'config file processing' or 'PortalHeapMemory'for the ALTER
SYSTEM case. Both of them are rather short-living. I don't think
that leaving them is a problem on both of the cases and there's
no point freeing only it among those (if any) allocated in the
generated code by bison and flex... I suppose.

I just added a comment in the v9.

|   * No further need for syncrep_parse_result. The memory blocks are
|   * released along with the deletion of the current context.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Tue, Apr 26, 2016 at 9:15 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:

> Hello, attached is the new version v8.
>
> At Tue, 26 Apr 2016 11:02:25 +0900 (Tokyo Standard Time), Kyotaro
> HORIGUCHI  wrote in <
> 20160426.110225.35506931.horiguchi.kyot...@lab.ntt.co.jp>
> > At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane  wrote
> in <476.1461420...@sss.pgh.pa.us>
> > > Amit Kapila  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.


It seems to me that similar problem can be there
for assign_pgstat_temp_directory() as it can also lead to "out of memory"
error.  However, in general I understand your concern and I think we should
avoid any such failure in assign functions.


>   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.
>
>
+ /* Convert SyncRepConfig into the packed struct fit to guc extra */

+ pconf = (SyncRepConfigData *)

+ malloc(SizeOfSyncRepConfig(

+   list_length(syncrep_parse_result->members)));

I think there should be a check for malloc failure in above code.


+ /* No further need for syncrep_parse_result */

+ syncrep_parse_result = NULL;

Isn't this a memory leak?  Shouldn't we need to free the corresponding
memory as well.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Hello, attached is the new version v8.

At Tue, 26 Apr 2016 11:02:25 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160426.110225.35506931.horiguchi.kyot...@lab.ntt.co.jp>
> At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane  wrote in 
> <476.1461420...@sss.pgh.pa.us>
> > Amit Kapila  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
 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane  wrote in 
<476.1461420...@sss.pgh.pa.us>
> Amit Kapila  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.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Tom Lane]

Amit Kapila  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.  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.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Sat, Apr 23, 2016 at 5:20 PM, Michael Paquier 
wrote:
>
> On Sat, Apr 23, 2016 at 7:44 PM, Amit Kapila 
wrote:
> > On Wed, Apr 20, 2016 at 12:46 PM, Kyotaro HORIGUCHI
> >  wrote:
> >>
> >>
> >> 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.
> >>
> >
> > Is it because syncrep_parse_result is not freed after creating a copy
of it
> > in assign_synchronous_standby_names()?  If it so, then I think we need
to
> > call SyncRepFreeConfig(syncrep_parse_result); in
> > assign_synchronous_standby_names at below place:
> >
> > + /* Copy the parsed config into TopMemoryContext if exists */
> >
> > + if (syncrep_parse_result)
> >
> > + SyncRepConfig = SyncRepCopyConfig(syncrep_parse_result);
> >
> > Could you please explain how to trigger the scenario where you have seen
> > SEGV?
>
> Seeing this discussion moving on, I am wondering if we should not
> discuss those improvements for 9.7.
>

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).  So, we can
push this improvement to 9.7, but OTOH we can also consider it as a
non-beta blocker issue and see if we can make this code path better in the
mean time.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Sat, Apr 23, 2016 at 7:44 PM, Amit Kapila  wrote:
> On Wed, Apr 20, 2016 at 12:46 PM, Kyotaro HORIGUCHI
>  wrote:
>>
>>
>> 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.
>>
>
> Is it because syncrep_parse_result is not freed after creating a copy of it
> in assign_synchronous_standby_names()?  If it so, then I think we need to
> call SyncRepFreeConfig(syncrep_parse_result); in
> assign_synchronous_standby_names at below place:
>
> + /* Copy the parsed config into TopMemoryContext if exists */
>
> + if (syncrep_parse_result)
>
> + SyncRepConfig = SyncRepCopyConfig(syncrep_parse_result);
>
> Could you please explain how to trigger the scenario where you have seen
> SEGV?

Seeing this discussion moving on, I am wondering if we should not
discuss those improvements for 9.7. We are getting close to beta 1,
and this is clearly not a bug, and it's not like HEAD is broken. So I
think that we should not take the risk to make the code unstable at
this stage.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Wed, Apr 20, 2016 at 12:46 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
>
>
> 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.
>

Is it because syncrep_parse_result is not freed after creating a copy of it
in assign_synchronous_standby_names()?  If it so, then I think we need to
call SyncRepFreeConfig(syncrep_parse_result); in
assign_synchronous_standby_names at below place:

+ /* Copy the parsed config into TopMemoryContext if exists */

+ if (syncrep_parse_result)

+ SyncRepConfig = SyncRepCopyConfig(syncrep_parse_result);

Could you please explain how to trigger the scenario where you have seen
SEGV?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Wed, 20 Apr 2016 11:51:09 +0900, Masahiko Sawada  
wrote in 
> On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
>  wrote:
> > At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada  
> > wrote in 
> > 
> >> 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 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Mon, Apr 18, 2016 at 2:15 PM, Kyotaro HORIGUCHI
 wrote:
> At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada  
> wrote in 
>> >> 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.
>

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.

+   /*
+* this memory block will be freed as a part of the
memory contxt for
+* config file processing.
+*/

s/contxt/context/

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Fri, 15 Apr 2016 17:36:57 +0900, Masahiko Sawada  
wrote in 
> >> 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;
 			

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Sat, 16 Apr 2016 12:50:30 +0530, Amit Kapila  wrote 
in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Fri, Apr 15, 2016 at 11:30 AM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
>
> At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila 
wrote :
> >
> > 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..
>

+ newconfig = (SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
Is there a reason to use malloc here, can't we use palloc directly?  Also
for both the functions SyncRepCopyConfig() and SyncRepFreeConfig(), if we
directly use TopMemoryContext inside the function (if required) rather than
taking it as argument, then it will simplify the code a lot.

+SyncRepFreeConfig(SyncRepConfigData *config, bool itself, MemoryContext
cxt)

Do we really need 'bool itself' parameter in above function?

+ if (cxt)

+ oldcxt = MemoryContextSwitchTo(cxt);

+ list_free_deep(config->members);

+

+ if(oldcxt)

+ MemoryContextSwitchTo(oldcxt);
Why do you need MemoryContextSwitchTo for freeing members?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Fri, Apr 15, 2016 at 3:00 PM, Kyotaro HORIGUCHI
 wrote:
> At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila  
> wrote in 
>> On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada 
>> wrote:
>> >
>> > On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
>> >  wrote:
>> > > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao 
>> wrote in > >
>> > >> > Yes, this is what I was trying to explain to Fujii-san upthread and
>> I have
>> > >> > also verified that the same works on Windows.
>> > >>
>> > >> Oh, okay, understood. Thanks for explaining that!
>> > >>
>> > >> > I think one point which we
>> > >> > should try to ensure in this patch is whether it is good to use
>> > >> > TopMemoryContext to allocate the memory in the check or assign
>> function or
>> > >> > should we allocate some temporary context (like we do in
>> load_tzoffsets())
>> > >> > to perform parsing and then delete the same at end.
>> > >>
>> > >> Seems yes if some memories are allocated by palloc and they are not
>> > >> free'd while parsing s_s_names.
>> > >>
>> > >> Here are another comment for the patch.
>> > >>
>> > >> -SyncRepFreeConfig(SyncRepConfigData *config)
>> > >> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
>> > >>
>> > >> SyncRepFreeConfig() was extended so that it accepts the second boolean
>> > >> argument. But it's always called with the second argument = false. So,
>> > >> I just wonder why that second argument is required.
>> > >>
>> > >> SyncRepConfigData *config =
>> > >> -(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
>> > >> +(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
>> > >>
>> > >> Why should we use malloc instead of palloc here?
>> > >>
>> > >> *If* we use malloc, its return value must be checked.
>> > >
>> > > Because it should live irrelevant to any memory context, as guc
>> > > values are so. guc.c provides guc_malloc for this purpose, which
>> > > is a malloc having some simple error handling, so having
>> > > walsender_malloc would be reasonable.
>> > >
>> > > I don't think it's good to use TopMemoryContext for syncrep
>> > > parser. syncrep_scanner.l uses palloc. This basically causes a
>> > > memory leak on all postgres processes.
>> > >
>> > > It might be better if the parser works on the current memory
>> > > context and the caller copies the result on the malloc'ed
>> > > memory. But some list-creation functions using palloc..
>>
>> 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.

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.

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Fri, 15 Apr 2016 08:52:56 +0530, Amit Kapila  wrote 
in 
> On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada 
> wrote:
> >
> > On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
> >  wrote:
> > > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao 
> wrote in  >
> > >> > Yes, this is what I was trying to explain to Fujii-san upthread and
> I have
> > >> > also verified that the same works on Windows.
> > >>
> > >> Oh, okay, understood. Thanks for explaining that!
> > >>
> > >> > I think one point which we
> > >> > should try to ensure in this patch is whether it is good to use
> > >> > TopMemoryContext to allocate the memory in the check or assign
> function or
> > >> > should we allocate some temporary context (like we do in
> load_tzoffsets())
> > >> > to perform parsing and then delete the same at end.
> > >>
> > >> Seems yes if some memories are allocated by palloc and they are not
> > >> free'd while parsing s_s_names.
> > >>
> > >> Here are another comment for the patch.
> > >>
> > >> -SyncRepFreeConfig(SyncRepConfigData *config)
> > >> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
> > >>
> > >> SyncRepFreeConfig() was extended so that it accepts the second boolean
> > >> argument. But it's always called with the second argument = false. So,
> > >> I just wonder why that second argument is required.
> > >>
> > >> SyncRepConfigData *config =
> > >> -(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
> > >> +(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> > >>
> > >> Why should we use malloc instead of palloc here?
> > >>
> > >> *If* we use malloc, its return value must be checked.
> > >
> > > Because it should live irrelevant to any memory context, as guc
> > > values are so. guc.c provides guc_malloc for this purpose, which
> > > is a malloc having some simple error handling, so having
> > > walsender_malloc would be reasonable.
> > >
> > > I don't think it's good to use TopMemoryContext for syncrep
> > > parser. syncrep_scanner.l uses palloc. This basically causes a
> > > memory leak on all postgres processes.
> > >
> > > It might be better if the parser works on the current memory
> > > context and the caller copies the result on the malloc'ed
> > > memory. But some list-creation functions using palloc..
> 
> 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?

> > Changing
> > > SyncRepConfigData.members to be char** would be messier..
> >
> > SyncRepGetSyncStandby logic assumes deeply that the sync standby names
> > are constructed as a list.
> > I think that it would entail a radical change in SyncRepGetStandby
> > Another idea is to prepare the some functions that allocate/free
> > element of list using by malloc, free.
> >
> 
> Yeah, that could be another way of doing it, but seems like much more work.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index 3c9142e..3778c94 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,61 @@ 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, bool itself, MemoryContext cxt)
 {
-	int	parse_rc;
+	MemoryContext oldcxt = NULL;
 
-	if (!SyncStandbysDefined())
+	if (!config)
 		return;
 
-	/*
-	 * check_synchronous_standby_names() verifies the setting value of
-	 * synchronous_standby_names before this 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Thu, 14 Apr 2016 21:05:40 +0900, Michael Paquier  
wrote in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Thu, Apr 14, 2016 at 1:10 PM, Masahiko Sawada 
wrote:
>
> On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
>  wrote:
> > At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao 
wrote in 
> >> > Yes, this is what I was trying to explain to Fujii-san upthread and
I have
> >> > also verified that the same works on Windows.
> >>
> >> Oh, okay, understood. Thanks for explaining that!
> >>
> >> > I think one point which we
> >> > should try to ensure in this patch is whether it is good to use
> >> > TopMemoryContext to allocate the memory in the check or assign
function or
> >> > should we allocate some temporary context (like we do in
load_tzoffsets())
> >> > to perform parsing and then delete the same at end.
> >>
> >> Seems yes if some memories are allocated by palloc and they are not
> >> free'd while parsing s_s_names.
> >>
> >> Here are another comment for the patch.
> >>
> >> -SyncRepFreeConfig(SyncRepConfigData *config)
> >> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
> >>
> >> SyncRepFreeConfig() was extended so that it accepts the second boolean
> >> argument. But it's always called with the second argument = false. So,
> >> I just wonder why that second argument is required.
> >>
> >> SyncRepConfigData *config =
> >> -(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
> >> +(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> >>
> >> Why should we use malloc instead of palloc here?
> >>
> >> *If* we use malloc, its return value must be checked.
> >
> > Because it should live irrelevant to any memory context, as guc
> > values are so. guc.c provides guc_malloc for this purpose, which
> > is a malloc having some simple error handling, so having
> > walsender_malloc would be reasonable.
> >
> > I don't think it's good to use TopMemoryContext for syncrep
> > parser. syncrep_scanner.l uses palloc. This basically causes a
> > memory leak on all postgres processes.
> >
> > It might be better if the parser works on the current memory
> > context and the caller copies the result on the malloc'ed
> > memory. But some list-creation functions using palloc..

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.


>
> Changing
> > SyncRepConfigData.members to be char** would be messier..
>
> SyncRepGetSyncStandby logic assumes deeply that the sync standby names
> are constructed as a list.
> I think that it would entail a radical change in SyncRepGetStandby
> Another idea is to prepare the some functions that allocate/free
> element of list using by malloc, free.
>

Yeah, that could be another way of doing it, but seems like much more work.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Thu, Apr 14, 2016 at 5:48 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 14 Apr 2016 17:25:39 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
>  wrote in 
> <20160414.172539.34325458.horiguchi.kyot...@lab.ntt.co.jp>
>> Hello,
>>
>> At Thu, 14 Apr 2016 13:24:34 +0900, Michael Paquier 
>>  wrote in 
>> 
>> > On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila  
>> > wrote:
>> > > Yes, this is what I was trying to explain to Fujii-san upthread and I 
>> > > have
>> > > also verified that the same works on Windows.
>> >
>> > If you could, it would be nice as well to check that nothing breaks
>> > with VS when using vcregress recoverycheck.
>
> IPC::Run is not installed on Active Perl on my environment and
> Active state seems to be saying that IPC-Run cannot be compiled
> on Windows. ppm doesn't show IPC-Run. Is there any means to do
> TAP test other than this way?
>
> https://code.activestate.com/ppm/IPC-Run/

IPC::Run is a mandatory dependency I am afraid. You could just
download it from cpan and install it manually in your PERL5LIB path.
That's what I did, and it proves to work just fine.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Thu, Apr 14, 2016 at 5:25 PM, Kyotaro HORIGUCHI
 wrote:
> diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
> index 3d14544..08e2acc 100644
> --- a/src/tools/msvc/vcregress.pl
> +++ b/src/tools/msvc/vcregress.pl
> @@ -548,6 +548,6 @@ sub usage
>  {
> print STDERR
>   "Usage: vcregress.pl ",
> -"
>  [schedule]\n";
> +"
>  [schedule]\n";
> exit(1);
>  }

Right, this is missing modulescheck, bincheck and recoverycheck. All 3
are actually mainly my fault, or perhaps Andrew scored once on
bincheck. Honestly, this is unreadable and that's always tiring to
decrypt it, so why not changing it to something more explicit like the
attached? See by yourself:
$ perl vcregress.pl
Usage: vcregress.pl  [  ]

Options for :
  bincheck   run tests of utilities in src/bin/
  check  deploy instance and run regression tests on it
  contribcheck   run tests of modules in contrib/
  ecpgcheck  run regression tests of ECPG driver
  installcheck   run regression tests on existing instance
  isolationcheck run isolation tests
  modulescheck   run tests of modules in src/test/modules
  plcheckrun tests of PL languages
  recoverycheck  run recovery test suite
  upgradecheck   run tests of pg_upgrade

Options for :
  serial serial mode
  parallel   parallel mode
-- 
Michael
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 3d14544..3348b9f 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -547,7 +547,20 @@ sub InstallTemp
 sub usage
 {
 	print STDERR
-	  "Usage: vcregress.pl ",
-" [schedule]\n";
+		"Usage: vcregress.pl  [  ]\n\n",
+		"Options for :\n",
+		"  bincheck   run tests of utilities in src/bin/\n",
+		"  check  deploy instance and run regression tests on it\n",
+		"  contribcheck   run tests of modules in contrib/\n",
+		"  ecpgcheck  run regression tests of ECPG driver\n",
+		"  installcheck   run regression tests on existing instance\n",
+		"  isolationcheck run isolation tests\n",
+		"  modulescheck   run tests of modules in src/test/modules\n",
+		"  plcheckrun tests of PL languages\n",
+		"  recoverycheck  run recovery test suite\n",
+		"  upgradecheck   run tests of pg_upgrade\n",
+		"\nOptions for :\n",
+		"  serial serial mode\n",
+		"  parallel   parallel mode\n";
 	exit(1);
 }

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Thu, 14 Apr 2016 17:25:39 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160414.172539.34325458.horiguchi.kyot...@lab.ntt.co.jp>
> Hello,
> 
> At Thu, 14 Apr 2016 13:24:34 +0900, Michael Paquier 
>  wrote in 
> 
> > On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila  
> > wrote:
> > > Yes, this is what I was trying to explain to Fujii-san upthread and I have
> > > also verified that the same works on Windows.
> > 
> > If you could, it would be nice as well to check that nothing breaks
> > with VS when using vcregress recoverycheck.

IPC::Run is not installed on Active Perl on my environment and
Active state seems to be saying that IPC-Run cannot be compiled
on Windows. ppm doesn't show IPC-Run. Is there any means to do
TAP test other than this way?

https://code.activestate.com/ppm/IPC-Run/

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Hello,

At Thu, 14 Apr 2016 13:24:34 +0900, Michael Paquier  
wrote in 
> On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila  wrote:
> > Yes, this is what I was trying to explain to Fujii-san upthread and I have
> > also verified that the same works on Windows.
> 
> If you could, it would be nice as well to check that nothing breaks
> with VS when using vcregress recoverycheck.

I failed the test because of not preparing for TAP tests. But
instead, I noticed that vcregress.pl shows a bit wrong help
message.

> >vcregress
> Usage: vcregress.pl 
> 
>  [schedule]

The new messages in the following diff is the same to the regexp
to check the parameter of vcregress.

==
diff --git a/src/tools/msvc/vcregress.pl b/src/tools/msvc/vcregress.pl
index 3d14544..08e2acc 100644
--- a/src/tools/msvc/vcregress.pl
+++ b/src/tools/msvc/vcregress.pl
@@ -548,6 +548,6 @@ sub usage
 {
print STDERR
  "Usage: vcregress.pl ",
-"
 [schedule]\n";
+"
 [schedule]\n";
exit(1);
 }
=

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Thu, Apr 14, 2016 at 1:11 PM, Kyotaro HORIGUCHI
 wrote:
> At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao  wrote 
> in 
>> > Yes, this is what I was trying to explain to Fujii-san upthread and I have
>> > also verified that the same works on Windows.
>>
>> Oh, okay, understood. Thanks for explaining that!
>>
>> > I think one point which we
>> > should try to ensure in this patch is whether it is good to use
>> > TopMemoryContext to allocate the memory in the check or assign function or
>> > should we allocate some temporary context (like we do in load_tzoffsets())
>> > to perform parsing and then delete the same at end.
>>
>> Seems yes if some memories are allocated by palloc and they are not
>> free'd while parsing s_s_names.
>>
>> Here are another comment for the patch.
>>
>> -SyncRepFreeConfig(SyncRepConfigData *config)
>> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
>>
>> SyncRepFreeConfig() was extended so that it accepts the second boolean
>> argument. But it's always called with the second argument = false. So,
>> I just wonder why that second argument is required.
>>
>> SyncRepConfigData *config =
>> -(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
>> +(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
>>
>> Why should we use malloc instead of palloc here?
>>
>> *If* we use malloc, its return value must be checked.
>
> Because it should live irrelevant to any memory context, as guc
> values are so. guc.c provides guc_malloc for this purpose, which
> is a malloc having some simple error handling, so having
> walsender_malloc would be reasonable.
>
> I don't think it's good to use TopMemoryContext for syncrep
> parser. syncrep_scanner.l uses palloc. This basically causes a
> memory leak on all postgres processes.
>
> It might be better if the parser works on the current memory
> context and the caller copies the result on the malloc'ed
> memory. But some list-creation functions using palloc.. Changing
> SyncRepConfigData.members to be char** would be messier..

SyncRepGetSyncStandby logic assumes deeply that the sync standby names
are constructed as a list.
I think that it would entail a radical change in SyncRepGetStandby
Another idea is to prepare the some functions that allocate/free
element of list using by malloc, free.

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila  wrote:
> Yes, this is what I was trying to explain to Fujii-san upthread and I have
> also verified that the same works on Windows.

If you could, it would be nice as well to check that nothing breaks
with VS when using vcregress recoverycheck.
--
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Thu, 14 Apr 2016 12:42:06 +0900, Fujii Masao  wrote 
in 
> > Yes, this is what I was trying to explain to Fujii-san upthread and I have
> > also verified that the same works on Windows.
> 
> Oh, okay, understood. Thanks for explaining that!
> 
> > I think one point which we
> > should try to ensure in this patch is whether it is good to use
> > TopMemoryContext to allocate the memory in the check or assign function or
> > should we allocate some temporary context (like we do in load_tzoffsets())
> > to perform parsing and then delete the same at end.
> 
> Seems yes if some memories are allocated by palloc and they are not
> free'd while parsing s_s_names.
> 
> Here are another comment for the patch.
> 
> -SyncRepFreeConfig(SyncRepConfigData *config)
> +SyncRepFreeConfig(SyncRepConfigData *config, bool itself)
> 
> SyncRepFreeConfig() was extended so that it accepts the second boolean
> argument. But it's always called with the second argument = false. So,
> I just wonder why that second argument is required.
> 
> SyncRepConfigData *config =
> -(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
> +(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));
> 
> Why should we use malloc instead of palloc here?
> 
> *If* we use malloc, its return value must be checked.

Because it should live irrelevant to any memory context, as guc
values are so. guc.c provides guc_malloc for this purpose, which
is a malloc having some simple error handling, so having
walsender_malloc would be reasonable.

I don't think it's good to use TopMemoryContext for syncrep
parser. syncrep_scanner.l uses palloc. This basically causes a
memory leak on all postgres processes.

It might be better if the parser works on the current memory
context and the caller copies the result on the malloc'ed
memory. But some list-creation functions using palloc.. Changing
SyncRepConfigData.members to be char** would be messier..

Any idea?

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Thu, Apr 14, 2016 at 11:45 AM, Amit Kapila  wrote:
> On Wed, Apr 13, 2016 at 1:44 PM, Kyotaro HORIGUCHI
>  wrote:
>>
>> At Wed, 13 Apr 2016 04:43:35 +0900, Fujii Masao 
>> wrote in
>> 
>> > >>> 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.
>> >
>> > SyncRepStandbyNames is passed but SyncRepConfig is not, I think.
>>
>> SyncRepStandbyNames is passed to exec'ed backends by
>> read_nondefault_variables, which calls set_config_option, which
>> calls check/assign_s_s_names then syncrep_yyparse, which sets
>> SyncRepConfig.
>>
>> Since guess battle is a waste of time, I actually built and ran
>> on Windows7 and observed that SyncRepConfig has been set before
>> WalSndLoop starts.
>>
>
> Yes, this is what I was trying to explain to Fujii-san upthread and I have
> also verified that the same works on Windows.

Oh, okay, understood. Thanks for explaining that!

> I think one point which we
> should try to ensure in this patch is whether it is good to use
> TopMemoryContext to allocate the memory in the check or assign function or
> should we allocate some temporary context (like we do in load_tzoffsets())
> to perform parsing and then delete the same at end.

Seems yes if some memories are allocated by palloc and they are not
free'd while parsing s_s_names.

Here are another comment for the patch.

-SyncRepFreeConfig(SyncRepConfigData *config)
+SyncRepFreeConfig(SyncRepConfigData *config, bool itself)

SyncRepFreeConfig() was extended so that it accepts the second boolean
argument. But it's always called with the second argument = false. So,
I just wonder why that second argument is required.

SyncRepConfigData *config =
-(SyncRepConfigData *) palloc(sizeof(SyncRepConfigData));
+(SyncRepConfigData *) malloc(sizeof(SyncRepConfigData));

Why should we use malloc instead of palloc here?

*If* we use malloc, its return value must be checked.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Wed, Apr 13, 2016 at 1:44 PM, Kyotaro HORIGUCHI <
horiguchi.kyot...@lab.ntt.co.jp> wrote:
>
> At Wed, 13 Apr 2016 04:43:35 +0900, Fujii Masao 
wrote in 
> > >>> 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.
> >
> > SyncRepStandbyNames is passed but SyncRepConfig is not, I think.
>
> SyncRepStandbyNames is passed to exec'ed backends by
> read_nondefault_variables, which calls set_config_option, which
> calls check/assign_s_s_names then syncrep_yyparse, which sets
> SyncRepConfig.
>
> Since guess battle is a waste of time, I actually built and ran
> on Windows7 and observed that SyncRepConfig has been set before
> WalSndLoop starts.
>

Yes, this is what I was trying to explain to Fujii-san upthread and I have
also verified that the same works on Windows.  I think one point which we
should try to ensure in this patch is whether it is good to use
TopMemoryContext to allocate the memory in the check or assign function or
should we allocate some temporary context (like we do in load_tzoffsets())
to perform parsing and then delete the same at end.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Wed, 13 Apr 2016 04:43:35 +0900, Fujii Masao  wrote 
in 
> >>> 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.
> 
> SyncRepStandbyNames is passed but SyncRepConfig is not, I think.

SyncRepStandbyNames is passed to exec'ed backends by
read_nondefault_variables, which calls set_config_option, which
calls check/assign_s_s_names then syncrep_yyparse, which sets
SyncRepConfig.

Since guess battle is a waste of time, I actually built and ran
on Windows7 and observed that SyncRepConfig has been set before
WalSndLoop starts.

> LOG:  check_s_s_names(pid=20596, newval=)
> LOG:  assign_s_s_names(pid=20596, newval=, SyncRepConfig=)
> LOG:  read_nondefault_variables(pid=20596)
> LOG:  set_config_option(synchronous_standby_names)(pid=20596)
> LOG:  check_s_s_names(pid=20596, newval=2[standby,sby2,sby3])
> LOG:  assign_s_s_names(pid=20596, newval=2[standby,sby2,sby3], 
> SyncRepConfig=01383598)
> LOG:  WalSndLoop(pid=20596)

By the way, the patch assumes that one check_s_s_names is
followed by exactly one assign_s_s_names. I suppose that myextra
should be handled without such assumption.

Plus, the name myextra should be any saner name..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 12, 2016 at 9:04 AM, Masahiko Sawada  wrote:
> On Mon, Apr 11, 2016 at 8:47 PM, Fujii Masao  wrote:
>> On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada  
>> wrote:
>>> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
 On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
 wrote:
> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
>> Jeff Janes  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:
 0x024d9a40 ***
 === Backtrace: =
 *** glibc detected *** postgres: wal sender process postgres [local]
 streaming 0/3015F18: munmap_chunk(): invalid pointer:
 0x024d9a40 ***
 /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 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Mon, Apr 11, 2016 at 8:47 PM, Fujii Masao  wrote:
> On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada  
> wrote:
>> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
>>> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
>>> wrote:
 On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  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:
>>> 0x024d9a40 ***
>>> === Backtrace: =
>>> *** glibc detected *** postgres: wal sender process postgres [local]
>>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>>> 0x024d9a40 ***
>>> /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 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 11, 2016 at 5:52 PM, Masahiko Sawada  wrote:
> On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
>> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
>> wrote:
>>> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
 Jeff Janes  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:
>> 0x024d9a40 ***
>> === Backtrace: =
>> *** glibc detected *** postgres: wal sender process postgres [local]
>> streaming 0/3015F18: munmap_chunk(): invalid pointer:
>> 0x024d9a40 ***
>> /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?

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Mon, Apr 11, 2016 at 1:31 PM, Fujii Masao  wrote:
> On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  
> wrote:
>> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
>>> Jeff Janes  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:
> 0x024d9a40 ***
> === Backtrace: =
> *** glibc detected *** postgres: wal sender process postgres [local]
> streaming 0/3015F18: munmap_chunk(): invalid pointer:
> 0x024d9a40 ***
> /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.
Attached updated version.

Regards,

--
Masahiko Sawada
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index cb6b5e5..93dd836 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 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  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]

Thanks for the report!

> 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.

Okay, I pushed that change. Thanks for the suggestion!

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 11, 2016 at 10:58 AM, Masahiko Sawada  wrote:
> On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
>> Jeff Janes  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:
0x024d9a40 ***
=== Backtrace: =
*** glibc detected *** postgres: wal sender process postgres [local]
streaming 0/3015F18: munmap_chunk(): invalid pointer:
0x024d9a40 ***
/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]

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Sat, Apr 9, 2016 at 12:32 PM, Tom Lane  wrote:
> Jeff Janes  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 = >walsnds[i];
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index fb091bc..3ce83bf 100644
--- 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Tom Lane]

Jeff Janes  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.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Jeff Janes]

On Wed, Apr 6, 2016 at 1:23 AM, Fujii Masao  wrote:

> Okay, I pushed the patch!
> Many thanks to all involved in the development of this feature!

Thanks, a nice feature.

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]

Cheers,

Jeff


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Thu, Apr 7, 2016 at 5:49 PM, Fujii Masao  wrote:
>
> On Thu, Apr 7, 2016 at 7:29 PM, Amit Kapila 
wrote:
> >
> > So if we go by this each time backend calls pg_stat_get_wal_senders, it
> > needs to do parsing to form SyncRepConfig whether it's changed or not
from
> > previous time.  I understand that this is not a performance critical
path,
> > but still if we can do it in some other optimal way which doesn't hurt
any
> > other path, then it will be better.
>
> So, will you write the patch? Either current implementation or
> the approach you're suggesting works to me. If you really want
> to change the current one, I'm happy to review that.
>

Sorry, I don't have time to complete the patch, but I have written an
initial patch to show you what I have in mind and something on this lines
should work.  I think with such an approach, you don't need to parse for
s_s_names twice (once in check_* and once in syncupdate* function),  you
can refer check_temp_tablespaces() and assign_temp_tablespaces() to see how
to use the work done by check_* function in assign_* function.  Also write
now, I have used TopMemoryContext for allocation in
assign_synchronous_standby_names,  it is better to use guc_malloc or
something similar for allocation as is done in other check_* and assign_*
functions.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


fix_sync_rep_update_conf_v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Fri, Apr 8, 2016 at 4:50 PM, Fujii Masao  wrote:
> On Fri, Apr 8, 2016 at 2:26 PM, Michael Paquier
>  wrote:
>> On Thu, Apr 7, 2016 at 11:43 PM, Fujii Masao  wrote:
>>> On Wed, Apr 6, 2016 at 5:04 PM, Michael Paquier
>>>  wrote:
 On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
  wrote:
> Here are few things I have noticed:
> +   for (i = 0; i < max_wal_senders; i++)
> +   {
> +   walsnd = >walsnds[i];
> No volatile pointer to prevent code reordering?
>
>   */
>  typedef struct WalSnd
>  {
> +   int slotno; /* index of this slot in WalSnd array */
> pid_t   pid;/* this walsender's process id, or 0 */
> slotno is used nowhere.
>
> I'll grab the tests and look at them.

 So I had a look at those tests and finished with the attached:
 - patch 1 adds a reload routine to PostgresNode
 - patch 2 the list of tests.
>>>
>>> Thanks for updating the patches!
>>>
>>> Attached is the refactored version of the patch.
>>
>> Thanks. This looks good to me.
>>
>> .gitattributes complains a bit:
>> $ git diff n_sync --check
>> src/test/recovery/t/007_sync_rep.pl:22: trailing whitespace.
>> +   $self->reload;
>
> Thanks for the review! I've finally pushed the patch.
>

Thank you!

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Fri, Apr 8, 2016 at 2:26 PM, Michael Paquier
 wrote:
> On Thu, Apr 7, 2016 at 11:43 PM, Fujii Masao  wrote:
>> On Wed, Apr 6, 2016 at 5:04 PM, Michael Paquier
>>  wrote:
>>> On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
>>>  wrote:
 Here are few things I have noticed:
 +   for (i = 0; i < max_wal_senders; i++)
 +   {
 +   walsnd = >walsnds[i];
 No volatile pointer to prevent code reordering?

   */
  typedef struct WalSnd
  {
 +   int slotno; /* index of this slot in WalSnd array */
 pid_t   pid;/* this walsender's process id, or 0 */
 slotno is used nowhere.

 I'll grab the tests and look at them.
>>>
>>> So I had a look at those tests and finished with the attached:
>>> - patch 1 adds a reload routine to PostgresNode
>>> - patch 2 the list of tests.
>>
>> Thanks for updating the patches!
>>
>> Attached is the refactored version of the patch.
>
> Thanks. This looks good to me.
>
> .gitattributes complains a bit:
> $ git diff n_sync --check
> src/test/recovery/t/007_sync_rep.pl:22: trailing whitespace.
> +   $self->reload;

Thanks for the review! I've finally pushed the patch.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Thu, Apr 7, 2016 at 11:43 PM, Fujii Masao  wrote:
> On Wed, Apr 6, 2016 at 5:04 PM, Michael Paquier
>  wrote:
>> On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
>>  wrote:
>>> Here are few things I have noticed:
>>> +   for (i = 0; i < max_wal_senders; i++)
>>> +   {
>>> +   walsnd = >walsnds[i];
>>> No volatile pointer to prevent code reordering?
>>>
>>>   */
>>>  typedef struct WalSnd
>>>  {
>>> +   int slotno; /* index of this slot in WalSnd array */
>>> pid_t   pid;/* this walsender's process id, or 0 */
>>> slotno is used nowhere.
>>>
>>> I'll grab the tests and look at them.
>>
>> So I had a look at those tests and finished with the attached:
>> - patch 1 adds a reload routine to PostgresNode
>> - patch 2 the list of tests.
>
> Thanks for updating the patches!
>
> Attached is the refactored version of the patch.

Thanks. This looks good to me.

.gitattributes complains a bit:
$ git diff n_sync --check
src/test/recovery/t/007_sync_rep.pl:22: trailing whitespace.
+   $self->reload;
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Fri, Apr 8, 2016 at 12:55 PM, Thomas Munro
 wrote:
> On Wed, Apr 6, 2016 at 8:23 PM, Fujii Masao  wrote:
>>
>> Okay, I pushed the patch!
>> Many thanks to all involved in the development of this feature!
>
>
> Hi,
>
> I spotted a couple of places in the documentation that still implied there
> was only one synchronous standby.  Please see suggested fixes attached.

Thanks! Applied.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Thomas Munro]

On Wed, Apr 6, 2016 at 8:23 PM, Fujii Masao  wrote:
>
> Okay, I pushed the patch!
> Many thanks to all involved in the development of this feature!
>

Hi,

I spotted a couple of places in the documentation that still implied there
was only one synchronous standby.  Please see suggested fixes attached.

-- 
Thomas Munro
http://www.enterprisedb.com


doc.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 5:04 PM, Michael Paquier
 wrote:
> On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
>  wrote:
>> Here are few things I have noticed:
>> +   for (i = 0; i < max_wal_senders; i++)
>> +   {
>> +   walsnd = >walsnds[i];
>> No volatile pointer to prevent code reordering?
>>
>>   */
>>  typedef struct WalSnd
>>  {
>> +   int slotno; /* index of this slot in WalSnd array */
>> pid_t   pid;/* this walsender's process id, or 0 */
>> slotno is used nowhere.
>>
>> I'll grab the tests and look at them.
>
> So I had a look at those tests and finished with the attached:
> - patch 1 adds a reload routine to PostgresNode
> - patch 2 the list of tests.

Thanks for updating the patches!

Attached is the refactored version of the patch.

Regards,

-- 
Fujii Masao
*** a/src/test/perl/PostgresNode.pm
--- b/src/test/perl/PostgresNode.pm
***
*** 668,673  sub stop
--- 668,691 
  
  =pod
  
+ =item $node->reload()
+ 
+ Reload configuration parameters on the node.
+ 
+ =cut
+ 
+ sub reload
+ {
+ 	my ($self) = @_;
+ 	my $port   = $self->port;
+ 	my $pgdata = $self->data_dir;
+ 	my $name   = $self->name;
+ 	print "### Reloading node \"$name\"\n";
+ 	TestLib::system_log('pg_ctl', '-D', $pgdata, 'reload');
+ }
+ 
+ =pod
+ 
  =item $node->restart()
  
  Wrapper for pg_ctl -w restart
*** /dev/null
--- b/src/test/recovery/t/007_sync_rep.pl
***
*** 0 
--- 1,149 
+ # Minimal test testing synchronous replication sync_state transition
+ use strict;
+ use warnings;
+ use PostgresNode;
+ use TestLib;
+ use Test::More tests => 8;
+ 
+ # Query checking sync_priority and sync_state of each standby
+ my $check_sql = "SELECT application_name, sync_priority, sync_state FROM pg_stat_replication ORDER BY application_name;";
+ 
+ # Check that sync_state of each standby is expected.
+ # If $setting is given, synchronous_standby_names is set to it and
+ # the configuration file is reloaded before the test.
+ sub test_sync_state
+ {
+ 	my ($self, $expected, $msg, $setting) = @_;
+ 
+ 	if (defined($setting))
+ 	{
+ 		$self->psql('postgres',
+ 	"ALTER SYSTEM SET synchronous_standby_names = '$setting';");
+ 		$self->reload;		
+ 	}
+ 
+ 	my $result = $self->safe_psql('postgres', $check_sql);
+ 	is($result, $expected, $msg);
+ }
+ 
+ # Initialize master node
+ my $node_master = get_new_node('master');
+ $node_master->init(allows_streaming => 1);
+ $node_master->start;
+ my $backup_name = 'master_backup';
+ 
+ # Take backup
+ $node_master->backup($backup_name);
+ 
+ # Create standby1 linking to master
+ my $node_standby_1 = get_new_node('standby1');
+ $node_standby_1->init_from_backup($node_master, $backup_name,
+ 	has_streaming => 1);
+ $node_standby_1->start;
+ 
+ # Create standby2 linking to master
+ my $node_standby_2 = get_new_node('standby2');
+ $node_standby_2->init_from_backup($node_master, $backup_name,
+ 	has_streaming => 1);
+ $node_standby_2->start;
+ 
+ # Create standby3 linking to master
+ my $node_standby_3 = get_new_node('standby3');
+ $node_standby_3->init_from_backup($node_master, $backup_name,
+ 	has_streaming => 1);
+ $node_standby_3->start;
+ 
+ # Check that sync_state is determined correctly when
+ # synchronous_standby_names is specified in old syntax.
+ test_sync_state($node_master, qq(standby1|1|sync
+ standby2|2|potential
+ standby3|0|async),
+ 	'old syntax of synchronous_standby_names',
+ 	'standby1,standby2');
+ 
+ # Check that all the standbys are considered as either sync or
+ # potential when * is specified in synchronous_standby_names.
+ # Note that standby1 is chosen as sync standby because
+ # it's stored in the head of WalSnd array which manages
+ # all the standbys though they have the same priority.
+ test_sync_state($node_master, qq(standby1|1|sync
+ standby2|1|potential
+ standby3|1|potential),
+ 	'asterisk in synchronous_standby_names',
+ 	'*');
+ 
+ # Stop and start standbys to rearrange the order of standbys
+ # in WalSnd array. Now, if standbys have the same priority,
+ # standby2 is selected preferentially and standby3 is next.
+ $node_standby_1->stop;
+ $node_standby_2->stop;
+ $node_standby_3->stop;
+ 
+ $node_standby_2->start;
+ $node_standby_3->start;
+ 
+ # Specify 2 as the number of sync standbys.
+ # Check that two standbys are in 'sync' state.
+ test_sync_state($node_master, qq(standby2|2|sync
+ standby3|3|sync),
+ 	'2 synchronous standbys',
+ 	'2(standby1,standby2,standby3)');
+ 
+ # Start standby1
+ $node_standby_1->start;
+ 
+ # Create standby4 linking to master
+ my $node_standby_4 = get_new_node('standby4');
+ $node_standby_4->init_from_backup($node_master, $backup_name,
+   has_streaming => 1);
+ $node_standby_4->start;
+ 
+ # Check that standby1 and standby2 whose names appear earlier in
+ # synchronous_standby_names are considered as sync. Also check that
+ # standby3 appearing later represents potential, and standby4 is
+ # in 'async' state 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Thu, Apr 7, 2016 at 7:29 PM, Amit Kapila  wrote:
> On Thu, Apr 7, 2016 at 1:30 PM, Amit Langote 
> wrote:
>>
>> On 2016/04/07 15:26, Fujii Masao wrote:
>> > On Thu, Apr 7, 2016 at 2:48 PM, Amit Kapila 
>> > wrote:
>> >> On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao 
>> >> wrote:
>> >>> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
>> >>> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
>> >>> complicated.
>> >> SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want
>> >> to
>> >> pass that?  I assume that current non-default value of
>> >> SyncRepStandbyNames
>> >> will be passed via write_nondefault_variables(), so we can use that to
>> >> regenerate SyncRepConfig.
>> >
>> > Yes, so SyncRepUpdateConfig() needs to be called by a backend after
>> > fork,
>> > to regenerate SyncRepConfig from the passed value of
>> > SyncRepStandbyNames.
>> > This is the approach of (2) which I explained upthread. assign_XXX
>> > function
>> > doesn't seem to be helpful for this case.
>>
>> I don't see why there is need to SyncRepUpdateConfig() after every fork or
>> anywhere outside syncrep.c/walsender.c for that matter.  AIUI, only
>> walsender or a backend that runs pg_stat_get_wal_senders() ever needs to
>> run SyncRepUpdateConfig() to get parsed synchronous standbys info from the
>> string that is SyncRepStandbyNames.
>>
>
> So if we go by this each time backend calls pg_stat_get_wal_senders, it
> needs to do parsing to form SyncRepConfig whether it's changed or not from
> previous time.  I understand that this is not a performance critical path,
> but still if we can do it in some other optimal way which doesn't hurt any
> other path, then it will be better.

So, will you write the patch? Either current implementation or
the approach you're suggesting works to me. If you really want
to change the current one, I'm happy to review that.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Thu, Apr 7, 2016 at 1:30 PM, Amit Langote 
wrote:
>
> On 2016/04/07 15:26, Fujii Masao wrote:
> > On Thu, Apr 7, 2016 at 2:48 PM, Amit Kapila 
wrote:
> >> On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao 
wrote:
> >>> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
> >>> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
> >>> complicated.
> >> SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want
to
> >> pass that?  I assume that current non-default value of
SyncRepStandbyNames
> >> will be passed via write_nondefault_variables(), so we can use that to
> >> regenerate SyncRepConfig.
> >
> > Yes, so SyncRepUpdateConfig() needs to be called by a backend after
fork,
> > to regenerate SyncRepConfig from the passed value of
SyncRepStandbyNames.
> > This is the approach of (2) which I explained upthread. assign_XXX
function
> > doesn't seem to be helpful for this case.
>
> I don't see why there is need to SyncRepUpdateConfig() after every fork or
> anywhere outside syncrep.c/walsender.c for that matter.  AIUI, only
> walsender or a backend that runs pg_stat_get_wal_senders() ever needs to
> run SyncRepUpdateConfig() to get parsed synchronous standbys info from the
> string that is SyncRepStandbyNames.
>

So if we go by this each time backend calls pg_stat_get_wal_senders, it
needs to do parsing to form SyncRepConfig whether it's changed or not from
previous time.  I understand that this is not a performance critical path,
but still if we can do it in some other optimal way which doesn't hurt any
other path, then it will be better.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Langote]

On 2016/04/07 15:26, Fujii Masao wrote:
> On Thu, Apr 7, 2016 at 2:48 PM, Amit Kapila  wrote:
>> On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao  wrote:
>>> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
>>> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
>>> complicated.
>> SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want to
>> pass that?  I assume that current non-default value of SyncRepStandbyNames
>> will be passed via write_nondefault_variables(), so we can use that to
>> regenerate SyncRepConfig.
> 
> Yes, so SyncRepUpdateConfig() needs to be called by a backend after fork,
> to regenerate SyncRepConfig from the passed value of SyncRepStandbyNames.
> This is the approach of (2) which I explained upthread. assign_XXX function
> doesn't seem to be helpful for this case.

I don't see why there is need to SyncRepUpdateConfig() after every fork or
anywhere outside syncrep.c/walsender.c for that matter.  AIUI, only
walsender or a backend that runs pg_stat_get_wal_senders() ever needs to
run SyncRepUpdateConfig() to get parsed synchronous standbys info from the
string that is SyncRepStandbyNames.  For rest of the world, it's just a
string guc and is written to and read from any external file as one (e.g.
the file that write_nondefault_variables() writes to in the EXEC_BACKEND
case).  I hope I'm not entirely missing the point of the discussion you
and Amit are having.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Thu, Apr 7, 2016 at 11:56 AM, Fujii Masao  wrote:
>
> On Thu, Apr 7, 2016 at 2:48 PM, Amit Kapila 
wrote:
> > On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao 
wrote:
> >>
> >> On Thu, Apr 7, 2016 at 1:22 PM, Amit Kapila 
> >> wrote:
> >> >
> >> > But for that, I think we don't need to do anything extra.  I mean
> >> > write_nondefault_variables() will automatically write the non-default
> >> > value
> >> > of variable and then during backend initialization, it will call
> >> > read_nondefault_variables which will call set_config_option for
> >> > non-default
> >> > parameters and that should set the required value if we have assign_*
> >> > function defined for the variable.
> >>
> >> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
> >> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
> >> complicated.
> >>
> >
> > SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want to
> > pass that?  I assume that current non-default value of
SyncRepStandbyNames
> > will be passed via write_nondefault_variables(), so we can use that to
> > regenerate SyncRepConfig.
>
> Yes, so SyncRepUpdateConfig() needs to be called by a backend after fork,
> to regenerate SyncRepConfig from the passed value of SyncRepStandbyNames.
> This is the approach of (2) which I explained upthread. assign_XXX
function
> doesn't seem to be helpful for this case.
>

Then where do you want to call it?  Also, this is only required for
EXEC_BACKEND builds.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Thu, Apr 7, 2016 at 2:48 PM, Amit Kapila  wrote:
> On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao  wrote:
>>
>> On Thu, Apr 7, 2016 at 1:22 PM, Amit Kapila 
>> wrote:
>> >
>> > But for that, I think we don't need to do anything extra.  I mean
>> > write_nondefault_variables() will automatically write the non-default
>> > value
>> > of variable and then during backend initialization, it will call
>> > read_nondefault_variables which will call set_config_option for
>> > non-default
>> > parameters and that should set the required value if we have assign_*
>> > function defined for the variable.
>>
>> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
>> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
>> complicated.
>>
>
> SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want to
> pass that?  I assume that current non-default value of SyncRepStandbyNames
> will be passed via write_nondefault_variables(), so we can use that to
> regenerate SyncRepConfig.

Yes, so SyncRepUpdateConfig() needs to be called by a backend after fork,
to regenerate SyncRepConfig from the passed value of SyncRepStandbyNames.
This is the approach of (2) which I explained upthread. assign_XXX function
doesn't seem to be helpful for this case.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Thu, Apr 7, 2016 at 10:02 AM, Fujii Masao  wrote:
>
> On Thu, Apr 7, 2016 at 1:22 PM, Amit Kapila 
wrote:
> >
> > But for that, I think we don't need to do anything extra.  I mean
> > write_nondefault_variables() will automatically write the non-default
value
> > of variable and then during backend initialization, it will call
> > read_nondefault_variables which will call set_config_option for
non-default
> > parameters and that should set the required value if we have assign_*
> > function defined for the variable.
>
> Yes if the variable that we'd like to pass to a backend is BOOL, INT,
> REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
> complicated.
>

SyncRepConfig is a parsed result of SyncRepStandbyNames, why you want to
pass that?  I assume that current non-default value of SyncRepStandbyNames
will be passed via write_nondefault_variables(), so we can use that to
regenerate SyncRepConfig.

>
> So ISTM that write_one_nondefault_variable() needs to
> be updated so that SyncRepConfig is written to a file.
>


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Thu, Apr 7, 2016 at 1:22 PM, Amit Kapila  wrote:
> On Wed, Apr 6, 2016 at 8:11 PM, Fujii Masao  wrote:
>>
>> On Wed, Apr 6, 2016 at 11:14 PM, Amit Kapila 
>> wrote:
>> > On Wed, Apr 6, 2016 at 7:03 PM, Fujii Masao 
>> > wrote:
>> >>
>> >> On Wed, Apr 6, 2016 at 8:59 PM, Amit Kapila 
>> >> wrote:
>> >> >
>> >> >> BTW, we can move SyncRepUpdateConfig() just after
>> >> >> ProcessConfigFile()
>> >> >> from pg_stat_get_wal_senders() and every backends always parse the
>> >> >> value
>> >> >> of s_s_names when the setting is changed.
>> >> >>
>> >> >
>> >> > That sounds appropriate, but not sure what is exact place to call it.
>> >>
>> >> Maybe just after the following ProcessConfigFile().
>> >>
>> >> -
>> >> /*
>> >> * (6) check for any other interesting events that happened while we
>> >> * slept.
>> >> */
>> >> if (got_SIGHUP)
>> >> {
>> >> got_SIGHUP = false;
>> >> ProcessConfigFile(PGC_SIGHUP);
>> >> }
>> >> -
>> >>
>> >> If we do the move, we also need to either (1) make postmaster call
>> >> SyncRepUpdateConfig() and pass the parsed result to any forked backends
>> >> via a file like write_nondefault_variables() does for EXEC_BACKEND
>> >> environment, or (2) make a backend call SyncRepUpdateConfig() during
>> >> its initialization phase so that the first call of pg_stat_replication
>> >> can use the parsed result. (1) seems complicated and overkill.
>> >> (2) may add very small overhead into the fork of a backend. It would
>> >> be almost negligible, though. So which logic should we adopt?
>> >>
>> >
>> > Won't it be possible to have assign_* function for
>> > synchronous_standby_names
>> > as we have for some of the other settings like assign_XactIsoLevel and
>> > then
>> > call SyncRepUpdateConfig() in that function?
>>
>> It's possible, but still seems to need (1), i.e., the variable that
>> assign_XXX
>> function assigned needs to be passed to a backend via file for
>> EXEC_BACKEND
>> environment.
>>
>
> But for that, I think we don't need to do anything extra.  I mean
> write_nondefault_variables() will automatically write the non-default value
> of variable and then during backend initialization, it will call
> read_nondefault_variables which will call set_config_option for non-default
> parameters and that should set the required value if we have assign_*
> function defined for the variable.

Yes if the variable that we'd like to pass to a backend is BOOL, INT,
REAL, STRING or ENUM. But SyncRepConfig variable is a bit more
complicated. So ISTM that write_one_nondefault_variable() needs to
be updated so that SyncRepConfig is written to a file.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Wed, Apr 6, 2016 at 8:11 PM, Fujii Masao  wrote:
>
> On Wed, Apr 6, 2016 at 11:14 PM, Amit Kapila 
wrote:
> > On Wed, Apr 6, 2016 at 7:03 PM, Fujii Masao 
wrote:
> >>
> >> On Wed, Apr 6, 2016 at 8:59 PM, Amit Kapila 
> >> wrote:
> >> >
> >> >> BTW, we can move SyncRepUpdateConfig() just after
ProcessConfigFile()
> >> >> from pg_stat_get_wal_senders() and every backends always parse the
> >> >> value
> >> >> of s_s_names when the setting is changed.
> >> >>
> >> >
> >> > That sounds appropriate, but not sure what is exact place to call it.
> >>
> >> Maybe just after the following ProcessConfigFile().
> >>
> >> -
> >> /*
> >> * (6) check for any other interesting events that happened while we
> >> * slept.
> >> */
> >> if (got_SIGHUP)
> >> {
> >> got_SIGHUP = false;
> >> ProcessConfigFile(PGC_SIGHUP);
> >> }
> >> -
> >>
> >> If we do the move, we also need to either (1) make postmaster call
> >> SyncRepUpdateConfig() and pass the parsed result to any forked backends
> >> via a file like write_nondefault_variables() does for EXEC_BACKEND
> >> environment, or (2) make a backend call SyncRepUpdateConfig() during
> >> its initialization phase so that the first call of pg_stat_replication
> >> can use the parsed result. (1) seems complicated and overkill.
> >> (2) may add very small overhead into the fork of a backend. It would
> >> be almost negligible, though. So which logic should we adopt?
> >>
> >
> > Won't it be possible to have assign_* function for
synchronous_standby_names
> > as we have for some of the other settings like assign_XactIsoLevel and
then
> > call SyncRepUpdateConfig() in that function?
>
> It's possible, but still seems to need (1), i.e., the variable that
assign_XXX
> function assigned needs to be passed to a backend via file for
EXEC_BACKEND
> environment.
>

But for that, I think we don't need to do anything extra.  I
mean write_nondefault_variables() will automatically write the non-default
value of variable and then during backend initialization, it will
call read_nondefault_variables which will call set_config_option for
non-default parameters and that should set the required value if we have
assign_* function defined for the variable.



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 11:14 PM, Amit Kapila  wrote:
> On Wed, Apr 6, 2016 at 7:03 PM, Fujii Masao  wrote:
>>
>> On Wed, Apr 6, 2016 at 8:59 PM, Amit Kapila 
>> wrote:
>> >
>> >> BTW, we can move SyncRepUpdateConfig() just after ProcessConfigFile()
>> >> from pg_stat_get_wal_senders() and every backends always parse the
>> >> value
>> >> of s_s_names when the setting is changed.
>> >>
>> >
>> > That sounds appropriate, but not sure what is exact place to call it.
>>
>> Maybe just after the following ProcessConfigFile().
>>
>> -
>> /*
>> * (6) check for any other interesting events that happened while we
>> * slept.
>> */
>> if (got_SIGHUP)
>> {
>> got_SIGHUP = false;
>> ProcessConfigFile(PGC_SIGHUP);
>> }
>> -
>>
>> If we do the move, we also need to either (1) make postmaster call
>> SyncRepUpdateConfig() and pass the parsed result to any forked backends
>> via a file like write_nondefault_variables() does for EXEC_BACKEND
>> environment, or (2) make a backend call SyncRepUpdateConfig() during
>> its initialization phase so that the first call of pg_stat_replication
>> can use the parsed result. (1) seems complicated and overkill.
>> (2) may add very small overhead into the fork of a backend. It would
>> be almost negligible, though. So which logic should we adopt?
>>
>
> Won't it be possible to have assign_* function for synchronous_standby_names
> as we have for some of the other settings like assign_XactIsoLevel and then
> call SyncRepUpdateConfig() in that function?

It's possible, but still seems to need (1), i.e., the variable that assign_XXX
function assigned needs to be passed to a backend via file for EXEC_BACKEND
environment.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Wed, Apr 6, 2016 at 7:03 PM, Fujii Masao  wrote:
>
> On Wed, Apr 6, 2016 at 8:59 PM, Amit Kapila 
wrote:
> >
> >> BTW, we can move SyncRepUpdateConfig() just after ProcessConfigFile()
> >> from pg_stat_get_wal_senders() and every backends always parse the
value
> >> of s_s_names when the setting is changed.
> >>
> >
> > That sounds appropriate, but not sure what is exact place to call it.
>
> Maybe just after the following ProcessConfigFile().
>
> -
> /*
> * (6) check for any other interesting events that happened while we
> * slept.
> */
> if (got_SIGHUP)
> {
> got_SIGHUP = false;
> ProcessConfigFile(PGC_SIGHUP);
> }
> -
>
> If we do the move, we also need to either (1) make postmaster call
> SyncRepUpdateConfig() and pass the parsed result to any forked backends
> via a file like write_nondefault_variables() does for EXEC_BACKEND
> environment, or (2) make a backend call SyncRepUpdateConfig() during
> its initialization phase so that the first call of pg_stat_replication
> can use the parsed result. (1) seems complicated and overkill.
> (2) may add very small overhead into the fork of a backend. It would
> be almost negligible, though. So which logic should we adopt?
>

Won't it be possible to have assign_* function
for synchronous_standby_names as we have for some of the other settings
like assign_XactIsoLevel and then call SyncRepUpdateConfig() in that
function?



With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 8:59 PM, Amit Kapila  wrote:
> On Wed, Apr 6, 2016 at 11:17 AM, Fujii Masao  wrote:
>>
>> On Tue, Apr 5, 2016 at 11:40 PM, Amit Kapila 
>> wrote:
>> >>
>> >> > 2.
>> >> > pg_stat_get_wal_senders()
>> >> > {
>> >> > ..
>> >> > /*
>> >> > ! * Allocate and update the config data of synchronous replication,
>> >> > ! * and then get the currently active synchronous standbys.
>> >> >   */
>> >> > + SyncRepUpdateConfig();
>> >> >   LWLockAcquire(SyncRepLock, LW_SHARED);
>> >> > ! sync_standbys = SyncRepGetSyncStandbys();
>> >> >   LWLockRelease(SyncRepLock);
>> >> > ..
>> >> > }
>> >> >
>> >> > Why is it important to update the config with patch?  Earlier also
>> >> > any
>> >> > update to config between calls wouldn't have been visible.
>> >>
>> >> Because a backend has no chance to call SyncRepUpdateConfig() and
>> >> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
>> >> called here. This means that pg_stat_replication may return the
>> >> information
>> >> based on the old value of s_s_names.
>> >>
>> >
>> > Thats right, but without this patch also won't pg_stat_replication can
>> > show
>> > old information? If no, why so?
>>
>> Without the patch, when s_s_names is changed and SIGHUP is sent,
>> a backend calls ProcessConfigFile(), parse the configuration file and
>> set the global variable SyncRepStandbyNames to the latest value of
>> s_s_names. When pg_stat_replication is accessed, a backend calculates
>> which standby is synchronous based on that latest value in
>> SyncRepStandbyNames,
>> and then displays the information of sync replication.
>>
>> With the patch, basically the same steps are executed when s_s_names is
>> changed. But the difference is that, with the patch, SyncRepUpdateConfig()
>> must be called after ProcessConfigFile() is called before the calculation
>> of
>> sync standbys. So I just added the call of SyncRepUpdateConfig() to
>> pg_stat_get_wal_senders().
>>
>
> Then why to call it just in pg_stat_get_wal_senders(), isn't it better if we
> call it always after ProcessConfigFile() (after setting SyncRepStandbyNames)
>
>> BTW, we can move SyncRepUpdateConfig() just after ProcessConfigFile()
>> from pg_stat_get_wal_senders() and every backends always parse the value
>> of s_s_names when the setting is changed.
>>
>
> That sounds appropriate, but not sure what is exact place to call it.

Maybe just after the following ProcessConfigFile().

-
/*
* (6) check for any other interesting events that happened while we
* slept.
*/
if (got_SIGHUP)
{
got_SIGHUP = false;
ProcessConfigFile(PGC_SIGHUP);
}
-

If we do the move, we also need to either (1) make postmaster call
SyncRepUpdateConfig() and pass the parsed result to any forked backends
via a file like write_nondefault_variables() does for EXEC_BACKEND
environment, or (2) make a backend call SyncRepUpdateConfig() during
its initialization phase so that the first call of pg_stat_replication
can use the parsed result. (1) seems complicated and overkill.
(2) may add very small overhead into the fork of a backend. It would
be almost negligible, though. So which logic should we adopt?

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Wed, Apr 6, 2016 at 11:17 AM, Fujii Masao  wrote:
>
> On Tue, Apr 5, 2016 at 11:40 PM, Amit Kapila 
wrote:
> >>
> >> > 2.
> >> > pg_stat_get_wal_senders()
> >> > {
> >> > ..
> >> > /*
> >> > ! * Allocate and update the config data of synchronous replication,
> >> > ! * and then get the currently active synchronous standbys.
> >> >   */
> >> > + SyncRepUpdateConfig();
> >> >   LWLockAcquire(SyncRepLock, LW_SHARED);
> >> > ! sync_standbys = SyncRepGetSyncStandbys();
> >> >   LWLockRelease(SyncRepLock);
> >> > ..
> >> > }
> >> >
> >> > Why is it important to update the config with patch?  Earlier also
any
> >> > update to config between calls wouldn't have been visible.
> >>
> >> Because a backend has no chance to call SyncRepUpdateConfig() and
> >> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
> >> called here. This means that pg_stat_replication may return the
> >> information
> >> based on the old value of s_s_names.
> >>
> >
> > Thats right, but without this patch also won't pg_stat_replication can
show
> > old information? If no, why so?
>
> Without the patch, when s_s_names is changed and SIGHUP is sent,
> a backend calls ProcessConfigFile(), parse the configuration file and
> set the global variable SyncRepStandbyNames to the latest value of
> s_s_names. When pg_stat_replication is accessed, a backend calculates
> which standby is synchronous based on that latest value in
SyncRepStandbyNames,
> and then displays the information of sync replication.
>
> With the patch, basically the same steps are executed when s_s_names is
> changed. But the difference is that, with the patch, SyncRepUpdateConfig()
> must be called after ProcessConfigFile() is called before the calculation
of
> sync standbys. So I just added the call of SyncRepUpdateConfig() to
> pg_stat_get_wal_senders().
>

Then why to call it just in pg_stat_get_wal_senders(), isn't it better if
we call it always after ProcessConfigFile() (after
setting SyncRepStandbyNames)

> BTW, we can move SyncRepUpdateConfig() just after ProcessConfigFile()
> from pg_stat_get_wal_senders() and every backends always parse the value
> of s_s_names when the setting is changed.
>

That sounds appropriate, but not sure what is exact place to call it.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 6 April 2016 at 09:23, Fujii Masao  wrote:


> Okay, I pushed the patch!
> Many thanks to all involved in the development of this feature!
>

Very good.

I think the description in the commit message that we don't support "quorum
commit" is sufficient to cover my concerns about what others might expect
from this feature. Could we add similar wording to the docs?

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Wed, Apr 6, 2016 at 5:23 PM, Fujii Masao  wrote:
> Okay, I pushed the patch!
> Many thanks to all involved in the development of this feature!

I think that I am crying... Really cool to see this milestone accomplished.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 5:07 PM, Fujii Masao  wrote:
> On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
>  wrote:
>> On Wed, Apr 6, 2016 at 3:29 PM, Fujii Masao  wrote:
>>> On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
>>>  wrote:
 At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao  
 wrote in 
 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

Sorry, my code was wrong in the case that the total numer of
synchronous standby exceeds required number and the wansender is
at priority 1.

Sorry for the noise.

At Wed, 06 Apr 2016 17:01:51 +0900 (Tokyo Standard Time), Kyotaro HORIGUCHI 
 wrote in 
<20160406.170151.246853881.horiguchi.kyot...@lab.ntt.co.jp>
> You must misread the patch. am_sync is originally set in the loop
> just after that for the case.

regards,


-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 5:01 PM, Kyotaro HORIGUCHI
 wrote:
> At Wed, 6 Apr 2016 15:29:12 +0900, Fujii Masao  wrote 
> in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
 wrote:
> On Wed, Apr 6, 2016 at 3:29 PM, Fujii Masao  wrote:
>> On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
>>  wrote:
>>> At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao  
>>> wrote in 
>>> 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Wed, Apr 6, 2016 at 4:08 PM, Michael Paquier
 wrote:
> Here are few things I have noticed:
> +   for (i = 0; i < max_wal_senders; i++)
> +   {
> +   walsnd = >walsnds[i];
> No volatile pointer to prevent code reordering?
>
>   */
>  typedef struct WalSnd
>  {
> +   int slotno; /* index of this slot in WalSnd array */
> pid_t   pid;/* this walsender's process id, or 0 */
> slotno is used nowhere.
>
> I'll grab the tests and look at them.

So I had a look at those tests and finished with the attached:
- patch 1 adds a reload routine to PostgresNode
- patch 2 the list of tests.

I took the tests from patch 21 and did many tweaks on them:
- Use of qq() instead of quotes
- Removal of hardcoded newlines
- typo fixes and sanity fixes
- etc.
Regards,
-- 
Michael


2_n_sync_tests.patch
Description: invalid/octet-stream


1_add_reload_routine.patch
Description: invalid/octet-stream

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Wed, 6 Apr 2016 15:29:12 +0900, Fujii Masao  wrote in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Wed, Apr 6, 2016 at 3:29 PM, Fujii Masao  wrote:
> On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
>  wrote:
>> At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao  wrote 
>> in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 2:51 PM, Masahiko Sawada  wrote:
> On Wed, Apr 6, 2016 at 2:21 PM, Fujii Masao  wrote:
>> On Tue, Apr 5, 2016 at 8:51 PM, Simon Riggs  wrote:
>>> On 5 April 2016 at 12:26, Fujii Masao  wrote:
>>>

 Multiple standbys with the same name may connect to the master.
 In this case, users might want to specifiy k<=N. So k<=N seems not invalid
 setting.
>>>
>>>
>>> Confusing as that is, it is already the case; k > N could make sense. ;-(
>>>
>>> However, in most cases, k > N would not make sense and we should issue a
>>> WARNING.
>>
>> Somebody (maybe Horiguchi-san and Sawada-san) commented this upthread
>> and the code for that test was included in the old patch (but I excluded it).
>> Now the majority seems to prefer to add that test, so I just revived and
>> revised that test code.
>
> The regression test codes seems not to be included in latest patch, no?

I intentionally excluded the regression test from the patch because
I'd like to review and commit it separately from the main part of the feature.

I'd appreciate if you read through the regression test which was included
in previous patch and update it if required.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Wed, Apr 6, 2016 at 2:18 PM, Kyotaro HORIGUCHI
 wrote:
> At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao  wrote 
> in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Michael Paquier]

On Wed, Apr 6, 2016 at 2:51 PM, Masahiko Sawada  wrote:
> On Wed, Apr 6, 2016 at 2:21 PM, Fujii Masao  wrote:
>> On Tue, Apr 5, 2016 at 8:51 PM, Simon Riggs  wrote:
>>> On 5 April 2016 at 12:26, Fujii Masao  wrote:
>>>

 Multiple standbys with the same name may connect to the master.
 In this case, users might want to specifiy k<=N. So k<=N seems not invalid
 setting.
>>>
>>>
>>> Confusing as that is, it is already the case; k > N could make sense. ;-(
>>>
>>> However, in most cases, k > N would not make sense and we should issue a
>>> WARNING.
>>
>> Somebody (maybe Horiguchi-san and Sawada-san) commented this upthread
>> and the code for that test was included in the old patch (but I excluded it).
>> Now the majority seems to prefer to add that test, so I just revived and
>> revised that test code.
>
> The regression test codes seems not to be included in latest patch, no?

I am looking at the latest patch now, and they are not included. It
would be good to get those tests bundled in for a last lookup I think.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Wed, Apr 6, 2016 at 2:21 PM, Fujii Masao  wrote:
> On Tue, Apr 5, 2016 at 8:51 PM, Simon Riggs  wrote:
>> On 5 April 2016 at 12:26, Fujii Masao  wrote:
>>
>>>
>>> Multiple standbys with the same name may connect to the master.
>>> In this case, users might want to specifiy k<=N. So k<=N seems not invalid
>>> setting.
>>
>>
>> Confusing as that is, it is already the case; k > N could make sense. ;-(
>>
>> However, in most cases, k > N would not make sense and we should issue a
>> WARNING.
>
> Somebody (maybe Horiguchi-san and Sawada-san) commented this upthread
> and the code for that test was included in the old patch (but I excluded it).
> Now the majority seems to prefer to add that test, so I just revived and
> revised that test code.

The regression test codes seems not to be included in latest patch, no?

Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 11:47 PM, Robert Haas  wrote:
> On Mon, Apr 4, 2016 at 4:28 AM, Fujii Masao  wrote:
 + ereport(LOG,
 + (errmsg("standby \"%s\" is now the synchronous standby with priority %u",
 + application_name, MyWalSnd->sync_standby_priority)));

 s/ the / a /
>>
>> I have no objection to this change itself. But we have used this message
>> in 9.5 or before, so if we apply this change, probably we need
>> back-patching.
>
> "the" implies that there can be only one synchronous standby at that
> priority, while "a" implies that there could be more than one.  So the
> situation might be different with this patch than previously.  (I
> haven't read the patch so I don't know whether this is actually true,
> but it might be what Thomas was going for.)

Thanks for the explanation!
I applied that change, to the latest patch I posted upthread.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 11:40 PM, Amit Kapila  wrote:
> On Tue, Apr 5, 2016 at 3:15 PM, Fujii Masao  wrote:
>>
>> On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila 
>> wrote:
>> > On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao 
>> > wrote:
>> >>
>> >>
>> >> Thanks for updating the patch!
>> >>
>> >> I applied the following changes to the patch.
>> >> Attached is the revised version of the patch.
>> >>
>> >
>> > 1.
>> >{
>> > {"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
>> > gettext_noop("List of names of potential synchronous standbys."),
>> > NULL,
>> > GUC_LIST_INPUT
>> > },
>> > ,
>> > "",
>> > check_synchronous_standby_names, NULL, NULL
>> > },
>> >
>> > Isn't it better to modify the description of synchronous_standby_names
>> > in
>> > guc.c based on new usage?
>>
>> What about "Number of synchronous standbys and list of names of
>> potential synchronous ones"? Better idea?
>>
>
> Looks good.
>
>>
>> > 2.
>> > pg_stat_get_wal_senders()
>> > {
>> > ..
>> > /*
>> > ! * Allocate and update the config data of synchronous replication,
>> > ! * and then get the currently active synchronous standbys.
>> >   */
>> > + SyncRepUpdateConfig();
>> >   LWLockAcquire(SyncRepLock, LW_SHARED);
>> > ! sync_standbys = SyncRepGetSyncStandbys();
>> >   LWLockRelease(SyncRepLock);
>> > ..
>> > }
>> >
>> > Why is it important to update the config with patch?  Earlier also any
>> > update to config between calls wouldn't have been visible.
>>
>> Because a backend has no chance to call SyncRepUpdateConfig() and
>> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
>> called here. This means that pg_stat_replication may return the
>> information
>> based on the old value of s_s_names.
>>
>
> Thats right, but without this patch also won't pg_stat_replication can show
> old information? If no, why so?

Without the patch, when s_s_names is changed and SIGHUP is sent,
a backend calls ProcessConfigFile(), parse the configuration file and
set the global variable SyncRepStandbyNames to the latest value of
s_s_names. When pg_stat_replication is accessed, a backend calculates
which standby is synchronous based on that latest value in SyncRepStandbyNames,
and then displays the information of sync replication.

With the patch, basically the same steps are executed when s_s_names is
changed. But the difference is that, with the patch, SyncRepUpdateConfig()
must be called after ProcessConfigFile() is called before the calculation of
sync standbys. So I just added the call of SyncRepUpdateConfig() to
pg_stat_get_wal_senders().

BTW, we can move SyncRepUpdateConfig() just after ProcessConfigFile()
from pg_stat_get_wal_senders() and every backends always parse the value
of s_s_names when the setting is changed.

>> > 3.
>> >   Planning for High Availability
>> >
>> >  
>> > ! synchronous_standby_names specifies the number of
>> > ! synchronous standbys that transaction commits made when
>> >
>> > Is it better to say like: synchronous_standby_names
>> > specifies
>> > the number and names of
>>
>> Precisely s_s_names specifies a list of names of potential sync standbys
>> not sync ones.
>>
>
> Okay, but you doesn't seem to have updated this in your latest patch.

I applied the change you suggested, to the patch. Thanks!

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 8:51 PM, Simon Riggs  wrote:
> On 5 April 2016 at 12:26, Fujii Masao  wrote:
>
>>
>> Multiple standbys with the same name may connect to the master.
>> In this case, users might want to specifiy k<=N. So k<=N seems not invalid
>> setting.
>
>
> Confusing as that is, it is already the case; k > N could make sense. ;-(
>
> However, in most cases, k > N would not make sense and we should issue a
> WARNING.

Somebody (maybe Horiguchi-san and Sawada-san) commented this upthread
and the code for that test was included in the old patch (but I excluded it).
Now the majority seems to prefer to add that test, so I just revived and
revised that test code.

Attached is the updated version of the patch. I also completed Amit's
and Robert's comments.

Regards,

-- 
Fujii Masao
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 2906,2939  include_dir 'conf.d'


 
! Specifies a comma-separated list of standby names that can support
  synchronous replication, as described in
  .
! At any one time there will be at most one active synchronous standby;
  transactions waiting for commit will be allowed to proceed after
! this standby server confirms receipt of their data.
! The synchronous standby will be the first standby named in this list
  that is both currently connected and streaming data in real-time
  (as shown by a state of streaming in the
  
  pg_stat_replication view).
  Other standby servers appearing later in this list represent potential
! synchronous standbys.
! If the current synchronous standby disconnects for whatever reason,
  it will be replaced immediately with the next-highest-priority standby.
  Specifying more than one standby name can allow very high availability.
 
 
  The name of a standby server for this purpose is the
  application_name setting of the standby, as set in the
  primary_conninfo of the standby's WAL receiver.  There is
  no mechanism to enforce uniqueness. In case of duplicates one of the
! matching standbys will be chosen to be the synchronous standby, though
  exactly which one is indeterminate.
  The special entry * matches any
  application_name, including the default application name
  of walreceiver.
 
 
  If no synchronous standby names are specified here, then synchronous
  replication is not enabled and transaction commits will not wait for
--- 2906,2974 


 
! Specifies a list of standby names that can support
  synchronous replication, as described in
  .
! There will be one or more active synchronous standbys;
  transactions waiting for commit will be allowed to proceed after
! these standby servers confirm receipt of their data.
! The synchronous standbys will be those whose names appear
! earlier in this list, and
  that is both currently connected and streaming data in real-time
  (as shown by a state of streaming in the
  
  pg_stat_replication view).
  Other standby servers appearing later in this list represent potential
! synchronous standbys. If any of the current synchronous
! standbys disconnects for whatever reason,
  it will be replaced immediately with the next-highest-priority standby.
  Specifying more than one standby name can allow very high availability.
 
 
+ This parameter specifies a list of standby servers by using
+ either of the following syntaxes:
+ 
+ num_sync ( standby_name [, ...] )
+ standby_name [, ...]
+ 
+ where num_sync is
+ the number of synchronous standbys that transactions need to
+ wait for replies from,
+ and standby_name
+ is the name of a standby server. For example, a setting of
+ '3 (s1, s2, s3, s4)' makes transaction commits wait
+ until their WAL records are received by three higher priority standbys
+ chosen from standby servers s1, s2,
+ s3 and s4.
+ 
+ 
+ The second syntax was used before PostgreSQL
+ version 9.6 and is still supported. It's the same as the first syntax
+ with num_sync=1.
+ For example, both settings of '1 (s1, s2)' and
+ 's1, s2' have the same meaning; either s1
+ or s2 is chosen as a synchronous standby.
+
+
  The name of a standby server for this purpose is the
  application_name setting of the standby, as set in the
  primary_conninfo of the standby's WAL receiver.  There is
  no mechanism to enforce uniqueness. In case of duplicates one of the
! 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Tue, 5 Apr 2016 20:17:21 +0900, Fujii Masao  wrote in 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Masahiko Sawada]

On Tue, Apr 5, 2016 at 7:23 PM, Kyotaro HORIGUCHI
 wrote:
> At Mon, 4 Apr 2016 22:00:24 +0900, Masahiko Sawada  
> wrote in 
>> > For this case, the tree members of SyncRepConfig are '2[Sby1,',
>> > 'Sby2', "Sby3]'. This syntax is valid for the current
>> > specification but will surely get different meaning by the future
>> > changes. We should refuse this known-to-be-wrong-in-future syntax
>> > from now.
>>
>> I couldn't get your point but why will the above syntax meaning be
>> different from current meaning by future change?
>> I thought that another method uses another kind of parentheses.
>
> If the 'another kind of parehtheses' is a pair of brackets, an
> application_name 'tokyo[A]', for example, is currently allowed to
> occur unquoted in the list but will become disallowed by the
> syntax change.
>
>

Thank you for explaining.
I understood but since the future syntax is yet to be reached
consensus, I thought that it would be difficult  to refuse particular
kind of parentheses for now.

> > list_member_int() performs the loop internally. So I'm not sure how much
> > adding extra list_member_int() here can optimize this processing.
> > Another idea is to make SyncRepGetSyncStandby() check whether I'm sync
> > standby or not. In this idea, without adding extra loop, we can exit 
> > earilier
> > in the case where I'm not a sync standby. Does this make sense?
> The list_member_int() is also performed in the "(snip)" part. So
> SyncRepGetSyncStandbys() returning am_sync seems making sense.
>
> sync_standbys = SyncRepGetSyncStandbys(am_sync);
>
> /*
> *  Quick exit if I am not synchronous or there's not
> *  enough synchronous standbys
> * /
> if (!*am_sync || list_length(sync_standbys) < SyncRepConfig->num_sync)
> {
>  list_free(sync_standbys);
> return false;

I meant that it can skip to acquire spin lock at least, so it will
optimise that logic.
But anyway I agree with making SyncRepGetSyncStandbys returns am_sync variable.

-- 
Regards,

--
Masahiko Sawada


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Robert Haas]

On Mon, Apr 4, 2016 at 4:28 AM, Fujii Masao  wrote:
>>> + ereport(LOG,
>>> + (errmsg("standby \"%s\" is now the synchronous standby with priority %u",
>>> + application_name, MyWalSnd->sync_standby_priority)));
>>>
>>> s/ the / a /
>
> I have no objection to this change itself. But we have used this message
> in 9.5 or before, so if we apply this change, probably we need
> back-patching.

"the" implies that there can be only one synchronous standby at that
priority, while "a" implies that there could be more than one.  So the
situation might be different with this patch than previously.  (I
haven't read the patch so I don't know whether this is actually true,
but it might be what Thomas was going for.)

Also, I'd like to associate myself with the general happiness about
the prospect of having this feature in 9.6 (but without specifically
endorsing the code, since I have not read it).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Tue, Apr 5, 2016 at 3:15 PM, Fujii Masao  wrote:
>
> On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila 
wrote:
> > On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao 
wrote:
> >>
> >>
> >> Thanks for updating the patch!
> >>
> >> I applied the following changes to the patch.
> >> Attached is the revised version of the patch.
> >>
> >
> > 1.
> >{
> > {"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
> > gettext_noop("List of names of potential synchronous standbys."),
> > NULL,
> > GUC_LIST_INPUT
> > },
> > ,
> > "",
> > check_synchronous_standby_names, NULL, NULL
> > },
> >
> > Isn't it better to modify the description of synchronous_standby_names
in
> > guc.c based on new usage?
>
> What about "Number of synchronous standbys and list of names of
> potential synchronous ones"? Better idea?
>

Looks good.

>
> > 2.
> > pg_stat_get_wal_senders()
> > {
> > ..
> > /*
> > ! * Allocate and update the config data of synchronous replication,
> > ! * and then get the currently active synchronous standbys.
> >   */
> > + SyncRepUpdateConfig();
> >   LWLockAcquire(SyncRepLock, LW_SHARED);
> > ! sync_standbys = SyncRepGetSyncStandbys();
> >   LWLockRelease(SyncRepLock);
> > ..
> > }
> >
> > Why is it important to update the config with patch?  Earlier also any
> > update to config between calls wouldn't have been visible.
>
> Because a backend has no chance to call SyncRepUpdateConfig() and
> parse the latest value of s_s_names if SyncRepUpdateConfig() is not
> called here. This means that pg_stat_replication may return the
information
> based on the old value of s_s_names.
>

Thats right, but without this patch also won't pg_stat_replication can show
old information? If no, why so?

> > 3.
> >   Planning for High Availability
> >
> >  
> > ! synchronous_standby_names specifies the number of
> > ! synchronous standbys that transaction commits made when
> >
> > Is it better to say like: synchronous_standby_names
specifies
> > the number and names of
>
> Precisely s_s_names specifies a list of names of potential sync standbys
> not sync ones.
>

Okay, but you doesn't seem to have updated this in your latest patch.

> > 4.
> > + /*
> > +  * Return the list of sync standbys, or NIL if no sync standby is
> > connected.
> > +  *
> > +  * If there are multiple standbys with the same priority,
> > +  * the first one found is considered as higher priority.
> >
> > Here line indentation of second line can be improved.
>
> What about "the first one found is selected first"? Or better idea?
>

What I was complaining about that few words from second line can be moved
to previous line, but may be pgindent will take care of same, so no need to
worry.


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 5 April 2016 at 12:26, Fujii Masao  wrote:


> Multiple standbys with the same name may connect to the master.
> In this case, users might want to specifiy k<=N. So k<=N seems not invalid
> setting.


Confusing as that is, it is already the case; k > N could make sense. ;-(

However, in most cases, k > N would not make sense and we should issue a
WARNING.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 8:08 PM, Simon Riggs  wrote:
> On 5 April 2016 at 11:18, Fujii Masao  wrote:
>
>>
>> > 1. Header comments in syncrep.c need changes, not just additions.
>>
>> Okay, will consider this later. And I'd appreciate if you elaborate what
>> changes are necessary specifically.
>
>
> Some of the old header comments are now wrong.

Okay, will check.

>> > 2. We need tests to ensure that k >=1 and k<=N
>>
>> The changes to replication test framework was included in the patch
>> before,
>> but I excluded it from the patch because I'd like to commit the core part
>> of
>> the patch first. Will review the test part later.
>
>
> I meant tests of setting the parameters, not tests of the feature itself.

k<=0 causes an error while parsing s_s_names in current patch.

Regarding the test of k<=N, you mean that an error should be emitted
when k is larger than or equal to the number of standby names in the list?
Multiple standbys with the same name may connect to the master.
In this case, users might want to specifiy k<=N. So k<=N seems not invalid
setting.

>> > 3. There should be a WARNING if k == N to say that we don't yet provide
>> > a
>> > level to give Apply consistency. (I mean if we specify 2 (n1, n2) or
>> > 3(n1,
>> > n2, n3) etc
>>
>> Sorry I failed to get your point. Could you tell me what Apply consistency
>> and why we cannot provide it when k = N?
>>
>> > 4. How does it work?
>> > It's pretty strange, but that isn't documented anywhere. It took me a
>> > while
>> > to figure it out even though I know that code. My thought is its a lot
>> > slower than before, which is a concern when we know by definition that k
>> > >=2
>> > for the new feature. I was going to mention the fact that this code only
>> > needs to be executed by standbys mentioned in s_s_n, so we can avoid
>> > overhead and contention for async standbys (But Masahiko just mentioned
>> > that
>> > also).
>>
>> Unless I'm missing something, the patch already avoids the overhead
>> of async standbys. Please see the top of SyncRepReleaseWaiters().
>> Since async standbys exit at the beginning of SyncRepReleaseWaiters(),
>> they don't need to perform any operations that the patch adds
>> (e.g., find out which standbys are synchronous).
>
>
> I was thinking about the overhead of scanning through the full list of
> WALSenders for each message, when it is a sync standby.

This is true even in current release or before.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 5 April 2016 at 11:23, Fujii Masao  wrote:

> On Tue, Apr 5, 2016 at 6:09 PM, Simon Riggs  wrote:
> > On 5 April 2016 at 08:58, Amit Langote 
> > wrote:
> >
> >>
> >>  So I am suggesting we put an extra keyword in front of the “k”, to
> >> > explain how the k responses should be gathered as an extension to the
> >> > the
> >> > syntax. I also think implementing “any k” is actually fairly trivial
> and
> >> > could be done for 9.6 (rather than just "first k").
> >>
> >> +1 for 'first/any k (...)', with possibly only 'first' supported for
> now,
> >> if the 'any' case is more involved than we would like to spend time on,
> >> given the time considerations. IMHO, the extra keyword adds to clarity
> of
> >> the syntax.
> >
> >
> > Further thoughts:
> >
> > I said "any k" was faster, though what I mean is both faster and more
> > robust. If you have network peaks from any of the k sync standbys then
> the
> > user will wait longer. With "any k", if a network peak occurs, then
> another
> > standby response will work just as well. So the performance of "any k"
> will
> > be both faster, more consistent and less prone to misconfiguration.
> >
> > I also didn't explain why I think it is easy to implement "any k".
> >
> > All we need to do is change SyncRepGetOldestSyncRecPtr() so that it
> returns
> > the k'th oldest pointer of any named standby.
>
> s/oldest/newest ?
>

Sure


> > Then use that to wake up user
> > backends. So the change requires only slightly modified logic in a very
> > isolated part of the code, almost all of which would be code inserts to
> cope
> > with the new option.
>
> Yes. Probably we need to use some time to find what algorithm is the best
> for searching the k'th newest pointer.
>

I think we would all agree an insertion sort would be the fastest for k ~
2-5, no much discussion there.

We do already use that in this section of code, namely SHMQueue.


> > The syntax and doc changes would take a couple of
> > hours.
>
> Yes, the updates of documentation would need more time.
>

I can help, if you wish that.

"any k" is in my mind what people would be expecting us to deliver with
this feature, which is why I suggest it now, especially since it is a small
additional item.

Please don't see these comments as blocking your progress to commit.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 7:17 PM, Kyotaro HORIGUCHI
 wrote:
> At Tue, 5 Apr 2016 18:08:20 +0900, Fujii Masao  wrote 
> in 
>> On Mon, Apr 4, 2016 at 10:00 PM, Masahiko Sawada  
>> wrote:
>> > On Mon, Apr 4, 2016 at 6:03 PM, Kyotaro HORIGUCHI
>> >  wrote:
>> >> Hello, thank you for testing.
>> >>
>> >> At Sat, 2 Apr 2016 14:20:55 +1300, Thomas Munro 
>> >>  wrote in 
>> >> 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 5 April 2016 at 11:18, Fujii Masao  wrote:


> > 1. Header comments in syncrep.c need changes, not just additions.
>
> Okay, will consider this later. And I'd appreciate if you elaborate what
> changes are necessary specifically.


Some of the old header comments are now wrong.


> > 2. We need tests to ensure that k >=1 and k<=N
>
> The changes to replication test framework was included in the patch before,
> but I excluded it from the patch because I'd like to commit the core part
> of
> the patch first. Will review the test part later.


I meant tests of setting the parameters, not tests of the feature itself.


> >
> > 3. There should be a WARNING if k == N to say that we don't yet provide a
> > level to give Apply consistency. (I mean if we specify 2 (n1, n2) or
> 3(n1,
> > n2, n3) etc
>
> Sorry I failed to get your point. Could you tell me what Apply consistency
> and why we cannot provide it when k = N?
>
> > 4. How does it work?
> > It's pretty strange, but that isn't documented anywhere. It took me a
> while
> > to figure it out even though I know that code. My thought is its a lot
> > slower than before, which is a concern when we know by definition that k
> >=2
> > for the new feature. I was going to mention the fact that this code only
> > needs to be executed by standbys mentioned in s_s_n, so we can avoid
> > overhead and contention for async standbys (But Masahiko just mentioned
> that
> > also).
>
> Unless I'm missing something, the patch already avoids the overhead
> of async standbys. Please see the top of SyncRepReleaseWaiters().
> Since async standbys exit at the beginning of SyncRepReleaseWaiters(),
> they don't need to perform any operations that the patch adds
> (e.g., find out which standbys are synchronous).
>

I was thinking about the overhead of scanning through the full list of
WALSenders for each message, when it is a sync standby.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 6:09 PM, Simon Riggs  wrote:
> On 5 April 2016 at 08:58, Amit Langote 
> wrote:
>
>>
>>  So I am suggesting we put an extra keyword in front of the “k”, to
>> > explain how the k responses should be gathered as an extension to the
>> > the
>> > syntax. I also think implementing “any k” is actually fairly trivial and
>> > could be done for 9.6 (rather than just "first k").
>>
>> +1 for 'first/any k (...)', with possibly only 'first' supported for now,
>> if the 'any' case is more involved than we would like to spend time on,
>> given the time considerations. IMHO, the extra keyword adds to clarity of
>> the syntax.
>
>
> Further thoughts:
>
> I said "any k" was faster, though what I mean is both faster and more
> robust. If you have network peaks from any of the k sync standbys then the
> user will wait longer. With "any k", if a network peak occurs, then another
> standby response will work just as well. So the performance of "any k" will
> be both faster, more consistent and less prone to misconfiguration.
>
> I also didn't explain why I think it is easy to implement "any k".
>
> All we need to do is change SyncRepGetOldestSyncRecPtr() so that it returns
> the k'th oldest pointer of any named standby.

s/oldest/newest ?

> Then use that to wake up user
> backends. So the change requires only slightly modified logic in a very
> isolated part of the code, almost all of which would be code inserts to cope
> with the new option.

Yes. Probably we need to use some time to find what algorithm is the best
for searching the k'th newest pointer.

> The syntax and doc changes would take a couple of
> hours.

Yes, the updates of documentation would need more time.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Mon, 4 Apr 2016 22:00:24 +0900, Masahiko Sawada  
wrote in 
> > For this case, the tree members of SyncRepConfig are '2[Sby1,',
> > 'Sby2', "Sby3]'. This syntax is valid for the current
> > specification but will surely get different meaning by the future
> > changes. We should refuse this known-to-be-wrong-in-future syntax
> > from now.
> 
> I couldn't get your point but why will the above syntax meaning be
> different from current meaning by future change?
> I thought that another method uses another kind of parentheses.

If the 'another kind of parehtheses' is a pair of brackets, an
application_name 'tokyo[A]', for example, is currently allowed to
occur unquoted in the list but will become disallowed by the
syntax change.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 4:35 PM, Simon Riggs  wrote:
> On 4 April 2016 at 10:35, Simon Riggs  wrote:
>>
>> On 4 April 2016 at 09:28, Fujii Masao  wrote:
>>
>>>
>>> Barring any objections, I'll commit this patch.
>>
>>
>> That sounds good.
>>
>> May I have one more day to review this? Actually more like 3-4 hours.
>
>
> What we have here is useful and elegant. I love the simplicity and backwards
> compatibility of the design. Very nice, chef.
>
> I am in favour of committing something for 9.6, though I do have some
> objective comments

Thanks for the review!

> 1. Header comments in syncrep.c need changes, not just additions.

Okay, will consider this later. And I'd appreciate if you elaborate what
changes are necessary specifically.

> 2. We need tests to ensure that k >=1 and k<=N

The changes to replication test framework was included in the patch before,
but I excluded it from the patch because I'd like to commit the core part of
the patch first. Will review the test part later.

>
> 3. There should be a WARNING if k == N to say that we don't yet provide a
> level to give Apply consistency. (I mean if we specify 2 (n1, n2) or 3(n1,
> n2, n3) etc

Sorry I failed to get your point. Could you tell me what Apply consistency
and why we cannot provide it when k = N?

> 4. How does it work?
> It's pretty strange, but that isn't documented anywhere. It took me a while
> to figure it out even though I know that code. My thought is its a lot
> slower than before, which is a concern when we know by definition that k >=2
> for the new feature. I was going to mention the fact that this code only
> needs to be executed by standbys mentioned in s_s_n, so we can avoid
> overhead and contention for async standbys (But Masahiko just mentioned that
> also).

Unless I'm missing something, the patch already avoids the overhead
of async standbys. Please see the top of SyncRepReleaseWaiters().
Since async standbys exit at the beginning of SyncRepReleaseWaiters(),
they don't need to perform any operations that the patch adds
(e.g., find out which standbys are synchronous).

> 5. Timing – k > 1 will be slower by definition and more complex to
> configure, yet there is no timing facility to measure the effect of this,
> even though we have a new timing facility in 9.6. It would be useful to have
> a track_syncrep option to keep track of typical response times from nodes.

Maybe it's useful. But it seems completely new feature, so I'm not sure
if we have enough time to push it to 9.6. Probably it's for 9.7.

> 6. Meaning of k (n1, n2, n3) with N servers
>
> It's clearly documented that this means k replies IN SEQUENCE. I believe the
> typical meaning of would be “any k out of N”, which would be faster than
> what we have, e.g.
>3 (n1, n2, n3) would release as soon as (n1, n2) or (n2, n3) or (n1, n3)
> acknowledge.
>
> The “any k” option is not currently possible, but could be fairly easily.
> The syntax should also be easily extensible.
>
> I would call what we have now “first” semantics, and we could have both of
> these...
>
> * first k (n1, n2, n3) – does the same as k (n1, n2, n3) does now
> * any k (n1, n2, n3) – would release waiters as soon as we have the
> responses from k out of N standbys. “any k” would be faster, so is desirable
> for performance and resilience

We discussed the syntax very long time, so restarting the discussion
and keeping the patch uncommited is not good. We might fail to commit
anything about N-sync rep in 9.6. So let's commit the current patch first
and restart the discussion later.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Kyotaro HORIGUCHI]

At Tue, 5 Apr 2016 18:08:20 +0900, Fujii Masao  wrote in 

> On Mon, Apr 4, 2016 at 10:00 PM, Masahiko Sawada  
> wrote:
> > On Mon, Apr 4, 2016 at 6:03 PM, Kyotaro HORIGUCHI
> >  wrote:
> >> Hello, thank you for testing.
> >>
> >> At Sat, 2 Apr 2016 14:20:55 +1300, Thomas Munro 
> >>  wrote in 
> >> 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Tue, Apr 5, 2016 at 4:31 PM, Amit Kapila  wrote:
> On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao  wrote:
>>
>>
>> Thanks for updating the patch!
>>
>> I applied the following changes to the patch.
>> Attached is the revised version of the patch.
>>
>
> 1.
>{
> {"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
> gettext_noop("List of names of potential synchronous standbys."),
> NULL,
> GUC_LIST_INPUT
> },
> ,
> "",
> check_synchronous_standby_names, NULL, NULL
> },
>
> Isn't it better to modify the description of synchronous_standby_names in
> guc.c based on new usage?

What about "Number of synchronous standbys and list of names of
potential synchronous ones"? Better idea?

> 2.
> pg_stat_get_wal_senders()
> {
> ..
> /*
> ! * Allocate and update the config data of synchronous replication,
> ! * and then get the currently active synchronous standbys.
>   */
> + SyncRepUpdateConfig();
>   LWLockAcquire(SyncRepLock, LW_SHARED);
> ! sync_standbys = SyncRepGetSyncStandbys();
>   LWLockRelease(SyncRepLock);
> ..
> }
>
> Why is it important to update the config with patch?  Earlier also any
> update to config between calls wouldn't have been visible.

Because a backend has no chance to call SyncRepUpdateConfig() and
parse the latest value of s_s_names if SyncRepUpdateConfig() is not
called here. This means that pg_stat_replication may return the information
based on the old value of s_s_names.

> 3.
>   Planning for High Availability
>
>  
> ! synchronous_standby_names specifies the number of
> ! synchronous standbys that transaction commits made when
>
> Is it better to say like: synchronous_standby_names specifies
> the number and names of

Precisely s_s_names specifies a list of names of potential sync standbys
not sync ones.

> 4.
> + /*
> +  * Return the list of sync standbys, or NIL if no sync standby is
> connected.
> +  *
> +  * If there are multiple standbys with the same priority,
> +  * the first one found is considered as higher priority.
>
> Here line indentation of second line can be improved.

What about "the first one found is selected first"? Or better idea?

>
> ! /*
> ! * syncrep_yyparse sets the global syncrep_parse_result as side effect.
> ! * But this function is required to just check, so frees it
> ! * once parsing parameter.
> ! */
> ! SyncRepFreeConfig(syncrep_parse_result);
>
> How about below change in comment?
> /so frees it once parsing parameter/so frees it after parsing the parameter

Will apply this to the patch.

Thanks for the review!

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Andres Freund]

On 2016-04-05 10:13:50 +0100, Simon Riggs wrote:
> The lack of per-database settings is not a blocker for me.

Just to clarify: Neither is it for me.


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 5 April 2016 at 10:10, Fujii Masao  wrote:

> On Mon, Apr 4, 2016 at 6:45 PM, Andres Freund  wrote:
> > On 2016-04-04 10:35:34 +0100, Simon Riggs wrote:
> >> On 4 April 2016 at 09:28, Fujii Masao  wrote:
> >> > Barring any objections, I'll commit this patch.
> >
> > No objection here either, just one question: Has anybody thought about
> > the ability to extend this to do per-database syncrep?
>
> Nope at least for me... You'd like to extend synchronous_standby_names
> so that users can specify that per-database?
>

As requested, I did consider whether we could have syntax for per-database
settings.

ISTM that it is already possible to have one database in async mode and
another in sync mode, using settings of synchronous_commit.

The easiest way to have per-database settings if you want more is to use
different instances. Adding a dbname into the syntax would complicate it
significantly and even if we agreed that, I don't think it would happen for
9.6. The lack of per-database settings is not a blocker for me.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 4, 2016 at 6:45 PM, Andres Freund  wrote:
> On 2016-04-04 10:35:34 +0100, Simon Riggs wrote:
>> On 4 April 2016 at 09:28, Fujii Masao  wrote:
>> > Barring any objections, I'll commit this patch.
>
> No objection here either, just one question: Has anybody thought about
> the ability to extend this to do per-database syncrep?

Nope at least for me... You'd like to extend synchronous_standby_names
so that users can specify that per-database?

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 5 April 2016 at 08:58, Amit Langote 
wrote:


>  So I am suggesting we put an extra keyword in front of the “k”, to
> > explain how the k responses should be gathered as an extension to the the
> > syntax. I also think implementing “any k” is actually fairly trivial and
> > could be done for 9.6 (rather than just "first k").
>
> +1 for 'first/any k (...)', with possibly only 'first' supported for now,
> if the 'any' case is more involved than we would like to spend time on,
> given the time considerations. IMHO, the extra keyword adds to clarity of
> the syntax.
>

Further thoughts:

I said "any k" was faster, though what I mean is both faster and more
robust. If you have network peaks from any of the k sync standbys then the
user will wait longer. With "any k", if a network peak occurs, then another
standby response will work just as well. So the performance of "any k" will
be both faster, more consistent and less prone to misconfiguration.

I also didn't explain why I think it is easy to implement "any k".

All we need to do is change SyncRepGetOldestSyncRecPtr() so that it returns
the k'th oldest pointer of any named standby. Then use that to wake up user
backends. So the change requires only slightly modified logic in a very
isolated part of the code, almost all of which would be code inserts to
cope with the new option. The syntax and doc changes would take a couple of
hours.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 4, 2016 at 10:00 PM, Masahiko Sawada  wrote:
> On Mon, Apr 4, 2016 at 6:03 PM, Kyotaro HORIGUCHI
>  wrote:
>> Hello, thank you for testing.
>>
>> At Sat, 2 Apr 2016 14:20:55 +1300, Thomas Munro 
>>  wrote in 
>> 

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Fujii Masao]

On Mon, Apr 4, 2016 at 5:59 PM, Abhijit Menon-Sen  wrote:
> At 2016-04-04 17:28:07 +0900, masao.fu...@gmail.com wrote:
>>
>> Barring any objections, I'll commit this patch.
>
> No objections, just a minor wording tweak:
>
> doc/src/sgml/config.sgml:
>
> "The synchronous standbys will be the standbys that their names appear
> early in this list" should be "The synchronous standbys will be those
> whose names appear earlier in this list".
>
> doc/src/sgml/high-availability.sgml:
>
> "The standbys that their names appear early in this list are given
> higher priority and will be considered as synchronous" should be "The
> standbys whose names appear earlier in the list are given higher
> priority and will be considered as synchronous".
>
> "The standbys that their names appear early in the list will be used as
> the synchronous standby" should be "The standbys whose names appear
> earlier in the list will be used as synchronous standbys".
>
> You may prefer to reword this in some other way, but the current "that
> their names appear" wording should be changed.

Thanks for the review! Will apply these comments to new patch.

Regards,

-- 
Fujii Masao


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Langote]

On 2016/04/05 16:35, Simon Riggs wrote:
> 6. Meaning of k (n1, n2, n3) with N servers
> 
> It's clearly documented that this means k replies IN SEQUENCE. I believe
> the typical meaning of would be “any k out of N”, which would be faster
> than what we have, e.g.
>3 (n1, n2, n3) would release as soon as (n1, n2) or (n2, n3) or (n1, n3)
> acknowledge.
> 
> The “any k” option is not currently possible, but could be fairly easily.
> The syntax should also be easily extensible.
> 
> I would call what we have now “first” semantics, and we could have both of
> these...
> 
> * first k (n1, n2, n3) – does the same as k (n1, n2, n3) does now
> * any k (n1, n2, n3) – would release waiters as soon as we have the
> responses from k out of N standbys. “any k” would be faster, so is
> desirable for performance and resilience
> 
 So I am suggesting we put an extra keyword in front of the “k”, to
> explain how the k responses should be gathered as an extension to the the
> syntax. I also think implementing “any k” is actually fairly trivial and
> could be done for 9.6 (rather than just "first k").

+1 for 'first/any k (...)', with possibly only 'first' supported for now,
if the 'any' case is more involved than we would like to spend time on,
given the time considerations. IMHO, the extra keyword adds to clarity of
the syntax.

Thanks,
Amit




-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 4 April 2016 at 10:35, Simon Riggs  wrote:

> On 4 April 2016 at 09:28, Fujii Masao  wrote:
>
>
>> Barring any objections, I'll commit this patch.
>>
>
> That sounds good.
>
> May I have one more day to review this? Actually more like 3-4 hours.
>

What we have here is useful and elegant. I love the simplicity and
backwards compatibility of the design. Very nice, chef.

I am in favour of committing something for 9.6, though I do have some
objective comments

1. Header comments in syncrep.c need changes, not just additions.

2. We need tests to ensure that k >=1 and k<=N

3. There should be a WARNING if k == N to say that we don't yet provide a
level to give Apply consistency. (I mean if we specify 2 (n1, n2) or 3(n1,
n2, n3) etc

4. How does it work?
It's pretty strange, but that isn't documented anywhere. It took me a while
to figure it out even though I know that code. My thought is its a lot
slower than before, which is a concern when we know by definition that k
>=2 for the new feature. I was going to mention the fact that this code
only needs to be executed by standbys mentioned in s_s_n, so we can avoid
overhead and contention for async standbys (But Masahiko just mentioned
that also).

5. Timing – k > 1 will be slower by definition and more complex to
configure, yet there is no timing facility to measure the effect of this,
even though we have a new timing facility in 9.6. It would be useful to
have a track_syncrep option to keep track of typical response times from
nodes.

6. Meaning of k (n1, n2, n3) with N servers

It's clearly documented that this means k replies IN SEQUENCE. I believe
the typical meaning of would be “any k out of N”, which would be faster
than what we have, e.g.
   3 (n1, n2, n3) would release as soon as (n1, n2) or (n2, n3) or (n1, n3)
acknowledge.

The “any k” option is not currently possible, but could be fairly easily.
The syntax should also be easily extensible.

I would call what we have now “first” semantics, and we could have both of
these...

* first k (n1, n2, n3) – does the same as k (n1, n2, n3) does now
* any k (n1, n2, n3) – would release waiters as soon as we have the
responses from k out of N standbys. “any k” would be faster, so is
desirable for performance and resilience

>>> So I am suggesting we put an extra keyword in front of the “k”, to
explain how the k responses should be gathered as an extension to the the
syntax. I also think implementing “any k” is actually fairly trivial and
could be done for 9.6 (rather than just "first k").



Future thoughts that relate to syntax choices now, not for 9.6

Eventually I would want to be able to specify this…
   2 ( any (london1, london2), any (nyc1, nyc2))
meaning I want a response from at least 1 London server and at least one
NYC server, but whichever one responds first doesn't matter.

And I also want to be able to specify node groups in there. So elsewhere we
would specify London node group as (London1, London2) and NYC node group as
(NYC1, NYC2) and then specify

any 2 (London, NYC, Tokyo).


Good work

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Amit Kapila]

On Mon, Apr 4, 2016 at 1:58 PM, Fujii Masao  wrote:
>
>
> Thanks for updating the patch!
>
> I applied the following changes to the patch.
> Attached is the revised version of the patch.
>

1.
   {
{"synchronous_standby_names", PGC_SIGHUP, REPLICATION_MASTER,
gettext_noop("List of names of potential synchronous standbys."),
NULL,
GUC_LIST_INPUT
},
,
"",
check_synchronous_standby_names, NULL, NULL
},

Isn't it better to modify the description of synchronous_standby_names in
guc.c based on new usage?

2.
pg_stat_get_wal_senders()
{
..
/*
! * Allocate and update the config data of synchronous replication,
! * and then get the currently active synchronous standbys.
  */
+ SyncRepUpdateConfig();
  LWLockAcquire(SyncRepLock, LW_SHARED);
! sync_standbys = SyncRepGetSyncStandbys();
  LWLockRelease(SyncRepLock);
..
}

Why is it important to update the config with patch?  Earlier also any
update to config between calls wouldn't have been visible.


3.
  Planning for High Availability

 
! synchronous_standby_names specifies the number of
! synchronous standbys that transaction commits made when

Is it better to say like: synchronous_standby_names specifies
the number and names of


4.
+ /*
+  * Return the list of sync standbys, or NIL if no sync standby is
connected.
+  *
+  * If there are multiple standbys with the same priority,
+  * the first one found is considered as higher priority.

Here line indentation of second line can be improved.

5.
! /*
! * syncrep_yyparse sets the global syncrep_parse_result as side effect.
! * But this function is required to just check, so frees it
! * once parsing parameter.
! */
! SyncRepFreeConfig(syncrep_parse_result);

How about below change in comment?
/so frees it once parsing parameter/so frees it after parsing the parameter


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Support for N synchronous standby servers - take 2

[Simon Riggs]

On 4 April 2016 at 10:45, Andres Freund  wrote:

>
> Simon, perhaps you could hold the above question in your mind while
> looking through this?
>

Sure, np.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services