Re: Support for N synchronous standby servers - take 2

On Tue, Apr 26, 2016 at 9:15 AM, Kyotaro HORIGUCHI <
horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:

> Hello, attached is the new version v8.
>
> At Tue, 26 Apr 2016 11:02:25 +0900 (Tokyo Standard Time), Kyotaro
> HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote in <
> 20160426(dot)110225(dot)35506931(dot)horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
> > At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote
> in <476(dot)1461420723(at)sss(dot)pgh(dot)pa(dot)us>
> > > Amit Kapila <amit(dot)kapila16(at)gmail(dot)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