[REVIEW] Re: postgresql.auto.conf read from wrong directory

Hi,

if you split configuration and data by setting data_directory,
postgresql.auto.conf is writen to the data directory
(/var/lib/postgresql/9.4/main in Debian), but tried to be read from
the etc directory (/etc/postgresql/9.4/main).

One place to fix it would be in ProcessConfigFile in
src/backend/utils/misc/guc-file.l:162 by always setting
CallingFileName = NULL in src/backend/utils/misc/guc-file.l:162, but
that breaks later in AbsoluteConfigLocation() when data_directory is
NULL. (As the comment in ProcessConfigFile says.)

I'm not sure where to break that dependency loop - the fix itself
seems easy, just don't tell to look for postgresql.auto.conf next to
ConfigFileName, but in the data_directory.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
> Hi,
>
> if you split configuration and data by setting data_directory,
> postgresql.auto.conf is writen to the data directory
> (/var/lib/postgresql/9.4/main in Debian), but tried to be read from
> the etc directory (/etc/postgresql/9.4/main).
>
> One place to fix it would be in ProcessConfigFile in
> src/backend/utils/misc/guc-file.l:162 by always setting
> CallingFileName = NULL in src/backend/utils/misc/guc-file.l:162, but
> that breaks later in AbsoluteConfigLocation() when data_directory is
> NULL. (As the comment in ProcessConfigFile says.)
>
> I'm not sure where to break that dependency loop - the fix itself
> seems easy, just don't tell to look for postgresql.auto.conf next to
> ConfigFileName, but in the data_directory.

Thanks for the report, will look into it.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
> Hi,
>
> if you split configuration and data by setting data_directory,
> postgresql.auto.conf is writen to the data directory
> (/var/lib/postgresql/9.4/main in Debian), but tried to be read from
> the etc directory (/etc/postgresql/9.4/main).
>
> One place to fix it would be in ProcessConfigFile in
> src/backend/utils/misc/guc-file.l:162 by always setting
> CallingFileName = NULL in src/backend/utils/misc/guc-file.l:162, but
> that breaks later in AbsoluteConfigLocation() when data_directory is
> NULL. (As the comment in ProcessConfigFile says.)

This problem occurs because we don't have the value of data_directory
set in postgresql.conf by the time we want to parse .auto.conf file
during server start. The value of data_directory is only available after
processing of config files. To fix it, we need to store the value of
data_directory during parse of postgresql.conf file so that we can use it
till data_directory is actually set. Attached patch fixes the problem.
Could you please once confirm if it fixes the problem in your
env./scenario.

Another way to fix could be that during parse, we directly set the config
value of data_directory, but I didn't prefer that way because the parse
routine is called from multiple paths which later on process the values
separately.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Amit Kapila 2014-05-07 <CAA4eK1KTJkpVMnkOS2gFnnh2ZskO4ggDsPSWsHJbq1Cpu9EWRQ(at)mail(dot)gmail(dot)com>
> This problem occurs because we don't have the value of data_directory
> set in postgresql.conf by the time we want to parse .auto.conf file
> during server start. The value of data_directory is only available after
> processing of config files. To fix it, we need to store the value of
> data_directory during parse of postgresql.conf file so that we can use it
> till data_directory is actually set. Attached patch fixes the problem.
> Could you please once confirm if it fixes the problem in your
> env./scenario.

Hi Amit,

the patch fixes the problem. Thanks for looking into this.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Wed, May 7, 2014 at 4:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>> Hi,
>>
>> if you split configuration and data by setting data_directory,
>> postgresql.auto.conf is writen to the data directory
>> (/var/lib/postgresql/9.4/main in Debian), but tried to be read from
>> the etc directory (/etc/postgresql/9.4/main).
>>
>> One place to fix it would be in ProcessConfigFile in
>> src/backend/utils/misc/guc-file.l:162 by always setting
>> CallingFileName = NULL in src/backend/utils/misc/guc-file.l:162, but
>> that breaks later in AbsoluteConfigLocation() when data_directory is
>> NULL. (As the comment in ProcessConfigFile says.)
>
> This problem occurs because we don't have the value of data_directory
> set in postgresql.conf by the time we want to parse .auto.conf file
> during server start. The value of data_directory is only available after
> processing of config files. To fix it, we need to store the value of
> data_directory during parse of postgresql.conf file so that we can use it
> till data_directory is actually set. Attached patch fixes the problem.
> Could you please once confirm if it fixes the problem in your
> env./scenario.

