Re: Support for N synchronous standby servers - take 2 - Mailing list pgsql-hackers

From Amit Kapila
Subject Re: Support for N synchronous standby servers - take 2
Date
Msg-id CAA4eK1KGVrQTueP2Rijjg_FNQ_TU3n5rt8-X5a0LaEzUQ-+i-Q@mail.gmail.com
Whole thread Raw
In response to Re: Support for N synchronous standby servers - take 2  (Kyotaro HORIGUCHI <horiguchi.kyotaro@lab.ntt.co.jp>)
Responses Re: Support for N synchronous standby servers - take 2
List pgsql-hackers
On Tue, Apr 26, 2016 at 9:15 AM, Kyotaro HORIGUCHI <horiguchi.kyotaro@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 <horiguchi.kyotaro@lab.ntt.co.jp> wrote in <20160426.110225.35506931.horiguchi.kyotaro@lab.ntt.co.jp>
> At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in <476.1461420723@sss.pgh.pa.us>
> > Amit Kapila <amit.kapila16@gmail.com> writes:
> > > The main point for this improvement is that the handling for guc s_s_names
> > > is not similar to what we do for other somewhat similar guc's and which
> > > causes in-efficiency in non-hot code path (less used code).
> >
> > This is not about efficiency, this is about correctness.  The proposed
> > v7 patch is flat out not acceptable, not now and not for 9.7 either,
> > because it introduces a GUC assign hook that can easily fail (eg, through
> > out-of-memory for the copy step).  Assign hook functions need to be
> > incapable of failure.

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

pgsql-hackers by date:

Previous
From: Michael Paquier
Date:
Subject: Re: Bogus cleanup code in PostgresNode.pm
Next
From: Tom Lane
Date:
Subject: Re: Bogus cleanup code in PostgresNode.pm