Thread: 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
theconfig 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
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
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
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 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.
>
> 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.
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.
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
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
diff --git a/src/backend/utils/misc/guc-file.l b/src/backend/utils/misc/guc-file.l
index 5c0a965..887077f 100644
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
*************** static sigjmp_buf *GUC_flex_fatal_jmp;
*** 47,52 ****
--- 47,58 ----
static void FreeConfigVariable(ConfigVariable *item);
+ static void record_config_file_error(const char *errmsg,
+ const char *config_file,
+ int lineno,
+ ConfigVariable **head_p,
+ ConfigVariable **tail_p);
+
static int GUC_flex_fatal(const char *msg);
static char *GUC_scanstr(const char *s);
*************** ProcessConfigFile(GucContext context)
*** 115,126 ****
bool error = false;
bool apply = false;
int elevel;
const char *ConfFileWithError;
ConfigVariable *item,
*head,
*tail;
int i;
- int file_variables_count = 0;
/*
* Config files are processed on startup (by the postmaster only)
--- 121,132 ----
bool error = false;
bool apply = false;
int elevel;
+ MemoryContext caller_cxt;
const char *ConfFileWithError;
ConfigVariable *item,
*head,
*tail;
int i;
/*
* Config files are processed on startup (by the postmaster only)
*************** ProcessConfigFile(GucContext context)
*** 135,149 ****
*/
elevel = IsUnderPostmaster ? DEBUG2 : LOG;
/* Parse the main config file into a list of option names and values */
ConfFileWithError = ConfigFileName;
head = tail = NULL;
if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head, &tail))
{
/* Syntax error(s) detected in the file, so bail out */
error = true;
! goto cleanup_list;
}
/*
--- 141,179 ----
*/
elevel = IsUnderPostmaster ? DEBUG2 : LOG;
+ /*
+ * Do all the work of processing the config file within a private context.
+ * We will keep this context around until the next ProcessConfigFile call,
+ * so that the pg_file_settings view can inspect the ConfigVariable list.
+ * However, we can guarantee that any memory leaked in this code will not
+ * accumulate across repeated SIGHUP cycles.
+ *
+ * First, flush the old data if any.
+ */
+ current_config_variables = NULL;
+ if (current_config_context)
+ {
+ MemoryContextDelete(current_config_context);
+ current_config_context = NULL;
+ }
+
+ current_config_context = AllocSetContextCreate(TopMemoryContext,
+ "Config file processing",
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+ caller_cxt = MemoryContextSwitchTo(current_config_context);
+
/* Parse the main config file into a list of option names and values */
ConfFileWithError = ConfigFileName;
head = tail = NULL;
+ ConfigFileLineno = 0;
if (!ParseConfigFile(ConfigFileName, NULL, true, 0, elevel, &head, &tail))
{
/* Syntax error(s) detected in the file, so bail out */
error = true;
! goto bail_out;
}
/*
*************** ProcessConfigFile(GucContext context)
*** 154,166 ****
*/
if (DataDir)
{
if (!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
&head, &tail))
{
/* Syntax error(s) detected in the file, so bail out */
error = true;
ConfFileWithError = PG_AUTOCONF_FILENAME;
! goto cleanup_list;
}
}
else
--- 184,197 ----
*/
if (DataDir)
{
+ ConfigFileLineno = 0;
if (!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
&head, &tail))
{
/* Syntax error(s) detected in the file, so bail out */
error = true;
ConfFileWithError = PG_AUTOCONF_FILENAME;
! goto bail_out;
}
}
else
*************** ProcessConfigFile(GucContext context)
*** 173,200 ****
* will be read later. OTOH, since data_directory isn't allowed in the
* PG_AUTOCONF_FILENAME file, it will never be overwritten later.
*/
! ConfigVariable *prev = NULL;
! /* Prune all items except "data_directory" from the list */
! for (item = head; item;)
{
! ConfigVariable *ptr = item;
!
! item = item->next;
! if (strcmp(ptr->name, "data_directory") != 0)
! {
! if (prev == NULL)
! head = ptr->next;
! else
! prev->next = ptr->next;
! if (ptr->next == NULL)
! tail = prev;
! FreeConfigVariable(ptr);
! }
! else
! prev = ptr;
}
/*
* Quick exit if data_directory is not present in file.
*
--- 204,226 ----
* will be read later. OTOH, since data_directory isn't allowed in the
* PG_AUTOCONF_FILENAME file, it will never be overwritten later.
*/
! ConfigVariable *newlist = NULL;
! /*
! * Prune all items except the last "data_directory" from the list. We
! * needn't bother freeing space, since the working context will be
! * cleaned up in the next call.
! */
! for (item = head; item; item = item->next)
{
! if (strcmp(item->name, "data_directory") == 0)
! newlist = item;
}
+ if (newlist)
+ newlist->next = NULL;
+ head = tail = newlist;
+
/*
* Quick exit if data_directory is not present in file.
*
*************** ProcessConfigFile(GucContext context)
*** 203,212 ****
* the config file.
*/
if (head == NULL)
! return;
}
/*
* Mark all extant GUC variables as not present in the config file.
* We need this so that we can tell below which ones have been removed
* from the file since we last processed it.
--- 229,244 ----
* the config file.
*/
if (head == NULL)
! goto bail_out;
}
/*
+ * Remember the raw data for possible examination via the pg_file_settings
+ * view. We want to do this even if an elog(ERROR) gets thrown below.
+ */
+ current_config_variables = head;
+
+ /*
* Mark all extant GUC variables as not present in the config file.
* We need this so that we can tell below which ones have been removed
* from the file since we last processed it.
*************** ProcessConfigFile(GucContext context)
*** 253,262 ****
errmsg("unrecognized configuration parameter \"%s\" in file \"%s\" line %u",
item->name,
item->filename, item->sourceline)));
error = true;
ConfFileWithError = item->filename;
}
- file_variables_count++;
}
/*
--- 285,294 ----
errmsg("unrecognized configuration parameter \"%s\" in file \"%s\" line %u",
item->name,
item->filename, item->sourceline)));
+ item->errmsg = pstrdup("unrecognized configuration parameter");
error = true;
ConfFileWithError = item->filename;
}
}
/*
*************** ProcessConfigFile(GucContext context)
*** 264,270 ****
* any changes.
*/
if (error)
! goto cleanup_list;
/* Otherwise, set flag that we're beginning to apply changes */
apply = true;
--- 296,302 ----
* any changes.
*/
if (error)
! goto bail_out;
/* Otherwise, set flag that we're beginning to apply changes */
apply = true;
*************** ProcessConfigFile(GucContext context)
*** 289,294 ****
--- 321,327 ----
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
errmsg("parameter \"%s\" cannot be changed without restarting the server",
gconf->name)));
+ item->errmsg = pstrdup("parameter cannot be changed without restarting the server");
error = true;
continue;
}
*************** ProcessConfigFile(GucContext context)
*** 344,397 ****
}
/*
- * Check if we have allocated the array yet.
- *
- * If not, allocate it based on the number of file variables we have seen.
- */
- if (!guc_file_variables)
- {
- /* For the first call */
- num_guc_file_variables = file_variables_count;
- guc_file_variables = (ConfigFileVariable *) guc_malloc(FATAL,
- num_guc_file_variables * sizeof(struct ConfigFileVariable));
- }
- else
- {
- int i;
-
- /* Free all of the previously allocated entries */
- for (i = 0; i < num_guc_file_variables; i++)
- {
- free(guc_file_variables[i].name);
- free(guc_file_variables[i].value);
- free(guc_file_variables[i].filename);
- }
-
- /* Update the global count and realloc based on the new size */
- num_guc_file_variables = file_variables_count;
- guc_file_variables = (ConfigFileVariable *) guc_realloc(FATAL,
- guc_file_variables,
- num_guc_file_variables * sizeof(struct ConfigFileVariable));
- }
-
- /*
- * Copy the settings which came from the files read into the
- * guc_file_variables array which backs the pg_show_file_settings()
- * function.
- */
- for (item = head, i = 0; item && i < num_guc_file_variables;
- item = item->next, i++)
- {
- guc_file_variables[i].name = guc_strdup(FATAL, item->name);
- guc_file_variables[i].value = guc_strdup(FATAL, item->value);
- guc_file_variables[i].filename = guc_strdup(FATAL, item->filename);
- guc_file_variables[i].sourceline = item->sourceline;
- }
-
- /* We had better have made it through the loop above to a clean ending. */
- Assert(!item && i == num_guc_file_variables);
-
- /*
* Now apply the values from the config file.
*/
for (item = head; item; item = item->next)
--- 377,382 ----
*************** ProcessConfigFile(GucContext context)
*** 432,437 ****
--- 417,423 ----
else if (scres == 0)
{
error = true;
+ item->errmsg = pstrdup("setting could not be applied");
ConfFileWithError = item->filename;
}
/* else no error but variable's active value was not changed */
*************** ProcessConfigFile(GucContext context)
*** 453,459 ****
/* Remember when we last successfully loaded the config file. */
PgReloadTime = GetCurrentTimestamp();
! cleanup_list:
if (error)
{
/* During postmaster startup, any error is fatal */
--- 439,449 ----
/* Remember when we last successfully loaded the config file. */
PgReloadTime = GetCurrentTimestamp();
! bail_out:
! /* Successful or otherwise, remember the data collected from the files */
! current_config_variables = head;
!
! /* Report any problem */
if (error)
{
/* During postmaster startup, any error is fatal */
*************** ProcessConfigFile(GucContext context)
*** 475,485 ****
}
/*
! * Calling FreeConfigVariables() any earlier than this can cause problems,
! * because ConfFileWithError could be pointing to a string that will be
! * freed here.
*/
! FreeConfigVariables(head);
}
/*
--- 465,475 ----
}
/*
! * Return to caller's memory context. Don't delete current_config_context
! * now; we will do that when next called. This allows the ConfigVariables
! * list to be inspected after-the-fact.
*/
! MemoryContextSwitchTo(caller_cxt);
}
/*
*************** AbsoluteConfigLocation(const char *locat
*** 520,525 ****
--- 510,519 ----
* If "strict" is true, treat failure to open the config file as an error,
* otherwise just skip the file.
*
+ * Note that ConfigFileLineno should be considered to be an implicit parameter
+ * to this function: it is the line number within calling_file, if any, that
+ * should be fingered for error reporting purposes.
+ *
* See ParseConfigFp for further details. This one merely adds opening the
* config file rather than working from a caller-supplied file descriptor,
* and absolute-ifying the path name if necessary.
*************** ParseConfigFile(const char *config_file,
*** 545,550 ****
--- 539,547 ----
(errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("could not open configuration file \"%s\": maximum nesting depth exceeded",
config_file)));
+ record_config_file_error("nesting depth exceeded",
+ calling_file, ConfigFileLineno,
+ head_p, tail_p);
return false;
}
*************** ParseConfigFile(const char *config_file,
*** 558,563 ****
--- 555,564 ----
(errcode_for_file_access(),
errmsg("could not open configuration file \"%s\": %m",
abs_path)));
+ record_config_file_error(psprintf("could not open file \"%s\"",
+ abs_path),
+ calling_file, ConfigFileLineno,
+ head_p, tail_p);
OK = false;
}
else
*************** cleanup:
*** 580,585 ****
--- 581,613 ----
}
/*
+ * Capture an error message in the ConfigVariable list returned by
+ * config file parsing.
+ */
+ static void
+ record_config_file_error(const char *errmsg,
+ const char *config_file,
+ int lineno,
+ ConfigVariable **head_p,
+ ConfigVariable **tail_p)
+ {
+ ConfigVariable *item;
+
+ item = palloc(sizeof *item);
+ item->name = NULL;
+ item->value = NULL;
+ item->errmsg = pstrdup(errmsg);
+ item->filename = config_file ? pstrdup(config_file) : NULL;
+ item->sourceline = lineno;
+ item->next = NULL;
+ if (*head_p == NULL)
+ *head_p = item;
+ else
+ (*tail_p)->next = item;
+ *tail_p = item;
+ }
+
+ /*
* Flex fatal errors bring us here. Stash the error message and jump back to
* ParseConfigFp(). Assume all msg arguments point to string constants; this
* holds for flex 2.5.31 (earliest we support) and flex 2.5.35 (latest as of
*************** ParseConfigFp(FILE *fp, const char *conf
*** 641,647 ****
*/
elog(elevel, "%s at file \"%s\" line %u",
GUC_flex_fatal_errmsg, config_file, ConfigFileLineno);
!
OK = false;
goto cleanup;
}
--- 669,677 ----
*/
elog(elevel, "%s at file \"%s\" line %u",
GUC_flex_fatal_errmsg, config_file, ConfigFileLineno);
! record_config_file_error(GUC_flex_fatal_errmsg,
! config_file, ConfigFileLineno,
! head_p, tail_p);
OK = false;
goto cleanup;
}
*************** ParseConfigFp(FILE *fp, const char *conf
*** 697,702 ****
--- 727,735 ----
ConfigFileLineno++;
}
+ /* Temporarily decr ConfigFileLineno so it identifies correct line */
+ ConfigFileLineno--;
+
/* OK, process the option name and value */
if (guc_name_compare(opt_name, "include_dir") == 0)
{
*************** ParseConfigFp(FILE *fp, const char *conf
*** 709,715 ****
head_p, tail_p))
OK = false;
yy_switch_to_buffer(lex_buffer);
- ConfigFileLineno = save_ConfigFileLineno;
pfree(opt_name);
pfree(opt_value);
}
--- 742,747 ----
*************** ParseConfigFp(FILE *fp, const char *conf
*** 747,754 ****
item = palloc(sizeof *item);
item->name = opt_name;
item->value = opt_value;
item->filename = pstrdup(config_file);
! item->sourceline = ConfigFileLineno-1;
item->next = NULL;
if (*head_p == NULL)
*head_p = item;
--- 779,787 ----
item = palloc(sizeof *item);
item->name = opt_name;
item->value = opt_value;
+ item->errmsg = NULL;
item->filename = pstrdup(config_file);
! item->sourceline = ConfigFileLineno;
item->next = NULL;
if (*head_p == NULL)
*head_p = item;
*************** ParseConfigFp(FILE *fp, const char *conf
*** 757,762 ****
--- 790,798 ----
*tail_p = item;
}
+ /* Done processing line, restore ConfigFileLineno */
+ ConfigFileLineno++;
+
/* break out of loop if read EOF, else loop for next line */
if (token == 0)
break;
*************** ParseConfigFp(FILE *fp, const char *conf
*** 771,785 ****
--- 807,831 ----
/* report the error */
if (token == GUC_EOL || token == 0)
+ {
ereport(elevel,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("syntax error in file \"%s\" line %u, near end of line",
config_file, ConfigFileLineno - 1)));
+ record_config_file_error("syntax error",
+ config_file, ConfigFileLineno - 1,
+ head_p, tail_p);
+ }
else
+ {
ereport(elevel,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("syntax error in file \"%s\" line %u, near token \"%s\"",
config_file, ConfigFileLineno, yytext)));
+ record_config_file_error("syntax error",
+ config_file, ConfigFileLineno,
+ head_p, tail_p);
+ }
OK = false;
errorcount++;
*************** cleanup:
*** 817,822 ****
--- 863,872 ----
/*
* Read and parse all config files in a subdirectory in alphabetical order
+ *
+ * Note that ConfigFileLineno should be considered to be an implicit parameter
+ * to this function: it is the line number within calling_file, if any, that
+ * should be fingered for error reporting purposes.
*/
bool
ParseConfigDirectory(const char *includedir,
*************** ParseConfigDirectory(const char *include
*** 841,846 ****
--- 891,900 ----
(errcode_for_file_access(),
errmsg("could not open configuration directory \"%s\": %m",
directory)));
+ record_config_file_error(psprintf("could not open directory \"%s\"",
+ directory),
+ calling_file, ConfigFileLineno,
+ head_p, tail_p);
status = false;
goto cleanup;
}
*************** ParseConfigDirectory(const char *include
*** 897,902 ****
--- 951,960 ----
(errcode_for_file_access(),
errmsg("could not stat file \"%s\": %m",
filename)));
+ record_config_file_error(psprintf("could not stat file \"%s\"",
+ filename),
+ calling_file, ConfigFileLineno,
+ head_p, tail_p);
status = false;
goto cleanup;
}
*************** ParseConfigDirectory(const char *include
*** 908,914 ****
qsort(filenames, num_filenames, sizeof(char *), pg_qsort_strcmp);
for (i = 0; i < num_filenames; i++)
{
! if (!ParseConfigFile(filenames[i], NULL, true,
depth, elevel, head_p, tail_p))
{
status = false;
--- 966,972 ----
qsort(filenames, num_filenames, sizeof(char *), pg_qsort_strcmp);
for (i = 0; i < num_filenames; i++)
{
! if (!ParseConfigFile(filenames[i], calling_file, true,
depth, elevel, head_p, tail_p))
{
status = false;
*************** FreeConfigVariables(ConfigVariable *list
*** 949,957 ****
static void
FreeConfigVariable(ConfigVariable *item)
{
! pfree(item->name);
! pfree(item->value);
! pfree(item->filename);
pfree(item);
}
--- 1007,1020 ----
static void
FreeConfigVariable(ConfigVariable *item)
{
! if (item->name)
! pfree(item->name);
! if (item->value)
! pfree(item->value);
! if (item->errmsg)
! pfree(item->errmsg);
! if (item->filename)
! pfree(item->filename);
pfree(item);
}
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 230c5cc..cab8da0 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** static struct config_generic **guc_varia
*** 3678,3702 ****
/* Current number of variables contained in the vector */
static int num_guc_variables;
- /*
- * Lookup of variables for pg_file_settings view.
- * guc_file_variables is an array of length num_guc_file_variables.
- */
- typedef struct ConfigFileVariable
- {
- char *name;
- char *value;
- char *filename;
- int sourceline;
- } ConfigFileVariable;
- static struct ConfigFileVariable *guc_file_variables;
-
- /* Number of file variables */
- static int num_guc_file_variables;
-
/* Vector capacity */
static int size_guc_variables;
static bool guc_dirty; /* TRUE if need to do commit/abort work */
--- 3678,3695 ----
/* Current number of variables contained in the vector */
static int num_guc_variables;
/* Vector capacity */
static int size_guc_variables;
+ /*
+ * The raw data most recently fetched from the config file(s) is stored
+ * in the current_config_variables list, which is stored within the
+ * current_config_context memory context. We keep it around until the next
+ * attempt to read the config files, so that it can be displayed in the
+ * pg_file_settings view.
+ */
+ static ConfigVariable *current_config_variables = NULL;
+ static MemoryContext current_config_context = NULL;
static bool guc_dirty; /* TRUE if need to do commit/abort work */
*************** replace_auto_config_value(ConfigVariable
*** 6781,6786 ****
--- 6774,6780 ----
item = palloc(sizeof *item);
item->name = pstrdup(name);
item->value = pstrdup(value);
+ item->errmsg = NULL;
item->filename = pstrdup(""); /* new item has no location */
item->sourceline = 0;
item->next = NULL;
*************** show_all_settings(PG_FUNCTION_ARGS)
*** 8181,8190 ****
/*
* show_all_file_settings
*
! * returns a table of all parameter settings in all configuration files
! * which includes the config file path/name, the line number, a sequence number
! * indicating when we loaded it, the parameter name, and the value it is
! * set to.
*
* Note: no filtering is done here, instead we depend on the GRANT system
* to prevent unprivileged users from accessing this function or the view
--- 8175,8185 ----
/*
* show_all_file_settings
*
! * Returns a table of all parameter settings in all configuration files
! * which includes the config file pathname, the line number, a sequence number
! * indicating the order in which the settings were encountered, the parameter
! * name and value, and possibly an associated error message. (For problems
! * such as syntax errors, the parameter name/value might be NULL.)
*
* Note: no filtering is done here, instead we depend on the GRANT system
* to prevent unprivileged users from accessing this function or the view
*************** show_all_settings(PG_FUNCTION_ARGS)
*** 8193,8203 ****
Datum
show_all_file_settings(PG_FUNCTION_ARGS)
{
! #define NUM_PG_FILE_SETTINGS_ATTS 5
FuncCallContext *funcctx;
TupleDesc tupdesc;
int call_cntr;
- int max_calls;
AttInMetadata *attinmeta;
MemoryContext oldcontext;
--- 8188,8198 ----
Datum
show_all_file_settings(PG_FUNCTION_ARGS)
{
! #define NUM_PG_FILE_SETTINGS_ATTS 6
FuncCallContext *funcctx;
TupleDesc tupdesc;
+ ConfigVariable *conf;
int call_cntr;
AttInMetadata *attinmeta;
MemoryContext oldcontext;
*************** show_all_file_settings(PG_FUNCTION_ARGS)
*** 8223,8260 ****
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "setting",
TEXTOID, -1, 0);
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
! funcctx->max_calls = num_guc_file_variables;
MemoryContextSwitchTo(oldcontext);
}
funcctx = SRF_PERCALL_SETUP();
call_cntr = funcctx->call_cntr;
- max_calls = funcctx->max_calls;
attinmeta = funcctx->attinmeta;
! if (call_cntr < max_calls)
{
char *values[NUM_PG_FILE_SETTINGS_ATTS];
HeapTuple tuple;
Datum result;
- ConfigFileVariable conf;
char buffer[12]; /* must be at least 12, per pg_ltoa */
! /* Check to avoid going past end of array */
! if (call_cntr > num_guc_file_variables)
! SRF_RETURN_DONE(funcctx);
!
! conf = guc_file_variables[call_cntr];
/* sourcefile */
! values[0] = conf.filename;
/* sourceline */
! pg_ltoa(conf.sourceline, buffer);
values[1] = pstrdup(buffer);
/* seqno */
--- 8218,8256 ----
TEXTOID, -1, 0);
TupleDescInitEntry(tupdesc, (AttrNumber) 5, "setting",
TEXTOID, -1, 0);
+ TupleDescInitEntry(tupdesc, (AttrNumber) 6, "error",
+ TEXTOID, -1, 0);
attinmeta = TupleDescGetAttInMetadata(tupdesc);
funcctx->attinmeta = attinmeta;
!
! /* We use user_fctx to point to next item to be returned */
! funcctx->user_fctx = (void *) current_config_variables;
!
MemoryContextSwitchTo(oldcontext);
}
funcctx = SRF_PERCALL_SETUP();
+ conf = (ConfigVariable *) funcctx->user_fctx;
call_cntr = funcctx->call_cntr;
attinmeta = funcctx->attinmeta;
! if (conf != NULL)
{
char *values[NUM_PG_FILE_SETTINGS_ATTS];
HeapTuple tuple;
Datum result;
char buffer[12]; /* must be at least 12, per pg_ltoa */
! /* Update pointer to next item to return */
! funcctx->user_fctx = (void *) conf->next;
/* sourcefile */
! values[0] = conf->filename;
/* sourceline */
! pg_ltoa(conf->sourceline, buffer);
values[1] = pstrdup(buffer);
/* seqno */
*************** show_all_file_settings(PG_FUNCTION_ARGS)
*** 8262,8271 ****
values[2] = pstrdup(buffer);
/* name */
! values[3] = conf.name;
/* setting */
! values[4] = conf.value;
/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
--- 8258,8270 ----
values[2] = pstrdup(buffer);
/* name */
! values[3] = conf->name;
/* setting */
! values[4] = conf->value;
!
! /* error */
! values[5] = conf->errmsg;
/* build a tuple */
tuple = BuildTupleFromCStrings(attinmeta, values);
*************** show_all_file_settings(PG_FUNCTION_ARGS)
*** 8275,8284 ****
SRF_RETURN_NEXT(funcctx, result);
}
! else
! {
! SRF_RETURN_DONE(funcctx);
! }
}
static char *
--- 8274,8281 ----
SRF_RETURN_NEXT(funcctx, result);
}
!
! SRF_RETURN_DONE(funcctx);
}
static char *
diff --git a/src/include/catalog/pg_proc.h b/src/include/catalog/pg_proc.h
index 6b3d194..c03e2d3 100644
*** a/src/include/catalog/pg_proc.h
--- b/src/include/catalog/pg_proc.h
*************** DATA(insert OID = 2078 ( set_config PG
*** 3071,3077 ****
DESCR("SET X as a function");
DATA(insert OID = 2084 ( pg_show_all_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 ""
"{25,25,25,25,25,25,25,25,25,25,25,1009,25,25,25,23,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline,pending_restart}"
_null__null_ show_all_settings _null_ _null_ _null_ ));
DESCR("SHOW ALL as a function");
! DATA(insert OID = 3329 ( pg_show_all_file_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 ""
"{25,23,23,25,25}""{o,o,o,o,o}" "{sourcefile,sourceline,seqno,name,setting}" _null_ _null_ show_all_file_settings
_null__null_ _null_ ));
DESCR("show config file settings");
DATA(insert OID = 1371 ( pg_lock_status PGNSP PGUID 12 1 1000 0 0 f f f f t t v 0 0 2249 ""
"{25,26,26,23,21,25,28,26,26,21,25,23,25,16,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted,fastpath}"
_null__null_ pg_lock_status _null_ _null_ _null_ ));
DESCR("view system lock information");
--- 3071,3077 ----
DESCR("SET X as a function");
DATA(insert OID = 2084 ( pg_show_all_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 ""
"{25,25,25,25,25,25,25,25,25,25,25,1009,25,25,25,23,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{name,setting,unit,category,short_desc,extra_desc,context,vartype,source,min_val,max_val,enumvals,boot_val,reset_val,sourcefile,sourceline,pending_restart}"
_null__null_ show_all_settings _null_ _null_ _null_ ));
DESCR("SHOW ALL as a function");
! DATA(insert OID = 3329 ( pg_show_all_file_settings PGNSP PGUID 12 1 1000 0 0 f f f f t t s 0 0 2249 ""
"{25,23,23,25,25,25}""{o,o,o,o,o,o}" "{sourcefile,sourceline,seqno,name,setting,error}" _null_ _null_
show_all_file_settings_null_ _null_ _null_ ));
DESCR("show config file settings");
DATA(insert OID = 1371 ( pg_lock_status PGNSP PGUID 12 1 1000 0 0 f f f f t t v 0 0 2249 ""
"{25,26,26,23,21,25,28,26,26,21,25,23,25,16,16}""{o,o,o,o,o,o,o,o,o,o,o,o,o,o,o}"
"{locktype,database,relation,page,tuple,virtualxid,transactionid,classid,objid,objsubid,virtualtransaction,pid,mode,granted,fastpath}"
_null__null_ pg_lock_status _null_ _null_ _null_ ));
DESCR("view system lock information");
diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h
index ffe1168..d7c286d 100644
*** a/src/include/utils/guc.h
--- b/src/include/utils/guc.h
*************** typedef enum
*** 121,133 ****
} GucSource;
/*
! * Parsing the configuration file will return a list of name-value pairs
! * with source location info.
*/
typedef struct ConfigVariable
{
char *name;
char *value;
char *filename;
int sourceline;
struct ConfigVariable *next;
--- 121,136 ----
} GucSource;
/*
! * Parsing the configuration file(s) will return a list of name-value pairs
! * with source location info. We also abuse this data structure to carry
! * error reports about the config files. An entry reporting an error will
! * have errmsg != NULL, and might have NULLs for name, value, and/or filename.
*/
typedef struct ConfigVariable
{
char *name;
char *value;
+ char *errmsg;
char *filename;
int sourceline;
struct ConfigVariable *next;
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 60c1f40..bcf24be 100644
*** a/src/test/regress/expected/rules.out
--- b/src/test/regress/expected/rules.out
*************** pg_file_settings| SELECT a.sourcefile,
*** 1316,1323 ****
a.sourceline,
a.seqno,
a.name,
! a.setting
! FROM pg_show_all_file_settings() a(sourcefile, sourceline, seqno, name, setting);
pg_group| SELECT pg_authid.rolname AS groname,
pg_authid.oid AS grosysid,
ARRAY( SELECT pg_auth_members.member
--- 1316,1324 ----
a.sourceline,
a.seqno,
a.name,
! a.setting,
! a.error
! FROM pg_show_all_file_settings() a(sourcefile, sourceline, seqno, name, setting, error);
pg_group| SELECT pg_authid.rolname AS groname,
pg_authid.oid AS grosysid,
ARRAY( SELECT pg_auth_members.member
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.
>
> > 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.
>
> 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.
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
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
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
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