Maybe this is nitpicking, but what happens when postgresql.auto.conf also
includes the setting of data_directory? This is possible because we can
set data_directory via ALTER SYSTEM now. Should we just ignore such
problematic setting in postgresql.auto.conf with warning message?

Regards,

--
Fujii Masao

On Thu, May 8, 2014 at 6:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Wed, May 7, 2014 at 4:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> This problem occurs because we don't have the value of data_directory
>> set in postgresql.conf by the time we want to parse .auto.conf file
>> during server start. The value of data_directory is only available after
>> processing of config files. To fix it, we need to store the value of
>> data_directory during parse of postgresql.conf file so that we can use it
>> till data_directory is actually set. Attached patch fixes the problem.
>> Could you please once confirm if it fixes the problem in your
>> env./scenario.
>
> Maybe this is nitpicking, but what happens when postgresql.auto.conf also
> includes the setting of data_directory? This is possible because we can
> set data_directory via ALTER SYSTEM now.

Yes, that will also be issue similar to above.

> Should we just ignore such
> problematic setting in postgresql.auto.conf with warning message?

Another way could be that we detect the same during server start
(while parsing postgresql.auto.conf) and then allow for reparse of
auto conf file, but I think that will be bit more complicated. So lets
try to solve it in a way suggested by you. If there is no objection by
anyone else then I will update the patch.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

* Amit Kapila (amit(dot)kapila16(at)gmail(dot)com) wrote:
> On Thu, May 8, 2014 at 6:51 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > Should we just ignore such
> > problematic setting in postgresql.auto.conf with warning message?
>
> Another way could be that we detect the same during server start
> (while parsing postgresql.auto.conf) and then allow for reparse of
> auto conf file, but I think that will be bit more complicated. So lets
> try to solve it in a way suggested by you. If there is no objection by
> anyone else then I will update the patch.

Pretty sure this is more-or-less exactly what I suggested quite a while
back during the massive ALTER SYSTEM SET thread.. There are certain
options which we shouldn't be allowing in .auto.conf. I doubt this is
going to be the only one...

Thanks,

Stephen

On 2014-05-08 22:21:43 +0900, Fujii Masao wrote:
> On Wed, May 7, 2014 at 4:57 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
> >> Hi,
> >>
> >> if you split configuration and data by setting data_directory,
> >> postgresql.auto.conf is writen to the data directory
> >> (/var/lib/postgresql/9.4/main in Debian), but tried to be read from
> >> the etc directory (/etc/postgresql/9.4/main).
> >>
> >> One place to fix it would be in ProcessConfigFile in
> >> src/backend/utils/misc/guc-file.l:162 by always setting
> >> CallingFileName = NULL in src/backend/utils/misc/guc-file.l:162, but
> >> that breaks later in AbsoluteConfigLocation() when data_directory is
> >> NULL. (As the comment in ProcessConfigFile says.)
> >
> > This problem occurs because we don't have the value of data_directory
> > set in postgresql.conf by the time we want to parse .auto.conf file
> > during server start. The value of data_directory is only available after
> > processing of config files. To fix it, we need to store the value of
> > data_directory during parse of postgresql.conf file so that we can use it
> > till data_directory is actually set. Attached patch fixes the problem.
> > Could you please once confirm if it fixes the problem in your
> > env./scenario.
>
> Maybe this is nitpicking, but what happens when postgresql.auto.conf also
> includes the setting of data_directory? This is possible because we can
> set data_directory via ALTER SYSTEM now. Should we just ignore such
> problematic setting in postgresql.auto.conf with warning message?

I think that's a case of "Doctor, it hurts when I do this. Doctor: don't
do that then".

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Re: Andres Freund 2014-05-08 <20140508145901(dot)GB1703(at)awork2(dot)anarazel(dot)de>
> > Maybe this is nitpicking, but what happens when postgresql.auto.conf also
> > includes the setting of data_directory? This is possible because we can
> > set data_directory via ALTER SYSTEM now. Should we just ignore such
> > problematic setting in postgresql.auto.conf with warning message?
>
> I think that's a case of "Doctor, it hurts when I do this. Doctor: don't
> do that then".

