Re: [HACKERS] Semantics of pg_file_settings view
Sawada Masahiko writes: > On Mon, Jun 29, 2015 at 12:01 AM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
On Mon, Jun 29, 2015 at 12:01 AM, Tom Lane wrote:
> Sawada Masahiko writes:
>> On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
Sawada Masahiko writes:
> On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
On Sun, Jun 28, 2015 at 12:47 AM, Tom Lane wrote: > Robert Haas writes: >> On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane 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/t
Re: [HACKERS] Semantics of pg_file_settings view
On Sat, Jun 27, 2015 at 7:19 PM, Tom Lane wrote: > > Amit Kapila writes: > > On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
Robert Haas writes: > On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane 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/
Re: [HACKERS] Semantics of pg_file_settings view
Amit Kapila writes: > On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
On Fri, Jun 26, 2015 at 9:47 PM, Tom Lane 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 > Branch: master [e3da0d4d1] 2014-08-06 14:49:43 +0900 > Branch: REL9_4_STABLE Release: REL9_4_0 [cf6a9c374] 2014-08-06 14:50:30 +0900 > > Change ParseConfigFp() so that it doesn't process unused entry of each parameter. > > ... however, it seems I removed that again in a cleanup commit awhile > later :-(. I think I'd meant to move it somewhere else, or maybe even > fix it to not be O(N^2), but clearly forgot while dealing with assorted > unrelated fallout from the ALTER SYSTEM patch. > > However putting it back now would be problematic, because of the recent > introduction of the pg_file_settings view, which purports to display > all entries in the config files, even ones which got overridden by later > duplicate entries. If we just put back Fujii-san's code then only the > last duplicate entry will be visible in pg_file_settings, which seems to > destroy the rationale for having that view at all. > > What we evidently need to do is fix things so that the pg_file_settings > data gets captured before we suppress duplicates. > > In view of that, I am wondering whether the current placement of that > data-capturing code was actually designed intentionally, or if it was > chosen by throwing a dart at the source code. The latter seems more > likely, because we don't capture the data until after we've decided > that all the entries seem provisionally valid, ie the stanza headed > > * Check if all the supplied option names are valid, as an additional > * quasi-syntactic check on the validity of the config file. It is > > in guc-file.l. ISTM that there is a good argument for capturing the data > before that, so that it's updated by any SIGHUP, whether or not we > conclude that the config file(s) are valid enough to apply data from. > This would mean that the view might contain data about "settings" that > aren't valid GUC variables. But I fail to see why that's a bad thing, > if the main use-case is to debug problems with the config files. > > The simplest change would be to move the whole thing to around line 208 in > guc-file.l, just after the stanza that loads PG_AUTOCONF_FILENAME. Or you > could argue that the approach is broken altogether, and that we should > capture the data while we read the files, so that you have some useful > data in the view even if ParseConfigFile later decides there's a syntax > error. I'm actually thinking maybe we should flush that data-capturing > logic altogether in favor of just not deleting the ConfigVariable list > data structure, and generating the view directly from that data structure. Idea for generating view form ConfigVariable list sounds good, but how will it preserve the duplicate entries in the list assuming either we need to revert the original fix (e3da0d4d1) or doing the same in loop where we set GUC_IS_IN_FILE? > (You could even imagine being able to finger syntax errors in the view > that way, by having ParseConfigFile attach a dummy ConfigVariable entry > when it bails out.) > > The reason I started looking at this is that the loop where we set > GUC_IS_IN_FILE seems like the most natural place to deal with removing > duplicates, since it can notice more or less for free whether there are > any. Keeping removal of duplicate items in ParseConfigFp() has the advantage that it will work for all other places from where ParseConfigFp() is called, though I am not sure if today that is required. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Semantics of pg_file_settings view
On Fri, Jun 26, 2015 at 4:02 PM, Tom Lane 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
Re: [HACKERS] Semantics of pg_file_settings view
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
[HACKERS] 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 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
