Re: [HACKERS] ALTER SYSTEM SET command to change postgresql.conf parameters
On Thu, Jan 9, 2014 at 11:06 PM, Amit Kapila wrote: > On Thu, Jan 9, 2014 at 12:26 AM, Robert Haas wrote: >> On Mon, Jan 6, 2014 at 11:37 PM, Amit Kapila wrote: >>> On Tue, Jan 7, 2014 at 12:52 AM, Robert Haas wrote: On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila wrote: >> Couldn't we also handle this by postponing FreeConfigVariables until >> after the if (error) block? > >Wouldn't doing that way can lead to bigger memory leak, if error level >is ERROR. Though in current fix also it can leak memory but it will be >just for ErrorConfFile_save. I think some similar case can happen for >'pre_value' in code currently as well, that's why I have fixed it in a >similar way in patch. I was assuming that error-recovery would reset the containing memory context, but I'm not sure what memory context we're executing in at this point. >>> >>> >>> In current code, the only time it can go to error path with elevel as >>> ERROR is during Postmaster startup >>> (context == PGC_POSTMASTER), at which it will anyway upgrade >>> ERROR to FATAL, so it should not be a problem to move >>> function FreeConfigVariables() after error block check. However >>> in future, if someone added any more ERROR (the chances of which >>> seems to be quite less), it can cause leak, may be thats why original >>> code has been written that way. >>> >>> If you think it's better to fix by moving FreeConfigVariables() after error >>> block check, then I can update the patch by doing so and incorporate other >>> change (directly use PG_AUTOCONF_FILENAME) suggested by you >>> as well? >> >> Yeah, let's do it that way. > > Okay, done. Attached patch fixes both the display of wrong file name and > usage of PG_AUTOCONF_FILENAME. Committed with a comment change. -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Thu, Jan 9, 2014 at 12:26 AM, Robert Haas wrote: > On Mon, Jan 6, 2014 at 11:37 PM, Amit Kapila wrote: >> On Tue, Jan 7, 2014 at 12:52 AM, Robert Haas wrote: >>> On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila wrote: > Couldn't we also handle this by postponing FreeConfigVariables until > after the if (error) block? Wouldn't doing that way can lead to bigger memory leak, if error level is ERROR. Though in current fix also it can leak memory but it will be just for ErrorConfFile_save. I think some similar case can happen for 'pre_value' in code currently as well, that's why I have fixed it in a similar way in patch. >>> >>> I was assuming that error-recovery would reset the containing memory >>> context, but I'm not sure what memory context we're executing in at >>> this point. >> >> >> In current code, the only time it can go to error path with elevel as >> ERROR is during Postmaster startup >> (context == PGC_POSTMASTER), at which it will anyway upgrade >> ERROR to FATAL, so it should not be a problem to move >> function FreeConfigVariables() after error block check. However >> in future, if someone added any more ERROR (the chances of which >> seems to be quite less), it can cause leak, may be thats why original >> code has been written that way. >> >> If you think it's better to fix by moving FreeConfigVariables() after error >> block check, then I can update the patch by doing so and incorporate other >> change (directly use PG_AUTOCONF_FILENAME) suggested by you >> as well? > > Yeah, let's do it that way. Okay, done. Attached patch fixes both the display of wrong file name and usage of PG_AUTOCONF_FILENAME. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com display_error_conf_filename_v2.patch Description: Binary data -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Mon, Jan 6, 2014 at 11:37 PM, Amit Kapila wrote: > On Tue, Jan 7, 2014 at 12:52 AM, Robert Haas wrote: >> On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila wrote: Couldn't we also handle this by postponing FreeConfigVariables until after the if (error) block? >>> >>>Wouldn't doing that way can lead to bigger memory leak, if error level >>>is ERROR. Though in current fix also it can leak memory but it will be >>>just for ErrorConfFile_save. I think some similar case can happen for >>>'pre_value' in code currently as well, that's why I have fixed it in a >>>similar way in patch. >> >> I was assuming that error-recovery would reset the containing memory >> context, but I'm not sure what memory context we're executing in at >> this point. > > This function is called from multiple places and based on when it would > get called the memory context varies. During Startup, it gets called in > Postmaster context and if some backend runs pg_reload_conf(), then > it will get called from other background processes (WAL Writer, > Checpointer, etc..) in their respective contexts (for WAL Writer, the > context will be WAL Writer, ..). > > In current code, the only time it can go to error path with elevel as > ERROR is during Postmaster startup > (context == PGC_POSTMASTER), at which it will anyway upgrade > ERROR to FATAL, so it should not be a problem to move > function FreeConfigVariables() after error block check. However > in future, if someone added any more ERROR (the chances of which > seems to be quite less), it can cause leak, may be thats why original > code has been written that way. > > If you think it's better to fix by moving FreeConfigVariables() after error > block check, then I can update the patch by doing so and incorporate other > change (directly use PG_AUTOCONF_FILENAME) suggested by you > as well? Yeah, let's do it that way. -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Tue, Jan 7, 2014 at 12:52 AM, Robert Haas wrote: > On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila wrote: >>> Couldn't we also handle this by postponing FreeConfigVariables until >>> after the if (error) block? >> >>Wouldn't doing that way can lead to bigger memory leak, if error level >>is ERROR. Though in current fix also it can leak memory but it will be >>just for ErrorConfFile_save. I think some similar case can happen for >>'pre_value' in code currently as well, that's why I have fixed it in a >>similar way in patch. > > I was assuming that error-recovery would reset the containing memory > context, but I'm not sure what memory context we're executing in at > this point. This function is called from multiple places and based on when it would get called the memory context varies. During Startup, it gets called in Postmaster context and if some backend runs pg_reload_conf(), then it will get called from other background processes (WAL Writer, Checpointer, etc..) in their respective contexts (for WAL Writer, the context will be WAL Writer, ..). In current code, the only time it can go to error path with elevel as ERROR is during Postmaster startup (context == PGC_POSTMASTER), at which it will anyway upgrade ERROR to FATAL, so it should not be a problem to move function FreeConfigVariables() after error block check. However in future, if someone added any more ERROR (the chances of which seems to be quite less), it can cause leak, may be thats why original code has been written that way. If you think it's better to fix by moving FreeConfigVariables() after error block check, then I can update the patch by doing so and incorporate other change (directly use PG_AUTOCONF_FILENAME) suggested by you as well? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Mon, Jan 6, 2014 at 9:48 AM, Amit Kapila wrote: >> Couldn't we also handle this by postponing FreeConfigVariables until >> after the if (error) block? > >Wouldn't doing that way can lead to bigger memory leak, if error level >is ERROR. Though in current fix also it can leak memory but it will be >just for ErrorConfFile_save. I think some similar case can happen for >'pre_value' in code currently as well, that's why I have fixed it in a >similar way in patch. I was assuming that error-recovery would reset the containing memory context, but I'm not sure what memory context we're executing in at this point. -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Mon, Jan 6, 2014 at 7:58 PM, Robert Haas wrote:
> On Sun, Dec 22, 2013 at 10:02 AM, Amit Kapila wrote:
>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
>>> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
>>> it is here.
>>>
FreeConfigVariables(head);
else if (apply)
ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors;
unaffected changes were applied",
ErrorConfFile)));
>>>
>>> The cause of the problem is that, in ProcessConfigFile(), the log
>>> message including
>>> the 'ErrorConfFile' is emitted after 'head' is free'd even though
>>> 'ErrorConfFile' points
>>> to one of entry of the 'head'.
>>
>> Your analysis is absolutely right.
>> The reason for this issue is that we want to display filename to which
>> erroneous parameter
>> belongs and in this case we are freeing the parameter info before actual
>> error.
>> To fix, we need to save the filename value before freeing parameter list.
>>
>> Please find the attached patch to fix this issue.
>>
>>
>
> Couldn't we also handle this by postponing FreeConfigVariables until
> after the if (error) block?
Wouldn't doing that way can lead to bigger memory leak, if error level
is ERROR. Though in current fix also it can leak memory but it will be
just for ErrorConfFile_save. I think some similar case can happen for
'pre_value' in code currently as well, that's why I have fixed it in a
similar way in patch.
> Also, what's the point of this:
>
> + snprintf(ConfigAutoFileName,sizeof(ConfigAutoFileName),"%s",
> PG_AUTOCONF_FILENAME);
>
> Why copy PG_AUTOCONF_FILENAME into another buffer? Why not just pass
> PG_AUTOCONF_FILENAME directly to ParseConfigFile?
Initially, I think we were doing concat of folder and file name for
Autofile, that's why
the code was written that way, but currently it can be changed to way you are
suggesting.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Sun, Dec 22, 2013 at 10:02 AM, Amit Kapila wrote:
> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
>> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
>> it is here.
>>
>> $ psql
>> =# ALTER SYSTEM SET shared_buffers = '512MB';
>> ALTER SYSTEM
>> =# \q
>> $ pg_ctl -D data reload
>> server signaled
>> 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
>> 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
>> changed without restarting the server
>> 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
>> errors; unaffected changes were applied
>>
>> The configuration file name in the last log message is broken. This problem
>> was
>> introduced by the ALTER SYSTEM SET patch.
>>
>>>FreeConfigVariables(head);
>>>
>>>else if (apply)
>>>ereport(elevel,
>>>(errcode(ERRCODE_CONFIG_FILE_ERROR),
>>> errmsg("configuration file \"%s\" contains errors;
>>> unaffected changes were applied",
>>>ErrorConfFile)));
>>
>> The cause of the problem is that, in ProcessConfigFile(), the log
>> message including
>> the 'ErrorConfFile' is emitted after 'head' is free'd even though
>> 'ErrorConfFile' points
>> to one of entry of the 'head'.
>
> Your analysis is absolutely right.
> The reason for this issue is that we want to display filename to which
> erroneous parameter
> belongs and in this case we are freeing the parameter info before actual
> error.
> To fix, we need to save the filename value before freeing parameter list.
>
> Please find the attached patch to fix this issue.
>
> Note - In my m/c, I could not reproduce the issue. I think this is not
> surprising because we
> are not setting freed memory to NULL, so it can display anything
> (sometimes right value as well)
Couldn't we also handle this by postponing FreeConfigVariables until
after the if (error) block?
Also, what's the point of this:
+ snprintf(ConfigAutoFileName,sizeof(ConfigAutoFileName),"%s",
PG_AUTOCONF_FILENAME);
Why copy PG_AUTOCONF_FILENAME into another buffer? Why not just pass
PG_AUTOCONF_FILENAME directly to ParseConfigFile?
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Fri, Dec 27, 2013 at 5:01 AM, Robert Haas wrote: > On Tue, Dec 24, 2013 at 10:08 AM, Fujii Masao wrote: >> When I read ProcessConfigFile() more carefully, I found another related >> problem. The procedure to reproduce the problem is here. >> >> >> $ psql >> =# ALTER SYSTEM SET shared_buffers = '256MB'; >> =# \q >> >> $ echo "shared_buffers = '256MB'" >> $PGDATA/postgresql.conf >> $ pg_ctl -D $PGDATA reload >> 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be >> changed without restarting the server >> 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be >> changed without restarting the server >> 2013-12-25 03:05:44 JST LOG: configuration file >> "/dav/alter-system/data/postgresql.auto.conf" contains errors; >> unaffected changes were applied >> >> >> Both postgresql.conf and postgresql.auto.conf contain the setting of >> the parameter that cannot be changed without restarting the server. >> But only postgresql.auto.conf was logged, and which would be confusing, >> I'm afraid. I think that this problem should be fixed together with the >> problem that I reported before. Thought? > > I have run into this problem on many occasions because my benchmark > scripts usually append a hunk of stuff to postgresql.conf, and any > parameters that were already set generate this warning every time you > reload, because postgresql.conf now mentions that parameter twice. > I'm not quite sure what other problem you want to fix it together > with, The 2 problems are: 1. First is that if user changes the config parameter (for ex. shared_buffers) both by manually editing postgresql.conf and by using Alter System command and then reload the config, it will show the message for parameter 'shared_buffers' twice, but will only show the last file where it exists. The detailed description of problem is in current mail. 2. The other problem is in some cases the name of file it display is junk, problem and fix can be found at link: http://www.postgresql.org/message-id/CAA4eK1+-yD2p=+6tdj_xpruhn4m_ckvfr51ah0gv3sxlgoa...@mail.gmail.com >and I'm not sure what the right fix is either, but +1 for coming > up with some solution that's better than what we have now. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Tue, Dec 24, 2013 at 11:38 PM, Fujii Masao wrote: >> On Sun, Dec 22, 2013 at 8:32 PM, Amit Kapila wrote: >>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote: I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce it is here. $ psql =# ALTER SYSTEM SET shared_buffers = '512MB'; ALTER SYSTEM =# \q $ pg_ctl -D data reload server signaled 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be changed without restarting the server 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains errors; unaffected changes were applied >>> >>> Your analysis is absolutely right. >>> The reason for this issue is that we want to display filename to which >>> erroneous parameter >>> belongs and in this case we are freeing the parameter info before actual >>> error. >>> To fix, we need to save the filename value before freeing parameter list. >>> >>> Please find the attached patch to fix this issue. > > When I read ProcessConfigFile() more carefully, I found another related > problem. The procedure to reproduce the problem is here. > > > $ psql > =# ALTER SYSTEM SET shared_buffers = '256MB'; > =# \q > > $ echo "shared_buffers = '256MB'" >> $PGDATA/postgresql.conf > $ pg_ctl -D $PGDATA reload > 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be > changed without restarting the server > 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be > changed without restarting the server > 2013-12-25 03:05:44 JST LOG: configuration file > "/dav/alter-system/data/postgresql.auto.conf" contains errors; > unaffected changes were applied > > > Both postgresql.conf and postgresql.auto.conf contain the setting of > the parameter that cannot be changed without restarting the server. > But only postgresql.auto.conf was logged, and which would be confusing, > I'm afraid. The reason for above behaviour is that while applying configuration parameters, it just note down the last file which has error and ignore others. Now if we just want to change behaviour for above case that it display both the files, then during processing of parameters, we can save the errorconffile if it is different from previous. However, let us say if user uses include mechanism of conf files and used new file for specifying some of the changed parameters, then again similar thing can be observed. I think here basic idea is user should avoid using multiple methods to change configuration parameters, however we cannot stop them from doing the same. So in some cases like above where user use multiple methods to change configuration can add more work for him to understand and detect the error. Also, some similar behaviour can be observed if user uses include mechanism to specify some config parameters without even Alter System changes. >I think that this problem should be fixed together with the > problem that I reported before. Thought? Here I think you might be worried that if we want to fix the behaviour reported in this mail, it might overlap with the changes of previous fix. So I think let us first decide if we want to keep the current behaviour as it is or would like to change it and if it needs change, what should be the new behaviour? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Tue, Dec 24, 2013 at 10:08 AM, Fujii Masao wrote:
> On Tue, Dec 24, 2013 at 2:57 AM, Amit Kapila wrote:
>> On Sun, Dec 22, 2013 at 8:32 PM, Amit Kapila wrote:
>>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
it is here.
$ psql
=# ALTER SYSTEM SET shared_buffers = '512MB';
ALTER SYSTEM
=# \q
$ pg_ctl -D data reload
server signaled
2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration
files
2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
errors; unaffected changes were applied
The configuration file name in the last log message is broken. This
problem was
introduced by the ALTER SYSTEM SET patch.
>FreeConfigVariables(head);
>
>else if (apply)
>ereport(elevel,
>(errcode(ERRCODE_CONFIG_FILE_ERROR),
> errmsg("configuration file \"%s\" contains errors;
> unaffected changes were applied",
>ErrorConfFile)));
The cause of the problem is that, in ProcessConfigFile(), the log
message including
the 'ErrorConfFile' is emitted after 'head' is free'd even though
'ErrorConfFile' points
to one of entry of the 'head'.
>>>
>>> Your analysis is absolutely right.
>>> The reason for this issue is that we want to display filename to which
>>> erroneous parameter
>>> belongs and in this case we are freeing the parameter info before actual
>>> error.
>>> To fix, we need to save the filename value before freeing parameter list.
>>>
>>> Please find the attached patch to fix this issue.
>>>
>>> Note - In my m/c, I could not reproduce the issue. I think this is not
>>> surprising because we
>>> are not setting freed memory to NULL, so it can display anything
>>> (sometimes right value as well)
>>
>> If you find that the provided patch doesn't fix the problem or you don't
>> find it appropriate due to some other reason, let me know the feedback.
>
> When I read ProcessConfigFile() more carefully, I found another related
> problem. The procedure to reproduce the problem is here.
>
>
> $ psql
> =# ALTER SYSTEM SET shared_buffers = '256MB';
> =# \q
>
> $ echo "shared_buffers = '256MB'" >> $PGDATA/postgresql.conf
> $ pg_ctl -D $PGDATA reload
> 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be
> changed without restarting the server
> 2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be
> changed without restarting the server
> 2013-12-25 03:05:44 JST LOG: configuration file
> "/dav/alter-system/data/postgresql.auto.conf" contains errors;
> unaffected changes were applied
>
>
> Both postgresql.conf and postgresql.auto.conf contain the setting of
> the parameter that cannot be changed without restarting the server.
> But only postgresql.auto.conf was logged, and which would be confusing,
> I'm afraid. I think that this problem should be fixed together with the
> problem that I reported before. Thought?
I have run into this problem on many occasions because my benchmark
scripts usually append a hunk of stuff to postgresql.conf, and any
parameters that were already set generate this warning every time you
reload, because postgresql.conf now mentions that parameter twice.
I'm not quite sure what other problem you want to fix it together
with, and I'm not sure what the right fix is either, but +1 for coming
up with some solution that's better than what we have now.
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Tue, Dec 24, 2013 at 2:57 AM, Amit Kapila wrote:
> On Sun, Dec 22, 2013 at 8:32 PM, Amit Kapila wrote:
>> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
>>> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
>>> it is here.
>>>
>>> $ psql
>>> =# ALTER SYSTEM SET shared_buffers = '512MB';
>>> ALTER SYSTEM
>>> =# \q
>>> $ pg_ctl -D data reload
>>> server signaled
>>> 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
>>> 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
>>> changed without restarting the server
>>> 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
>>> errors; unaffected changes were applied
>>>
>>> The configuration file name in the last log message is broken. This problem
>>> was
>>> introduced by the ALTER SYSTEM SET patch.
>>>
FreeConfigVariables(head);
else if (apply)
ereport(elevel,
(errcode(ERRCODE_CONFIG_FILE_ERROR),
errmsg("configuration file \"%s\" contains errors;
unaffected changes were applied",
ErrorConfFile)));
>>>
>>> The cause of the problem is that, in ProcessConfigFile(), the log
>>> message including
>>> the 'ErrorConfFile' is emitted after 'head' is free'd even though
>>> 'ErrorConfFile' points
>>> to one of entry of the 'head'.
>>
>> Your analysis is absolutely right.
>> The reason for this issue is that we want to display filename to which
>> erroneous parameter
>> belongs and in this case we are freeing the parameter info before actual
>> error.
>> To fix, we need to save the filename value before freeing parameter list.
>>
>> Please find the attached patch to fix this issue.
>>
>> Note - In my m/c, I could not reproduce the issue. I think this is not
>> surprising because we
>> are not setting freed memory to NULL, so it can display anything
>> (sometimes right value as well)
>
> If you find that the provided patch doesn't fix the problem or you don't
> find it appropriate due to some other reason, let me know the feedback.
When I read ProcessConfigFile() more carefully, I found another related
problem. The procedure to reproduce the problem is here.
$ psql
=# ALTER SYSTEM SET shared_buffers = '256MB';
=# \q
$ echo "shared_buffers = '256MB'" >> $PGDATA/postgresql.conf
$ pg_ctl -D $PGDATA reload
2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-25 03:05:44 JST LOG: parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-25 03:05:44 JST LOG: configuration file
"/dav/alter-system/data/postgresql.auto.conf" contains errors;
unaffected changes were applied
Both postgresql.conf and postgresql.auto.conf contain the setting of
the parameter that cannot be changed without restarting the server.
But only postgresql.auto.conf was logged, and which would be confusing,
I'm afraid. I think that this problem should be fixed together with the
problem that I reported before. Thought?
Regards,
--
Fujii Masao
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On 23 December 2013 19:35, Robert Haas wrote: > On Mon, Dec 23, 2013 at 2:19 PM, Thom Brown wrote: >>> I would think that you'd need to have auto_explain loaded in the >>> backend where you're trying to make a change, but you shouldn't need >>> the setting to be present in postgresql.conf, I would think. >> >> This appears to be the case. I hadn't set the library to be loaded in >> the config. >> >> I guess therefore it follows that arbitrary configuration parameters >> aren't supported (e.g. moo.bark = 5), only pre-defined ones. > > Yeah, and that's by design. Otherwise, it would be too easy to set a > config parameter to a value that wasn't legal, and therefore make the > server fail to start. moo.bark = 5 likely won't cause any problems, > but auto_explain.log_verbose = fasle could. By insisting that the > module providing the GUC be loaded, we can sanity-check the value at > the time it gets set. Alles klar. :) -- Thom -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Mon, Dec 23, 2013 at 2:19 PM, Thom Brown wrote: >> I would think that you'd need to have auto_explain loaded in the >> backend where you're trying to make a change, but you shouldn't need >> the setting to be present in postgresql.conf, I would think. > > This appears to be the case. I hadn't set the library to be loaded in > the config. > > I guess therefore it follows that arbitrary configuration parameters > aren't supported (e.g. moo.bark = 5), only pre-defined ones. Yeah, and that's by design. Otherwise, it would be too easy to set a config parameter to a value that wasn't legal, and therefore make the server fail to start. moo.bark = 5 likely won't cause any problems, but auto_explain.log_verbose = fasle could. By insisting that the module providing the GUC be loaded, we can sanity-check the value at the time it gets set. -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On 23 December 2013 19:14, Robert Haas wrote: > On Mon, Dec 23, 2013 at 1:42 PM, Thom Brown wrote: >> Discussion around this topic has reached hundreds of messages, and >> whilst I have failed to find mention of my following question, I >> appreciate it may have already been discussed. >> >> I have noticed that configuration parameters for extensions are only >> supported if the server was started with one of the parameters already >> being set in postgresql.conf: >> >> # without any mention in postgresql.conf >> postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; >> ERROR: unrecognized configuration parameter "auto_explain.log_verbose" >> >> # with auto_explain.log_verbose = false added to postgresql.conf >> postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; >> ALTER SYSTEM >> >> Removing the entry from postgresql.conf, restarting Postgres, setting >> it to the default, then restarting again renders it unsettable again: >> >> # removed entry from postgresql.conf and restarted >> >> postgres=# ALTER SYSTEM SET auto_explain.log_verbose = default; >> ALTER SYSTEM >> >> # restart postgres >> >> postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; >> ERROR: unrecognized configuration parameter "auto_explain.log_verbose" >> >> Is this by design? > > I would think that you'd need to have auto_explain loaded in the > backend where you're trying to make a change, but you shouldn't need > the setting to be present in postgresql.conf, I would think. This appears to be the case. I hadn't set the library to be loaded in the config. I guess therefore it follows that arbitrary configuration parameters aren't supported (e.g. moo.bark = 5), only pre-defined ones. Thanks Thom -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Mon, Dec 23, 2013 at 1:42 PM, Thom Brown wrote: > Discussion around this topic has reached hundreds of messages, and > whilst I have failed to find mention of my following question, I > appreciate it may have already been discussed. > > I have noticed that configuration parameters for extensions are only > supported if the server was started with one of the parameters already > being set in postgresql.conf: > > # without any mention in postgresql.conf > postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; > ERROR: unrecognized configuration parameter "auto_explain.log_verbose" > > # with auto_explain.log_verbose = false added to postgresql.conf > postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; > ALTER SYSTEM > > Removing the entry from postgresql.conf, restarting Postgres, setting > it to the default, then restarting again renders it unsettable again: > > # removed entry from postgresql.conf and restarted > > postgres=# ALTER SYSTEM SET auto_explain.log_verbose = default; > ALTER SYSTEM > > # restart postgres > > postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; > ERROR: unrecognized configuration parameter "auto_explain.log_verbose" > > Is this by design? I would think that you'd need to have auto_explain loaded in the backend where you're trying to make a change, but you shouldn't need the setting to be present in postgresql.conf, I would think. -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
Discussion around this topic has reached hundreds of messages, and whilst I have failed to find mention of my following question, I appreciate it may have already been discussed. I have noticed that configuration parameters for extensions are only supported if the server was started with one of the parameters already being set in postgresql.conf: # without any mention in postgresql.conf postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; ERROR: unrecognized configuration parameter "auto_explain.log_verbose" # with auto_explain.log_verbose = false added to postgresql.conf postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; ALTER SYSTEM Removing the entry from postgresql.conf, restarting Postgres, setting it to the default, then restarting again renders it unsettable again: # removed entry from postgresql.conf and restarted postgres=# ALTER SYSTEM SET auto_explain.log_verbose = default; ALTER SYSTEM # restart postgres postgres=# ALTER SYSTEM SET auto_explain.log_verbose = true; ERROR: unrecognized configuration parameter "auto_explain.log_verbose" Is this by design? -- Thom -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Sun, Dec 22, 2013 at 8:32 PM, Amit Kapila wrote:
> On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
>> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
>> it is here.
>>
>> $ psql
>> =# ALTER SYSTEM SET shared_buffers = '512MB';
>> ALTER SYSTEM
>> =# \q
>> $ pg_ctl -D data reload
>> server signaled
>> 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
>> 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
>> changed without restarting the server
>> 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
>> errors; unaffected changes were applied
>>
>> The configuration file name in the last log message is broken. This problem
>> was
>> introduced by the ALTER SYSTEM SET patch.
>>
>>>FreeConfigVariables(head);
>>>
>>>else if (apply)
>>>ereport(elevel,
>>>(errcode(ERRCODE_CONFIG_FILE_ERROR),
>>> errmsg("configuration file \"%s\" contains errors;
>>> unaffected changes were applied",
>>>ErrorConfFile)));
>>
>> The cause of the problem is that, in ProcessConfigFile(), the log
>> message including
>> the 'ErrorConfFile' is emitted after 'head' is free'd even though
>> 'ErrorConfFile' points
>> to one of entry of the 'head'.
>
> Your analysis is absolutely right.
> The reason for this issue is that we want to display filename to which
> erroneous parameter
> belongs and in this case we are freeing the parameter info before actual
> error.
> To fix, we need to save the filename value before freeing parameter list.
>
> Please find the attached patch to fix this issue.
>
> Note - In my m/c, I could not reproduce the issue. I think this is not
> surprising because we
> are not setting freed memory to NULL, so it can display anything
> (sometimes right value as well)
If you find that the provided patch doesn't fix the problem or you don't
find it appropriate due to some other reason, let me know the feedback.
I shall be able to provide updated patch early next as I will be on vacation for
remaining part of this week.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Sun, Dec 22, 2013 at 3:01 PM, Fujii Masao wrote:
> I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
> it is here.
>
> $ psql
> =# ALTER SYSTEM SET shared_buffers = '512MB';
> ALTER SYSTEM
> =# \q
> $ pg_ctl -D data reload
> server signaled
> 2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
> 2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
> changed without restarting the server
> 2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
> errors; unaffected changes were applied
>
> The configuration file name in the last log message is broken. This problem
> was
> introduced by the ALTER SYSTEM SET patch.
>
>>FreeConfigVariables(head);
>>
>>else if (apply)
>>ereport(elevel,
>>(errcode(ERRCODE_CONFIG_FILE_ERROR),
>> errmsg("configuration file \"%s\" contains errors;
>> unaffected changes were applied",
>>ErrorConfFile)));
>
> The cause of the problem is that, in ProcessConfigFile(), the log
> message including
> the 'ErrorConfFile' is emitted after 'head' is free'd even though
> 'ErrorConfFile' points
> to one of entry of the 'head'.
Your analysis is absolutely right.
The reason for this issue is that we want to display filename to which
erroneous parameter
belongs and in this case we are freeing the parameter info before actual error.
To fix, we need to save the filename value before freeing parameter list.
Please find the attached patch to fix this issue.
Note - In my m/c, I could not reproduce the issue. I think this is not
surprising because we
are not setting freed memory to NULL, so it can display anything
(sometimes right value as well)
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
display_error_conf_filename.patch
Description: Binary data
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Fri, Dec 20, 2013 at 2:35 AM, Fujii Masao wrote:
> On Thu, Dec 19, 2013 at 2:21 PM, Tatsuo Ishii wrote:
>>> I found that the psql tab-completion for ALTER SYSTEM SET has not been
>>> implemented yet.
>>> Attached patch does that. Barring any objections, I will commit this patch.
>>
>> Good catch!
>
> Committed.
I found the bug of ALTER SYSTEM SET patch. The procedure to reproduce
it is here.
$ psql
=# ALTER SYSTEM SET shared_buffers = '512MB';
ALTER SYSTEM
=# \q
$ pg_ctl -D data reload
server signaled
2013-12-22 18:24:13 JST LOG: received SIGHUP, reloading configuration files
2013-12-22 18:24:13 JST LOG: parameter "shared_buffers" cannot be
changed without restarting the server
2013-12-22 18:24:13 JST LOG: configuration file "X??" contains
errors; unaffected changes were applied
The configuration file name in the last log message is broken. This problem was
introduced by the ALTER SYSTEM SET patch.
>FreeConfigVariables(head);
>
>else if (apply)
>ereport(elevel,
>(errcode(ERRCODE_CONFIG_FILE_ERROR),
> errmsg("configuration file \"%s\" contains errors;
> unaffected changes were applied",
>ErrorConfFile)));
The cause of the problem is that, in ProcessConfigFile(), the log
message including
the 'ErrorConfFile' is emitted after 'head' is free'd even though
'ErrorConfFile' points
to one of entry of the 'head'.
Regards,
--
Fujii Masao
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Thu, Dec 19, 2013 at 2:21 PM, Tatsuo Ishii wrote: >> I found that the psql tab-completion for ALTER SYSTEM SET has not been >> implemented yet. >> Attached patch does that. Barring any objections, I will commit this patch. > > Good catch! Committed. Regards, -- Fujii Masao -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
> I found that the psql tab-completion for ALTER SYSTEM SET has not been > implemented yet. > Attached patch does that. Barring any objections, I will commit this patch. Good catch! Best regards, -- Tatsuo Ishii SRA OSS, Inc. Japan English: http://www.sraoss.co.jp/index_en.php Japanese: http://www.sraoss.co.jp -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Thu, Dec 19, 2013 at 12:08 PM, Amit Kapila wrote:
> On Wed, Dec 18, 2013 at 8:25 PM, Tatsuo Ishii wrote:
Is there any reason for the function returns int as it always returns
0 or 1. Maybe returns bool is better?
>>>
>>
>> I have committed your patches. Thanks.
>
> Thank you very much.
I found that the psql tab-completion for ALTER SYSTEM SET has not been
implemented yet.
Attached patch does that. Barring any objections, I will commit this patch.
Regards,
--
Fujii Masao
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
***
*** 541,546 static const SchemaQuery Query_for_list_of_matviews = {
--- 541,552
"SELECT pg_catalog.quote_ident(nspname) FROM pg_catalog.pg_namespace "\
" WHERE substring(pg_catalog.quote_ident(nspname),1,%d)='%s'"
+ #define Query_for_list_of_alter_system_set_vars \
+ "SELECT name FROM "\
+ " (SELECT pg_catalog.lower(name) AS name FROM pg_catalog.pg_settings "\
+ " WHERE context != 'internal') ss "\
+ " WHERE substring(name,1,%d)='%s'"
+
#define Query_for_list_of_set_vars \
"SELECT name FROM "\
" (SELECT pg_catalog.lower(name) AS name FROM pg_catalog.pg_settings "\
***
*** 930,936 psql_completion(char *text, int start, int end)
{"AGGREGATE", "COLLATION", "CONVERSION", "DATABASE", "DEFAULT PRIVILEGES", "DOMAIN",
"EXTENSION", "FOREIGN DATA WRAPPER", "FOREIGN TABLE", "FUNCTION",
"GROUP", "INDEX", "LANGUAGE", "LARGE OBJECT", "MATERIALIZED VIEW", "OPERATOR",
! "ROLE", "RULE", "SCHEMA", "SERVER", "SEQUENCE", "TABLE",
"TABLESPACE", "TEXT SEARCH", "TRIGGER", "TYPE",
"USER", "USER MAPPING FOR", "VIEW", NULL};
--- 936,942
{"AGGREGATE", "COLLATION", "CONVERSION", "DATABASE", "DEFAULT PRIVILEGES", "DOMAIN",
"EXTENSION", "FOREIGN DATA WRAPPER", "FOREIGN TABLE", "FUNCTION",
"GROUP", "INDEX", "LANGUAGE", "LARGE OBJECT", "MATERIALIZED VIEW", "OPERATOR",
! "ROLE", "RULE", "SCHEMA", "SERVER", "SEQUENCE", "SYSTEM SET", "TABLE",
"TABLESPACE", "TEXT SEARCH", "TRIGGER", "TYPE",
"USER", "USER MAPPING FOR", "VIEW", NULL};
***
*** 1263,1268 psql_completion(char *text, int start, int end)
--- 1269,1279
COMPLETE_WITH_LIST(list_ALTER_SERVER);
}
+ /* ALTER SYSTEM SET */
+ else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev2_wd, "SYSTEM") == 0 &&
+ pg_strcasecmp(prev_wd, "SET") == 0)
+ COMPLETE_WITH_QUERY(Query_for_list_of_alter_system_set_vars);
/* ALTER VIEW */
else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
pg_strcasecmp(prev2_wd, "VIEW") == 0)
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Wed, Dec 18, 2013 at 8:25 PM, Tatsuo Ishii wrote: >>> Is there any reason for the function returns int as it always returns >>> 0 or 1. Maybe returns bool is better? >> > > I have committed your patches. Thanks. Thank you very much. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
>> Is there any reason for the function returns int as it always returns
>> 0 or 1. Maybe returns bool is better?
>
> No, return type should be bool, I have changed the same in attached patch.
Confirmed.
>> 2) initdb.c
>>
>> + strcpy(tempautobuf, "# Do not edit this file manually! \n");
>> + autoconflines[0] = pg_strdup(tempautobuf);
>> + strcpy(tempautobuf, "# It will be overwritten by the ALTER SYSTEM
>> command. \n");
>> + autoconflines[1] = pg_strdup(tempautobuf);
>>
>> Is there any reason to use "tempautobuf" here? I think we can simply change
>> to this:
>>
>> + autoconflines[0] = pg_strdup("# Do not edit this file manually! \n");
>> + autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER
>> SYSTEM command. \n");
>
> You are right, I have changed code as per your suggestion.
Confirmed.
>> 3) initdb.c
>>
>> It seems the memory allocated for autoconflines[0] and
>> autoconflines[1] by pg_strdup is never freed.
>
> I think, it gets freed in writefile() in below code.
Oh, I see. Sorry for noise.
I have committed your patches. Thanks.
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
On 12/18/2013 03:35 AM, Tatsuo Ishii wrote: 3) initdb.c It seems the memory allocated for autoconflines[0] and autoconflines[1] by pg_strdup is never freed. (I think there's similar problem with "conflines" as well, though it was not introduced by the patch). Why would we care? initdb doesn't run for very long, and the memory will be freed when it exits, usually within a few seconds. My recollection from back when I originally rewrote initdb in C was that cleaning up the memory leaks wasn't worth the trouble. cheers andrew -- 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] ALTER SYSTEM SET command to change postgresql.conf parameters
On Wed, Dec 18, 2013 at 2:05 PM, Tatsuo Ishii wrote:
> Hi,
>
> I have looked into this because it's marked as "ready for committer".
Thank you.
> It looks like working as advertised. Great! However I have noticed a
> few minor issues.
>
> 1) validate_conf_option
>
> +/*
> + * Validates configuration parameter and value, by calling check hook
> functions
> + * depending on record's vartype. It validates if the parameter
> + * value given is in range of expected predefined value for that parameter.
> + *
> + * freemem - true indicates memory for newval and newextra will be
> + * freed in this function, false indicates it will be
> freed
> + * by caller.
> + * Return value:
> + * 1: the value is valid
> + * 0: the name or value is invalid
> + */
> +int
> +validate_conf_option(struct config_generic * record, const char *name,
> +const char *value, GucSource source,
> int elevel,
> +bool freemem, void *newval, void
> **newextra)
>
> Is there any reason for the function returns int as it always returns
> 0 or 1. Maybe returns bool is better?
No, return type should be bool, I have changed the same in attached patch.
> 2) initdb.c
>
> + strcpy(tempautobuf, "# Do not edit this file manually! \n");
> + autoconflines[0] = pg_strdup(tempautobuf);
> + strcpy(tempautobuf, "# It will be overwritten by the ALTER SYSTEM
> command. \n");
> + autoconflines[1] = pg_strdup(tempautobuf);
>
> Is there any reason to use "tempautobuf" here? I think we can simply change
> to this:
>
> + autoconflines[0] = pg_strdup("# Do not edit this file manually! \n");
> + autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER
> SYSTEM command. \n");
You are right, I have changed code as per your suggestion.
> 3) initdb.c
>
> It seems the memory allocated for autoconflines[0] and
> autoconflines[1] by pg_strdup is never freed.
I think, it gets freed in writefile() in below code.
for (line = lines; *line != NULL; line++)
{
if (fputs(*line, out_file) < 0)
{
fprintf(stderr, _("%s: could not write file \"%s\": %s\n"),
progname, path, strerror(errno));
exit_nicely();
}
free(*line);
}
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
alter_system_v13.patch
Description: Binary data
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters
Hi,
I have looked into this because it's marked as "ready for committer".
> On Tue, Nov 19, 2013 at 2:06 PM, Haribabu kommi
> wrote:
>> On 19 November 2013 09:59 Amit Kapila wrote:
>>> On Mon, Nov 18, 2013 at 8:31 PM, Haribabu kommi
>>> wrote:
>>> > On 18 November 2013 20:01 Amit Kapila wrote:
>>> >> > Code changes are fine.
>>> >> > If two or three errors are present in the configuration file, it
>>> >> prints the last error
>>
>> Ok fine I marked the patch as ready for committer.
>
>Thanks for review.
>
> With Regards,
> Amit Kapila.
> EnterpriseDB: http://www.enterprisedb.com
It looks like working as advertised. Great! However I have noticed a
few minor issues.
1) validate_conf_option
+/*
+ * Validates configuration parameter and value, by calling check hook functions
+ * depending on record's vartype. It validates if the parameter
+ * value given is in range of expected predefined value for that parameter.
+ *
+ * freemem - true indicates memory for newval and newextra will be
+ * freed in this function, false indicates it will be
freed
+ * by caller.
+ * Return value:
+ * 1: the value is valid
+ * 0: the name or value is invalid
+ */
+int
+validate_conf_option(struct config_generic * record, const char *name,
+const char *value, GucSource source,
int elevel,
+bool freemem, void *newval, void
**newextra)
Is there any reason for the function returns int as it always returns
0 or 1. Maybe returns bool is better?
2) initdb.c
+ strcpy(tempautobuf, "# Do not edit this file manually! \n");
+ autoconflines[0] = pg_strdup(tempautobuf);
+ strcpy(tempautobuf, "# It will be overwritten by the ALTER SYSTEM
command. \n");
+ autoconflines[1] = pg_strdup(tempautobuf);
Is there any reason to use "tempautobuf" here? I think we can simply change to
this:
+ autoconflines[0] = pg_strdup("# Do not edit this file manually! \n");
+ autoconflines[1] = pg_strdup("# It will be overwritten by the ALTER
SYSTEM command. \n");
3) initdb.c
It seems the memory allocated for autoconflines[0] and
autoconflines[1] by pg_strdup is never freed.
(I think there's similar problem with "conflines" as well, though it
was not introduced by the patch).
Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese: http://www.sraoss.co.jp
--
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] ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])
Greg Stark writes: > Writing out each guc in a separate file is a singularly bad idea. It's I'm not buying into any of your arguments here, and have something to add to that part: > I'm not even clear we do want this in /etc since none of our GUC > options are repeatable things like Apache virtual servers. It actually > makes *more* sense for pg_hba than it does for gucs. I think we can > assume that in the future we'll have something like it however. Given a conf.d option in /etc it's then quite easy to add per-extension configuration files in the packaging system, so that users don't have to edit postgresql.conf for default values. We still need some kind of repeatable settings that we don't have yet for that to happen: cumulative setting of a "list" GUC such as local_preload_libraries and suchlike. Regards, -- Dimitri Fontaine http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