I'd opt to forbid setting data_directory at ALTER SYSTEM time. For the
other options, I agree with Andres that you should get to keep all
parts if you manage to break it.

Fortunately, ALTER SYSTEM already refuses to change config_file :)

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Thu, May 8, 2014 at 9:47 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
> Re: Andres Freund 2014-05-08 <20140508145901(dot)GB1703(at)awork2(dot)anarazel(dot)de>
>> > Maybe this is nitpicking, but what happens when postgresql.auto.conf also
>> > includes the setting of data_directory? This is possible because we can
>> > set data_directory via ALTER SYSTEM now. Should we just ignore such
>> > problematic setting in postgresql.auto.conf with warning message?
>>
>> I think that's a case of "Doctor, it hurts when I do this. Doctor: don't
>> do that then".
>
> I'd opt to forbid setting data_directory at ALTER SYSTEM time. For the
> other options, I agree with Andres that you should get to keep all
> parts if you manage to break it.

There is no harm in forbidding data_directory, but similarly we can
imagine that people can set some very large values for some config
variables due to which later it can have symptoms similar to this
issue.

> Fortunately, ALTER SYSTEM already refuses to change config_file :)

That is because config_file is not a file parameter (not present in
postgresql.conf). We disallow non-file and internal (like wal_segment_size)
parameters in Alter System

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Thu, May 8, 2014 at 9:47 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>> Re: Andres Freund 2014-05-08 <20140508145901(dot)GB1703(at)awork2(dot)anarazel(dot)de>
>>> > Maybe this is nitpicking, but what happens when postgresql.auto.conf also
>>> > includes the setting of data_directory? This is possible because we can
>>> > set data_directory via ALTER SYSTEM now. Should we just ignore such
>>> > problematic setting in postgresql.auto.conf with warning message?
>>>
>>> I think that's a case of "Doctor, it hurts when I do this. Doctor: don't
>>> do that then".
>>
>> I'd opt to forbid setting data_directory at ALTER SYSTEM time. For the
>> other options, I agree with Andres that you should get to keep all
>> parts if you manage to break it.
>
> There is no harm in forbidding data_directory, but similarly we can
> imagine that people can set some very large values for some config
> variables due to which later it can have symptoms similar to this
> issue.

Yes, that can prevent the server from restarting at all. In this case,
to restart the server, we need to edit postgresql.auto.conf manually
and remove the problematic settings though the header of
postgresql.auto.conf warns "Do not edit this file manually!".

Regards,

--
Fujii Masao

On Fri, May 9, 2014 at 7:01 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> There is no harm in forbidding data_directory, but similarly we can
>> imagine that people can set some very large values for some config
>> variables due to which later it can have symptoms similar to this
>> issue.
>
> Yes, that can prevent the server from restarting at all. In this case,
> to restart the server, we need to edit postgresql.auto.conf manually
> and remove the problematic settings though the header of
> postgresql.auto.conf warns "Do not edit this file manually!".

So lets not try to forbid data_directory in postgresql.auto.conf and just
fix the case reported in this mail for postgresql.conf for which the
patch is attached upthread.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sat, May 10, 2014 at 12:30 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Fri, May 9, 2014 at 7:01 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>>> There is no harm in forbidding data_directory, but similarly we can
>>> imagine that people can set some very large values for some config
>>> variables due to which later it can have symptoms similar to this
>>> issue.
>>
>> Yes, that can prevent the server from restarting at all. In this case,
>> to restart the server, we need to edit postgresql.auto.conf manually
>> and remove the problematic settings though the header of
>> postgresql.auto.conf warns "Do not edit this file manually!".
>
> So lets not try to forbid data_directory in postgresql.auto.conf and just
> fix the case reported in this mail for postgresql.conf for which the
> patch is attached upthread.

