Re: Re: Proposal for Allow postgresql.conf values to be changed via SQL [review]

On Tuesday, January 22, 2013 10:14 PM Fujii Masao wrote:
> On Tue, Jan 22, 2013 at 11:54 PM, Fujii Masao <masao.fujii@gmail.com>
> wrote:
> > On Tue, Jan 22, 2013 at 11:07 PM, Amit Kapila
> <amit.kapila@huawei.com> wrote:
> >> On Tuesday, January 22, 2013 7:10 PM Zoltán Böszörményi wrote:
> >>> 2013-01-22 13:32 keltezéssel, Amit kapila írta:
> >>> > On Saturday, January 19, 2013 2:37 AM Boszormenyi Zoltan wrote:
> >>> > 2013-01-18 21:48 keltezéssel, Boszormenyi Zoltan írta:
> >>> >> 2013-01-18 21:32 keltezéssel, Tom Lane írta:
> >>> >>> Boszormenyi Zoltan <zb@cybertec.at> writes:
> >>> >>>> 2013-01-18 11:05 keltezéssel, Amit kapila írta:
> >>> >>>>>> On using mktemp, linux compilation gives below warning
> >>> >>>>>> warning: the use of `mktemp' is dangerous, better use
> `mkstemp'
> >>> >
> >>> >>>> Everywhere else that we need to do something like this, we
> just
> >>> use our
> >>> >>>> own PID to disambiguate, ie
> >>> >>>>      sprintf(tempfilename, "/path/to/file.%d", (int)
> getpid());
> >>> >>>> There is no need to deviate from that pattern or introduce
> >>> portability
> >>> >>>> issues, since we can reasonably assume that no non-Postgres
> >>> programs are
> >>> >>>> creating files in this directory.
> >>> >>> Thanks for the enlightenment, I will post a new version soon.
> >>> >> Here it is.
> >>> > The patch sent by you works fine.
> >>> > It needs small modification as below:
> >>> >
> >>> > The "auto.conf.d" directory should follow the postgresql.conf
> file
> >>> directory not the data_directory.
> >>> > The same is validated while parsing the postgresql.conf
> configuration
> >>> file.
> >>> >
> >>> > Patch is changed to use the postgresql.conf file directory as
> below.
> >>> >
> >>> > StrNCpy(ConfigFileDir, ConfigFileName, sizeof(ConfigFileDir));
> >>> > get_parent_directory(ConfigFileDir);
> >>> > /* Frame auto conf name and auto conf sample temp file name */
> >>> > snprintf(AutoConfFileName, sizeof(AutoConfFileName), "%s/%s/%s",
> >>> >                                          ConfigFileDir,
> >>> >                                          PG_AUTOCONF_DIR,
> >>> >                                          PG_AUTOCONF_FILENAME);
> >>>
> >>> Maybe it's just me but I prefer to have identical
> >>> settings across all replicated servers. But I agree
> >>> that there can be use cases with different setups.
> >>>
> >>> All in all, this change makes it necessary to run the
> >>> same SET PERSISTENT statements on all slave servers,
> >>> too, to make them identical again if the configuration
> >>> is separated from the data directory (like on Debian
> >>> or Ubuntu using the default packages). This needs to be
> >>> documented explicitly.
> >>
> >> SET PERSISTENT command needs to run on all slave servers even if the
> path we
> >> take w.r.t data directory
> >> unless user takes backup after command.
> >
> > Is it safe to write something in the directory other than data
> directory
> > via SQL?
> >
> > postgres user usually has the write permission for the configuration
> > directory like /etc/postgresql?
> >
> >>> > This closes all comments raised till now for this patch.
> >>> > Kindly let me know if you feel something is missing?
> >>>
> >>> I can't think of anything else.
> >
> > For removing
> > +   a configuration entry from
> <filename>postgresql.auto.conf</filename> file,
> > +   use <command>RESET PERSISTENT</command>. The values will be
> effective
> > +   after reload of server configuration (SIGHUP) or server startup.
> >
> > The description of RESET PERSISTENT is in the document, but it
> > seems not to be implemented.
>
> I read only document part in the patch and did simple test.
> Here are the comments:
>
> storage.sgml needs to be updated.
>
> Doesn't auto.conf.d need to be explained in config.sgml?

I shall update both these files.
> When I created my own configuration file in auto.conf.d and
> started the server, I got the following LOG message. This
> means that users should not create any their own configuration
> file there? Why shouldn't they?

It can be allowed, but for now it has been kept such that automatically
generated conf files will be
present in this directory, that’s what the name 'auto.conf.d' suggests.

>  I think that it's more useful to
> allow users to put also their own configuration files in auto.conf.d.
> Then users can include any configuration file without adding
> new include derective into postgresql.conf because auto.conf.d
> has already been included.
>
> LOG:  unexpected file found in automatic configuration directory:
> "/data/auto.conf.d/hoge"

Personally I don't have objection in getting other user specific conf files
in this directory.
But I think then we should name this directory also differently as it was
initially (conf_dir) in the Patch.
I would like to see opinion of others also in this matter, so that later I
don't need to
change it back to what currently it is.

> When I removed postgresql.auto.conf and restarted the server,
> I got the following warning message. This is not correct because
> I didn't remove "auto.conf.d" from postgresql.conf. What I removed
> is only postgresql.auto.conf.
>
> WARNING:  Default "auto.conf.d" is not present in postgresql.conf.
> Configuration parameters changed with SET PERSISTENT command will not
> be effective.

How about changing it to below message:

WARNING:  File 'postgresql.auto.conf' is not accessible, either file
'postgresql.auto.conf' or folder '%s' doesn't exist or default "auto.conf.d"
is not present in postgresql.conf.
Configuration parameters changed with SET PERSISTENT command will not be
effective.

The reason I have kept single error message for all issues related to
'postgresql.auto.conf' is to keep code little simpler, else I need to
distinguish for each particular case.

With Regards,
Amit Kapila.