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@postgresql.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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
I wrote:
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.)
While snouting around in the same area, I noticed that
ParseConfigDirectory() leaks memory: it neglects to pfree the file names
it's collected. I'm not sure that it's worth fixing in the back branches,
because you'd need to have SIGHUP'd the postmaster a few million times
before the leak would amount to anything worth noticing. However, this
does demonstrate that all the functionality we've loaded into the GUC code
of late is likely to have some memory leaks in it.
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.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane <tgl@sss.pgh.pa.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@postgresql.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 isin 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
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.
regards, tom lane
Attachments:
pg-file-settings-redesign-1.patchtext/x-diff; charset=us-ascii; name=pg-file-settings-redesign-1.patchDownload+250-193
On Sat, Jun 27, 2015 at 7:19 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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.
Sounds good way to deal with this problem.
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.
I have implemented that patch, so if you see any problem's with that
approach, Fujji-san is not right person to blame.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Sawada Masahiko <sawada.mshk@gmail.com> writes:
On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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?
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.
Yeah, I realize that it's *possible* to get this information out of the
view as it stands. But it's not especially *convenient*. And since this
seems like the main if not only use-case for the view (at least prior to
the addition of error information), I don't see why we insist on making it
difficult for users to identify ignored entries.
(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.
er ... what?
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 29, 2015 at 12:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Sawada Masahiko <sawada.mshk@gmail.com> writes:
On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
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?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.Yeah, I realize that it's *possible* to get this information out of the
view as it stands. But it's not especially *convenient*. And since this
seems like the main if not only use-case for the view (at least prior to
the addition of error information), I don't see why we insist on making it
difficult for users to identify ignored entries.
Yep, I think that it will have enough information by adding additional
information of WIP patch.
(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.er ... what?
Sorry for confusing you.
Anyway I meant that I got SEGV after applied WIP patch, and the cause
is the above changes.
The case is following.
1. Add "port = 6543" to postgresql.conf and restart server.
2. Remove that added line from postgresql.conf
3. Reload.
Got SEGV.
Regards,
--
Sawada Masahiko
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Sawada Masahiko <sawada.mshk@gmail.com> writes:
On Mon, Jun 29, 2015 at 12:01 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
er ... what?
Sorry for confusing you.
Anyway I meant that I got SEGV after applied WIP patch, and the cause
is the above changes.
The case is following.
1. Add "port = 6543" to postgresql.conf and restart server.
2. Remove that added line from postgresql.conf
3. Reload.
Got SEGV.
Oh, okay. I'll check, thanks for noticing it!
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers