Re: Semantics of pg_file_settings view

On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> 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.

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?

> (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.

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.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com