Hello, At Tue, 26 Apr 2016 09:57:50 +0530, Amit Kapila <amit.kapil...@gmail.com> wrote in <caa4ek1kgvrqtuep2rijjg_fnq_tu3n5rt8-x5a0laezuq-+...@mail.gmail.com> > > > > 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. regards, -- Kyotaro Horiguchi NTT Open Source Software Center
-- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers