On Tue, Apr 26, 2016 at 9:15 AM, Kyotaro HORIGUCHI < horiguchi.kyot...@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.kyot...@lab.ntt.co.jp> wrote in < > 20160426.110225.35506931.horiguchi.kyot...@lab.ntt.co.jp> > > At Sat, 23 Apr 2016 10:12:03 -0400, Tom Lane <t...@sss.pgh.pa.us> wrote > in <476.1461420...@sss.pgh.pa.us> > > > 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 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
