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

On Tuesday, January 22, 2013 8:25 PM Fujii Masao 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?

As postgresql.conf will also be in same path and if user can change that,
then what's the problem with postgresql.auto.conf

Do you see any security risk?

> >> > 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.

This command support has been removed from patch due to parser issues, so it
should be removed from documentation as well. I shall fix this along with
other issues raised by you.

With Regards,
Amit Kapila.