ISTM that data_directory is in different situation from the other. That is,
setting data_directory in postgresql.auto.conf is problematic whether its
setting value is valid or invalid. Imagine the case where data_directory
is set to '/data1' and '/data2' in config_directory/postgresql.conf and
/data1/postgresql.auto.conf, respectively. In this case, firstly the server
doesn't read /data2/postgresql.auto.conf when it starts up, even if your
patch has been applied. Secondly, the server doesn't read
/data1/postgresql.auto.conf when it receives SIGHUP, and this causes
the following error.

LOG: parameter "data_directory" cannot be changed without restarting the server

Regards,

--
Fujii Masao

On Sun, May 11, 2014 at 4:38 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> ISTM that data_directory is in different situation from the other. That is,
> setting data_directory in postgresql.auto.conf is problematic whether its
> setting value is valid or invalid. Imagine the case where data_directory
> is set to '/data1' and '/data2' in config_directory/postgresql.conf and
> /data1/postgresql.auto.conf, respectively. In this case, firstly the server
> doesn't read /data2/postgresql.auto.conf when it starts up, even if your
> patch has been applied.

I think after my patch, first server will read data1/postgresql.auto.conf
on start as that is what is set in postgresql.conf.

> Secondly, the server doesn't read
> /data1/postgresql.auto.conf when it receives SIGHUP, and this causes
> the following error.
>
> LOG: parameter "data_directory" cannot be changed without restarting the server

This happens because data_directory is PGC_POSTMASTER parameter
and none of such parameters can be changed with SIGHUP.

In above scenario, I think you are expecting it should use
/data2/postgresql.auto.conf and that is what you have mentioned
up-thread. The way to handle it by server is just to forbid setting
this parameter
by Alter System or the user himself should not perform such an action.
Here if we want user to be careful of performing such an action, then may
be it's better to have such an indication in ALTER SYSTEM documentation.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> In above scenario, I think you are expecting it should use
> /data2/postgresql.auto.conf and that is what you have mentioned
> up-thread. The way to handle it by server is just to forbid setting
> this parameter
> by Alter System or the user himself should not perform such an action.
> Here if we want user to be careful of performing such an action, then may
> be it's better to have such an indication in ALTER SYSTEM documentation.

I think it's clearly *necessary* to forbid setting data_directory in
postgresql.auto.conf. The file is defined to be found in the data
directory, so any such setting is circular logic by definition;
no good can come of not rejecting it.

We already have a GUC flag bit about disallowing certain variables
in the config file (though I don't remember if it's enforced or
just advisory). It seems to me that we'd better invent one for
disallowing in ALTER SYSTEM, as well.

regards, tom lane

Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> writes:
> This problem occurs because we don't have the value of data_directory
> set in postgresql.conf by the time we want to parse .auto.conf file
> during server start. The value of data_directory is only available after
> processing of config files. To fix it, we need to store the value of
> data_directory during parse of postgresql.conf file so that we can use it
> till data_directory is actually set. Attached patch fixes the problem.

Aside from being about as ugly as it could possibly be, this is wrong,
because it only covers one of the possible sources of a data_directory
that's different from the config file location. In particular, it's
possible to set --config_file=something on the postmaster command line
and also set a data directory different than that via -D or a PGDATA
environment variable, rather than an entry in postgresql.conf (see the
initial logic in SelectConfigFiles).

While it's possible that you could deal with that with some even uglier
variant on this patch (perhaps involving making SelectConfigFiles export
its internal value of configdir), I think that having ProcessConfigFile
understand exactly what SelectConfigFiles is going to do to select the
final value of data_directory would be a clear modularity violation,
and very fragile as well.

I think what probably has to happen is that ProcessConfigFile shouldn't
be internally responsible for reading the auto file at all, but that we
do that via two separate calls to ProcessConfigFile, one for the main
file and then one for the auto file; and during initial startup,
SelectConfigFiles doesn't make the call for the auto file until after
it's established the final value of data_directory. (And by the by,
I think that DataDir not data_directory is what to be using to compute
the auto file's path.)

It's distressing that this wasn't noticed till now; it shows that nobody
tested ALTER SYSTEM with a config file outside the data directory, which
seems like a rather fundamental omission for any work with GUC files.

regards, tom lane

