Re: ALTER SYSTEM SET command to change postgresql.conf parameters (RE: Proposal for Allow postgresql.conf values to be changed via SQL [review])

On Monday, July 15, 2013 11:51 PM Fujii Masao wrote:
On Mon, Jul 15, 2013 at 10:43 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Friday, July 12, 2013 6:46 PM Amit kapila wrote:
>> On Friday, July 12, 2013 12:02 AM Fujii Masao wrote:
>> On Fri, Jul 5, 2013 at 2:19 PM, Amit Kapila <amit(dot)kapila(at)huawei(dot)com>
>> wrote:
>> > On Wednesday, July 03, 2013 2:58 AM Alvaro Herrera wrote:
>> >> Amit Kapila escribió:
>> >>
>>

> Thanks for updating the patch!

> In the patch, ALTER SYSTEM SET validates the postgresql.conf.
> I think this is overkill.

I assume you are talking about below code, if I am wrong please point me to the exact code you are reffering:

/*
* Verify if include_dir for postgresql.auto.conf file exists in
* postgresql.conf. If it doesn't exist then warn the user that parameters
* changed with this command will not be effective.
*/
autoconf_errcause = AUTOCONF_DIR_NOT_PARSED;

if (!ParseConfigFile(ConfigFileName, NULL, false, 0, LOG, &head, &tail))
ereport(ERROR,
(errmsg("configuration file \"%s\" contains errors;
parameters changed by "
"this command will not be effective", ConfigFileName)));

/*
* ignore the list of options as we are intersted to just ensure the
* existence of include directive for config folder.
*/
head = tail = NULL;

if (autoconf_errcause == AUTOCONF_DIR_NOT_PARSED)
ereport(WARNING,
(errmsg("configuration parameters changed by this
command will not be effective"),
errdetail("default include directive for \"%s\"
folder is not present in postgresql.conf", PG_AUTOCONF_DIR)));

This code is to parse postgresql.conf to check if include directive for config directory is present.

> While ALTER SYSTEM SET is being executed,
> a user might be modifying the postgresql.conf. That validation
> breaks this use case.

> +# This includes the default configuration directory included to support
> +# ALTER SYSTEM statement.
> +#
> +# WARNING: User should not remove below include_dir or directory config,
> +# as both are required to make ALTER SYSTEM command work.
> +# Any configuration parameter values specified after this line
> +# will override the values set by ALTER SYSTEM statement.
> +#include_dir = 'config'

> Why do we need to expose this setting to a user?

a) This can be a knob to turn this feature off. This has been asked by few people,
one of the mail link is mentioned below (refer towards end of mail in the link):
http://www.postgresql.org/message-id/515B04F9.30900@gmx.net

b) In case user wants to change priority of parameters set by Alter System, he can move the
include_dir up or down in postgresql.conf.

> As the warning says,
> if a user *should not* remove this setting, I think we should not expose
> it from the beginning and should always treat the postgresql.auto.conf
> as included configuration file even if that setting is not in postgresql.conf.

I think it will be usefull to keep include_dir for some users.

If you think that above use for keeping include_dir is not worth or it can be provided in some
another way, then we can change this behavior and remove parsing of postgresql.conf from
AlterSystemSetConfigFile() function.

Thank you very much for reviewing this patch so carefully and giving valuable feedback.

With Regards,
Amit Kapila.