Re: Semantics of pg_file_settings view - Mailing list pgsql-hackers

On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane <tgl@sss.pgh.pa.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.
>

I think that we can find applied value by arranging
pg_file_settings.seqno ascending order.
The value which has highest seqno is the currently applied value, and
it's default value if pg_file_settings does not have that entry.
Because of above reason, I think it's enough to mark which entry was
not applied due to contain error in its config file rather than
marking which entry was applied.
For example, the 'ignored' column of the ignored parameter due to
contain the error in config file is true, OTOH, the ignored parameter
due to duplicate later is false.
Though?


@@ -289,6 +321,7 @@ ProcessConfigFile(GucContext context)

(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),                                        errmsg("parameter \"%s\"
cannot be changed without restarting the server",
+                       item->errmsg = pstrdup("parameter cannot be
changed without restarting the server");                       error = true;                       continue;

Also, the above hunk is wrong, because the item variable is wobbly and
the correspond line is not exists here.
If we restart after removing a line to use default value which context
is SIGHUP(e,g, port), it leads to occur SEGV.

Regards,

--
Sawada Masahiko