I wrote:
> I think what probably has to happen is that ProcessConfigFile shouldn't
> be internally responsible for reading the auto file at all, but that we
> do that via two separate calls to ProcessConfigFile, one for the main
> file and then one for the auto file; and during initial startup,
> SelectConfigFiles doesn't make the call for the auto file until after
> it's established the final value of data_directory.

Since this bug would block testing of ALTER SYSTEM by a nontrivial
population of users, I felt it was important to get it fixed before beta,
so I went to try and fix it as above. It turns out that reading the two
config files separately doesn't work because ProcessConfigFile will think
all the settings got removed from the file. What we can do for the
moment, though, is to just run ProcessConfigFile twice during startup,
skipping the auto file the first time. It might be worth refactoring
ProcessConfigFile to avoid the overhead of reading postgresql.conf twice,
but it would be a lot of work and I think the gain would be marginal; in
any case there's not time to do that today. But I've got the main problem
fixed in time for beta.

regards, tom lane

On Sun, May 11, 2014 at 3:20 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
>> I think what probably has to happen is that ProcessConfigFile shouldn't
>> be internally responsible for reading the auto file at all, but that we
>> do that via two separate calls to ProcessConfigFile, one for the main
>> file and then one for the auto file; and during initial startup,
>> SelectConfigFiles doesn't make the call for the auto file until after
>> it's established the final value of data_directory.
>
> Since this bug would block testing of ALTER SYSTEM by a nontrivial
> population of users, I felt it was important to get it fixed before beta,
> so I went to try and fix it as above. It turns out that reading the two
> config files separately doesn't work because ProcessConfigFile will think
> all the settings got removed from the file. What we can do for the
> moment, though, is to just run ProcessConfigFile twice during startup,
> skipping the auto file the first time. It might be worth refactoring
> ProcessConfigFile to avoid the overhead of reading postgresql.conf twice,
> but it would be a lot of work and I think the gain would be marginal; in
> any case there's not time to do that today. But I've got the main problem
> fixed in time for beta.

Thanks for dealing with this. I was skeptical of the proposed patch,
but didn't have time to think hard enough about it to say something
intelligent. I'm glad you did.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Mon, May 12, 2014 at 12:50 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Since this bug would block testing of ALTER SYSTEM by a nontrivial
> population of users, I felt it was important to get it fixed before beta,
> so I went to try and fix it as above. It turns out that reading the two
> config files separately doesn't work because ProcessConfigFile will think
> all the settings got removed from the file. What we can do for the
> moment, though, is to just run ProcessConfigFile twice during startup,
> skipping the auto file the first time. It might be worth refactoring
> ProcessConfigFile to avoid the overhead of reading postgresql.conf twice,
> but it would be a lot of work and I think the gain would be marginal; in
> any case there's not time to do that today. But I've got the main problem
> fixed in time for beta.

Thanks for fixing it.
I will provide a fix to forbid data_directory via Alter System as suggested
by you upthread.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Sun, May 11, 2014 at 11:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I think it's clearly *necessary* to forbid setting data_directory in
> postgresql.auto.conf. The file is defined to be found in the data
> directory, so any such setting is circular logic by definition;
> no good can come of not rejecting it.
>
> We already have a GUC flag bit about disallowing certain variables
> in the config file (though I don't remember if it's enforced or
> just advisory). It seems to me that we'd better invent one for
> disallowing in ALTER SYSTEM, as well.

I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to
disallow parameters by Alter System and disallowed data_directory to
be set by Alter System.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 27, 2014 at 10:35 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> On Sun, May 11, 2014 at 11:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > I think it's clearly *necessary* to forbid setting data_directory in
> > postgresql.auto.conf. The file is defined to be found in the data
> > directory, so any such setting is circular logic by definition;
> > no good can come of not rejecting it.
> >
> > We already have a GUC flag bit about disallowing certain variables
> > in the config file (though I don't remember if it's enforced or
> > just advisory). It seems to me that we'd better invent one for
> > disallowing in ALTER SYSTEM, as well.
>
> I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to
> disallow parameters by Alter System and disallowed data_directory to
> be set by Alter System.

