Re: Semantics of pg_file_settings view

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Combining this with my idea about preserving the ConfigVariable list,
>> I'm thinking that it would be a good idea for ProcessConfigFile() to
>> run in a context created for the purpose of processing the config files,
>> rather than blindly using the caller's context, which is likely to be
>> a process-lifespan context and thus not a good place to leak in.
>> We could keep this context around until the next SIGHUP event, so that
>> the ConfigVariable list remains available, and then destroy it and
>> replace it with the next ProcessConfigFile's instance of the context.
>> In this way, any leakage in the processing code could not accumulate
>> over multiple SIGHUPs, and so it would be certain to remain fairly
>> negligible.

> That seems like a nice idea.

Attached is a WIP version of this idea. It lacks docs, and there is one
further API change I'd like to discuss, but what is there so far is:

1. It implements the above idea of doing SIGHUP work in a dedicated
context that gets flushed at the next SIGHUP, and using the
ConfigVariables list directly as the source data for the pg_file_settings
view.

2. It adds an "error message" field to struct ConfigVariable, and a
corresponding column to the pg_file_settings view, allowing problems
to be reported. Some examples of what it can do:

Normal case with an initdb-generated postgresql.conf:

regression=# select * from pg_file_settings;
sourcefile | sourceline | seqno | name | setting | error
-------------------------------------------------+------------+-------+----------------------------+--------------------+-------
/home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections | 100 |
/home/postgres/testversion/data/postgresql.conf | 116 | 2 | shared_buffers | 128MB |
/home/postgres/testversion/data/postgresql.conf | 131 | 3 | dynamic_shared_memory_type | posix |
/home/postgres/testversion/data/postgresql.conf | 446 | 4 | log_timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 533 | 5 | datestyle | iso, mdy |
/home/postgres/testversion/data/postgresql.conf | 535 | 6 | timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 548 | 7 | lc_messages | C |
/home/postgres/testversion/data/postgresql.conf | 550 | 8 | lc_monetary | C |
/home/postgres/testversion/data/postgresql.conf | 551 | 9 | lc_numeric | C |
/home/postgres/testversion/data/postgresql.conf | 552 | 10 | lc_time | C |
/home/postgres/testversion/data/postgresql.conf | 555 | 11 | default_text_search_config | pg_catalog.english |
(11 rows)

postgresql.conf is not there/not readable:

regression=# select * from pg_file_settings;
sourcefile | sourceline | seqno | name | setting | error
------------+------------+-------+------+---------+-----------------------------------------------------------------------
| 0 | 1 | | | could not open file "/home/postgres/testversion/data/postgresql.conf"
(1 row)

Bad include_dir entry:

sourcefile | sourceline | seqno | name | setting | error

-------------------------------------------------+------------+-------+----------------------------+--------------------+--------------------------------------------------------------------
/home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections | 100 |
/home/postgres/testversion/data/postgresql.conf | 116 | 2 | shared_buffers | 128MB |
/home/postgres/testversion/data/postgresql.conf | 131 | 3 | dynamic_shared_memory_type | posix |
/home/postgres/testversion/data/postgresql.conf | 446 | 4 | log_timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 533 | 5 | datestyle | iso, mdy |
/home/postgres/testversion/data/postgresql.conf | 535 | 6 | timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 548 | 7 | lc_messages | C |
/home/postgres/testversion/data/postgresql.conf | 550 | 8 | lc_monetary | C |
/home/postgres/testversion/data/postgresql.conf | 551 | 9 | lc_numeric | C |
/home/postgres/testversion/data/postgresql.conf | 552 | 10 | lc_time | C |
/home/postgres/testversion/data/postgresql.conf | 555 | 11 | default_text_search_config | pg_catalog.english |
/home/postgres/testversion/data/postgresql.conf | 626 | 12 | | | could not open directory "/home/postgres/testversion/data/conf.xx"
(12 rows)

Bad value for a known GUC variable:

sourcefile | sourceline | seqno | name | setting | error
-------------------------------------------------+------------+-------+----------------------------+--------------------+------------------------------
/home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections | 100 |
/home/postgres/testversion/data/postgresql.conf | 116 | 2 | shared_buffers | 128MB |
/home/postgres/testversion/data/postgresql.conf | 131 | 3 | dynamic_shared_memory_type | posix |
/home/postgres/testversion/data/postgresql.conf | 446 | 4 | log_timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 533 | 5 | datestyle | iso, mdy |
/home/postgres/testversion/data/postgresql.conf | 535 | 6 | timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 548 | 7 | lc_messages | C |
/home/postgres/testversion/data/postgresql.conf | 550 | 8 | lc_monetary | C |
/home/postgres/testversion/data/postgresql.conf | 551 | 9 | lc_numeric | C |
/home/postgres/testversion/data/postgresql.conf | 552 | 10 | lc_time | C |
/home/postgres/testversion/data/postgresql.conf | 555 | 11 | default_text_search_config | pg_catalog.english |
/home/postgres/testversion/data/conf.d/foo.conf | 1 | 12 | work_mem | bogus | setting could not be applied
(12 rows)

Invalid GUC variable name (which causes SIGHUP processing to be abandoned,
so we don't get as far as noticing work_mem = bogus):

sourcefile | sourceline | seqno | name | setting | error
-------------------------------------------------+------------+-------+----------------------------+--------------------+--------------------------------------
/home/postgres/testversion/data/postgresql.conf | 64 | 1 | max_connections | 100 |
/home/postgres/testversion/data/postgresql.conf | 116 | 2 | shared_buffers | 128MB |
/home/postgres/testversion/data/postgresql.conf | 131 | 3 | dynamic_shared_memory_type | posix |
/home/postgres/testversion/data/postgresql.conf | 446 | 4 | log_timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 533 | 5 | datestyle | iso, mdy |
/home/postgres/testversion/data/postgresql.conf | 535 | 6 | timezone | US/Eastern |
/home/postgres/testversion/data/postgresql.conf | 548 | 7 | lc_messages | C |
/home/postgres/testversion/data/postgresql.conf | 550 | 8 | lc_monetary | C |
/home/postgres/testversion/data/postgresql.conf | 551 | 9 | lc_numeric | C |
/home/postgres/testversion/data/postgresql.conf | 552 | 10 | lc_time | C |
/home/postgres/testversion/data/postgresql.conf | 555 | 11 | default_text_search_config | pg_catalog.english |
/home/postgres/testversion/data/conf.d/foo.conf | 1 | 12 | work_mem | bogus |
/home/postgres/testversion/data/postgresql.conf | 628 | 13 | bad | worse | unrecognized configuration parameter
(13 rows)

ISTM that this represents a quantum jump in the usefulness of the
pg_file_settings view for its ostensible purpose of diagnosing config-file
problems, so I would like to push forward with getting it done even though
we're on the verge of 9.5alpha1.

However there's a further tweak to the view that I'd like to think about
making. Once this is in and we make the originally-discussed change to
suppress application of duplicated GUC entries, it would be possible to
mark the ignored entries in the view, which would save people the effort
of figuring out manually which ones were ignored. The question is exactly
how to mark the ignored entries. A simple tweak would be to put something
in the "error" column, say "ignored because of later duplicate entry".
However, this would break the property that an entry in the error column
means there's something you'd better fix, which I think would be a useful
rule for nagios-like monitoring tools.

Another issue with the API as it stands here is that you can't easily
distinguish errors that caused the entire config file to be ignored from
those that only prevented application of one setting.

The best idea I have at the moment is to also add a boolean "applied"
column which is true if the entry was successfully applied. Errors that
result in the whole file being ignored would mean that *all* the entries
show applied = false. Otherwise, applied = false with nothing in the
error column would imply that the entry was ignored due to a later
duplicate. There are multiple other ways it could be done of course;
anyone want to lobby for some other design?

There is more that could be done with this basic idea; in particular,
it would be useful if set_config failures would be broken down in more
detail than "setting could not be applied". That would require somewhat
invasive refactoring though, and it would only be an incremental
improvement in usability, so I'm disinclined to tackle the point under
time pressure.

Comments, better ideas? Barring strong objections I'd like to get this
finished over the weekend.

regards, tom lane