At Tue, 26 Apr 2016 09:57:50 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote
> > > > Amit Kapila <amit.kapil...@gmail.com> writes:
> > > > > The main point for this improvement is that the handling for guc
> > s_s_names
> > > > > is not similar to what we do for other somewhat similar guc's and
> > which
> > > > > causes in-efficiency in non-hot code path (less used code).
> > > >
> > > > This is not about efficiency, this is about correctness. The proposed
> > > > v7 patch is flat out not acceptable, not now and not for 9.7 either,
> > > > because it introduces a GUC assign hook that can easily fail (eg,
> > through
> > > > out-of-memory for the copy step). Assign hook functions need to be
> > > > incapable of failure.
> 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.
NTT Open Source Software Center
Sent via pgsql-hackers mailing list (firstname.lastname@example.org)
To make changes to your subscription: