Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What we evidently need to do is fix things so that the pg_file_settings
>> data gets captured before we suppress duplicates.
>>
>> The simplest change would be to move the whole thing to around line 208 in
>> guc-file.l, just after the stanza that loads PG_AUTOCONF_FILENAME. Or you
>> could argue that the approach is broken altogether, and that we should
>> capture the data while we read the files, so that you have some useful
>> data in the view even if ParseConfigFile later decides there's a syntax
>> error. I'm actually thinking maybe we should flush that data-capturing
>> logic altogether in favor of just not deleting the ConfigVariable list
>> data structure, and generating the view directly from that data structure.
> Idea for generating view form ConfigVariable list sounds good, but how
> will it preserve the duplicate entries in the list assuming either we need
> to revert the original fix (e3da0d4d1) or doing the same in loop where
> we set GUC_IS_IN_FILE?
I'm thinking of adding an "ignore" boolean flag to ConfigVariable, which
the GUC_IS_IN_FILE loop would set in ConfigVariables that are superseded
by later list entries. Then the GUC-application loop would just skip
those entries. This would be good because the flag could be displayed
somehow in the pg_file_settings view, whereas right now you have to
manually check for duplicates.
> Keeping removal of duplicate items in ParseConfigFp() has the advantage
> that it will work for all other places from where ParseConfigFp() is called,
> though I am not sure if today that is required.
I don't think it is; if it were, we'd have had other complaints about
that, considering that 9.4.0 is the *only* release we've ever shipped
that suppressed duplicates right inside ParseConfigFp(). I would in
fact turn that argument on its head, and state that Fujii-san's patch
was probably ill-conceived because it implicitly assumes that duplicate
suppression is okay for every caller of ParseConfigFp. It's not hard
to imagine use-cases that that would break, even if we have none today.
regards, tom lane