I have added this patch in next CF to avoid getting it lost.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, May 27, 2014 at 2:05 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sun, May 11, 2014 at 11:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> I think it's clearly *necessary* to forbid setting data_directory in
>> postgresql.auto.conf. The file is defined to be found in the data
>> directory, so any such setting is circular logic by definition;
>> no good can come of not rejecting it.
>>
>> We already have a GUC flag bit about disallowing certain variables
>> in the config file (though I don't remember if it's enforced or
>> just advisory). It seems to me that we'd better invent one for
>> disallowing in ALTER SYSTEM, as well.
>
> I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to
> disallow parameters by Alter System and disallowed data_directory to
> be set by Alter System.

We should document what types of parameters are not allowed to be set by
ALTER SYSTEM SET?

data_directory was displayed when I typed "TAB" just after ALTER SYSTEM SET.
Probably tab-completion for ALTER SYSTEM SET needs to be changed.

Regards,

--
Fujii Masao

On Thu, Jun 12, 2014 at 7:26 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Tue, May 27, 2014 at 2:05 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:
> > On Sun, May 11, 2014 at 11:23 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> I think it's clearly *necessary* to forbid setting data_directory in
> >> postgresql.auto.conf. The file is defined to be found in the data
> >> directory, so any such setting is circular logic by definition;
> >> no good can come of not rejecting it.
> >>
> >> We already have a GUC flag bit about disallowing certain variables
> >> in the config file (though I don't remember if it's enforced or
> >> just advisory). It seems to me that we'd better invent one for
> >> disallowing in ALTER SYSTEM, as well.
> >
> > I introduced a new flag bit (GUC_DISALLOW_IN_AUTO_FILE) to
> > disallow parameters by Alter System and disallowed data_directory to
> > be set by Alter System.
>
> We should document what types of parameters are not allowed to be set by
> ALTER SYSTEM SET?

Agreed, I had mentioned in Notes section of document. Apart from that
I had disallowed parameters that are excluded from postgresql.conf by
initdb (Developer options) and they are recommended in user manual
to be not used in production.

> data_directory was displayed when I typed "TAB" just after ALTER SYSTEM
SET.
> Probably tab-completion for ALTER SYSTEM SET needs to be changed.

This information is not stored in pg_settings. One way is to specify
manually all the parameters which are disallowed but it seems the query
will become clumsy, another could be to have another column in pg_settings.
Do you think of any other way?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Amit Kapila 2014-06-13 <CAA4eK1KLn1SmgVtd=5emAbQxrrPVeEdTBUU94E-rEPMwxWVL+A(at)mail(dot)gmail(dot)com>
> Agreed, I had mentioned in Notes section of document. Apart from that
> I had disallowed parameters that are excluded from postgresql.conf by
> initdb (Developer options) and they are recommended in user manual
> to be not used in production.

Excluding developer options seems too excessive to me. ALTER SYSTEM
should be useful for developing too.

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Sun, Jun 15, 2014 at 6:29 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>
> Re: Amit Kapila 2014-06-13 <CAA4eK1KLn1SmgVtd=
5emAbQxrrPVeEdTBUU94E-rEPMwxWVL+A(at)mail(dot)gmail(dot)com>
> > Agreed, I had mentioned in Notes section of document. Apart from that
> > I had disallowed parameters that are excluded from postgresql.conf by
> > initdb (Developer options) and they are recommended in user manual
> > to be not used in production.
>
> Excluding developer options seems too excessive to me. ALTER SYSTEM
> should be useful for developing too.

Developer options are mainly for debugging information or might help in one
of the situations, so I thought somebody might not want them to be part of
server configuration once they are set. We already disallow parameters like
config_file, transaction_isolation, etc. which are disallowed to be set in
postgresql.conf. Could you please explain me a bit in which
situations/scenarios, do you think that allowing developer options via Alter
System can be helpful?

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Re: Amit Kapila 2014-06-16 <CAA4eK1++wcFswhzH=92qiQFB=C0_Uy=mReadvHZXdrF3JOWX6A(at)mail(dot)gmail(dot)com>
> Developer options are mainly for debugging information or might help in one
> of the situations, so I thought somebody might not want them to be part of
> server configuration once they are set. We already disallow parameters like
> config_file, transaction_isolation, etc. which are disallowed to be set in
> postgresql.conf. Could you please explain me a bit in which
> situations/scenarios, do you think that allowing developer options via Alter
> System can be helpful?

