Re: Support for N synchronous standby servers - take 2

Hello,

At Tue, 26 Apr 2016 09:57:50 +0530, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote in <CAA4eK1KGVrQTueP2Rijjg_FNQ_TU3n5rt8-X5a0LaEzUQ-+i-Q(at)mail(dot)gmail(dot)com>
> > > > 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 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