Semantics of pg_file_settings view

I looked into bug #13471, which states that we gripe about multiple
occurrences of the same variable in postgresql.conf + related files.
Now, that had clearly been fixed some time ago:

Author: Fujii Masao <fujii(at)postgresql(dot)org>
Branch: master [e3da0d4d1] 2014-08-06 14:49:43 +0900
Branch: REL9_4_STABLE Release: REL9_4_0 [cf6a9c374] 2014-08-06 14:50:30 +0900

Change ParseConfigFp() so that it doesn't process unused entry of each parameter.

... however, it seems I removed that again in a cleanup commit awhile
later :-(. I think I'd meant to move it somewhere else, or maybe even
fix it to not be O(N^2), but clearly forgot while dealing with assorted
unrelated fallout from the ALTER SYSTEM patch.

However putting it back now would be problematic, because of the recent
introduction of the pg_file_settings view, which purports to display
all entries in the config files, even ones which got overridden by later
duplicate entries. If we just put back Fujii-san's code then only the
last duplicate entry will be visible in pg_file_settings, which seems to
destroy the rationale for having that view at all.

What we evidently need to do is fix things so that the pg_file_settings
data gets captured before we suppress duplicates.

In view of that, I am wondering whether the current placement of that
data-capturing code was actually designed intentionally, or if it was
chosen by throwing a dart at the source code. The latter seems more
likely, because we don't capture the data until after we've decided
that all the entries seem provisionally valid, ie the stanza headed

* Check if all the supplied option names are valid, as an additional
* quasi-syntactic check on the validity of the config file. It is

in guc-file.l. ISTM that there is a good argument for capturing the data
before that, so that it's updated by any SIGHUP, whether or not we
conclude that the config file(s) are valid enough to apply data from.
This would mean that the view might contain data about "settings" that
aren't valid GUC variables. But I fail to see why that's a bad thing,
if the main use-case is to debug problems with the config files.

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.
(You could even imagine being able to finger syntax errors in the view
that way, by having ParseConfigFile attach a dummy ConfigVariable entry
when it bails out.)

The reason I started looking at this is that the loop where we set
GUC_IS_IN_FILE seems like the most natural place to deal with removing
duplicates, since it can notice more or less for free whether there are
any. But as the code stands, that's still too early.

Thoughts?

regards, tom lane