Oh if these are disallowed in postgresql.conf, they should be
disallowed in auto.conf too. I meant to say that there shouldn't be
additional restrictions for auto.conf, except for cases like
data_directory which won't work at all (or are highly confusing).

Christoph
--
cb(at)df7cb(dot)de | http://www.df7cb.de/

On Mon, Jun 16, 2014 at 12:14 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> On Sun, Jun 15, 2014 at 6:29 PM, Christoph Berg <cb(at)df7cb(dot)de> wrote:
>>
>> Re: Amit Kapila 2014-06-13
>> <CAA4eK1KLn1SmgVtd=5emAbQxrrPVeEdTBUU94E-rEPMwxWVL+A(at)mail(dot)gmail(dot)com>
>> > Agreed, I had mentioned in Notes section of document. Apart from that
>> > I had disallowed parameters that are excluded from postgresql.conf by
>> > initdb (Developer options) and they are recommended in user manual
>> > to be not used in production.
>>
>> Excluding developer options seems too excessive to me. ALTER SYSTEM
>> should be useful for developing too.
>
> Developer options are mainly for debugging information or might help in one
> of the situations, so I thought somebody might not want them to be part of
> server configuration once they are set. We already disallow parameters like
> config_file, transaction_isolation, etc. which are disallowed to be set in
> postgresql.conf. Could you please explain me a bit in which
> situations/scenarios, do you think that allowing developer options via Alter
> System can be helpful?

I think that's helpful. I've heard that some users enable the developer option
"trace_sort" in postgresql.conf in order to collect the information
about sorting.
They might want to set trace_sort via ALTER SYSTEM, for example.

> This information is not stored in pg_settings. One way is to specify
> manually all the parameters which are disallowed but it seems the query
> will become clumsy, another could be to have another column in pg_settings.
> Do you think of any other way?

I like the latter idea, i.e., exposing the flags of GUC parameters in
pg_settings,
but it seems overkill just for tab-completion purpose. I'd withdraw my comment.

Regards,

--
Fujii Masao

Hi.

Just a few minor comments about your patch:

At 2014-06-13 11:46:21 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> + <title>Notes</title>
> +
> + <para>
> + This command will not allow to set parameters that are disallowed or
> + excluded in postgresql.conf. It also disallows to set configuration
> + parameter <xref linkend="guc-data-directory">.
> + </para>
> + </refsect1>

I suggest the following wording:

This command may not be used to set
<xref linkend="guc-data-directory">
or any parameters that are not allowed in postgresql.conf.

> + /*
> + * Disallow parameter's that are excluded or disallowed in
> + * postgresql.conf.
> + */

"parameters", no apostrophe.

> if ((record->context == PGC_INTERNAL) ||
> - (record->flags & GUC_DISALLOW_IN_FILE))
> - ereport(ERROR,
> - (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
> - errmsg("parameter \"%s\" cannot be changed",
> - name)));
> + (record->flags & GUC_DISALLOW_IN_FILE) ||
> + (record->flags & GUC_DISALLOW_IN_AUTO_FILE) ||
> + (record->flags & GUC_NOT_IN_SAMPLE))
> + ereport(ERROR,
> + (errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
> + errmsg("parameter \"%s\" cannot be changed",
> + name)));

I looked at the settings that are marked GUC_NOT_IN_SAMPLE but neither
PGC_INTERNAL nor GUC_DISALLOW_IN_*FILE. I don't feel strongly about it,
but I don't see any particularly good reason to exclude them here.

(I also agree with Fujii-san that it isn't worth making extensive
changes to avoid data_directory being offered via tab-completion.)

-- Abhijit

On Mon, Jun 16, 2014 at 7:06 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> > Developer options are mainly for debugging information or might help in
one
> > of the situations, so I thought somebody might not want them to be part
of
> > server configuration once they are set. We already disallow parameters
like
> > config_file, transaction_isolation, etc. which are disallowed to be set
in
> > postgresql.conf. Could you please explain me a bit in which
> > situations/scenarios, do you think that allowing developer options via
Alter
> > System can be helpful?
>
> I think that's helpful. I've heard that some users enable the developer
option
> "trace_sort" in postgresql.conf in order to collect the information
> about sorting.
> They might want to set trace_sort via ALTER SYSTEM, for example.

