postgresql.auto.conf read from wrong directory

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb@df7cb.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb@df7cb.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@mail.gmail.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@df7cb.de | http://www.df7cb.de/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Wed, May 7, 2014 at 4:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb@df7cb.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 8, 2014 at 6:51 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Wed, May 7, 2014 at 4:57 PM, Amit Kapila <amit.kapila16@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

* Amit Kapila (amit.kapila16@gmail.com) wrote:

On Thu, May 8, 2014 at 6:51 PM, Fujii Masao <masao.fujii@gmail.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.kapila16@gmail.com> wrote:

On Tue, May 6, 2014 at 7:04 PM, Christoph Berg <cb@df7cb.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: Andres Freund 2014-05-08 <20140508145901.GB1703@awork2.anarazel.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@df7cb.de | http://www.df7cb.de/

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Thu, May 8, 2014 at 9:47 PM, Christoph Berg <cb@df7cb.de> wrote:

Re: Andres Freund 2014-05-08 <20140508145901.GB1703@awork2.anarazel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Thu, May 8, 2014 at 9:47 PM, Christoph Berg <cb@df7cb.de> wrote:

Re: Andres Freund 2014-05-08 <20140508145901.GB1703@awork2.anarazel.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Fri, May 9, 2014 at 7:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit.kapila16@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sat, May 10, 2014 at 12:30 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

On Fri, May 9, 2014 at 7:01 PM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Fri, May 9, 2014 at 1:06 PM, Amit Kapila <amit.kapila16@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, May 11, 2014 at 4:38 AM, Fujii Masao <masao.fujii@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Kapila <amit.kapila16@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Amit Kapila <amit.kapila16@gmail.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, May 11, 2014 at 3:20 PM, Tom Lane <tgl@sss.pgh.pa.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Mon, May 12, 2014 at 12:50 AM, Tom Lane <tgl@sss.pgh.pa.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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

On Sun, May 11, 2014 at 11:23 PM, Tom Lane <tgl@sss.pgh.pa.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