Okay, if you have usecase where people use such parameters in
postegresql.conf, then it makes sense. I have removed that restriction
from patch.

> I suggest the following wording:
>
> This command may not be used to set
> <xref linkend="guc-data-directory">
> or any parameters that are not allowed in postgresql.conf.

I think we should not use *may* in this line, because in no
circumstance we will allow this command for the parameters
mentioned. I have changed it as per your suggestion.

> > + /*
> > + * Disallow parameter's that are excluded or disallowed in
> > + * postgresql.conf.
> > + */
>
> "parameters", no apostrophe.
okay.
I have changed above comment as earlier comment has
information about developer option which we don't need now.

> > if ((record->context == PGC_INTERNAL) ||
> > - (record->flags & GUC_DISALLOW_IN_FILE))
> > - ereport(ERROR,
> > -
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
> > - errmsg("parameter \"%s\" cannot be
changed",
> > - name)));
> > + (record->flags & GUC_DISALLOW_IN_FILE) ||
> > + (record->flags & GUC_DISALLOW_IN_AUTO_FILE) ||
> > + (record->flags & GUC_NOT_IN_SAMPLE))
> > + ereport(ERROR,
> > +
(errcode(ERRCODE_CANT_CHANGE_RUNTIME_PARAM),
> > + errmsg("parameter \"%s\" cannot be
changed",
> > + name)));
>
> I looked at the settings that are marked GUC_NOT_IN_SAMPLE but neither
> PGC_INTERNAL nor GUC_DISALLOW_IN_*FILE. I don't feel strongly about it,
> but I don't see any particularly good reason to exclude them here.

By above do you mean that the patch should allow GUC_NOT_IN_SAMPLE or
something else, if earlier then I have removed it as per comment from
Fujji-san.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

At 2014-06-17 09:49:35 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>
> By above do you mean that the patch should allow GUC_NOT_IN_SAMPLE or
> something else, if earlier then I have removed it as per comment from
> Fujji-san.

Yes, what you've done in v3 of the patch is what I meant. I've marked
this as ready for committer now.

-- Abhijit

On Tue, Jun 17, 2014 at 2:08 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> At 2014-06-17 09:49:35 +0530, amit(dot)kapila16(at)gmail(dot)com wrote:
>>
>> By above do you mean that the patch should allow GUC_NOT_IN_SAMPLE or
>> something else, if earlier then I have removed it as per comment from
>> Fujji-san.
>
> Yes, what you've done in v3 of the patch is what I meant. I've marked
> this as ready for committer now.

Barring objections, I will commit this patch.

Also I'm thinking to backpatch this to 9.4 because this is a bugfix rather
than new feature.

Regards,

--
Fujii Masao

At 2014-06-18 12:55:36 +0900, masao(dot)fujii(at)gmail(dot)com wrote:
>
> Also I'm thinking to backpatch this to 9.4 because this is a bugfix
> rather than new feature.

That sounds reasonable, thanks.

-- Abhijit

On Wed, Jun 18, 2014 at 1:07 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com> wrote:
> At 2014-06-18 12:55:36 +0900, masao(dot)fujii(at)gmail(dot)com wrote:
>>
>> Also I'm thinking to backpatch this to 9.4 because this is a bugfix
>> rather than new feature.
>
> That sounds reasonable, thanks.

Committed!

Regards,

--
Fujii Masao

On Thu, Jun 19, 2014 at 5:21 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> On Wed, Jun 18, 2014 at 1:07 PM, Abhijit Menon-Sen <ams(at)2ndquadrant(dot)com>
wrote:
> > At 2014-06-18 12:55:36 +0900, masao(dot)fujii(at)gmail(dot)com wrote:
> >>
> >> Also I'm thinking to backpatch this to 9.4 because this is a bugfix
> >> rather than new feature.
> >
> > That sounds reasonable, thanks.
>
> Committed!

Thank you.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com