Re: postgresql.auto.conf and reload

On Fri, Aug 8, 2014 at 11:41 AM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>
> Yep, right. ParseConfigFp() is not good place to pick up data_directory.
> What about the attached patch which makes ProcessConfigFile() instead of
> ParseConfigFp() pick up data_directory just after the configuration file
is
> parsed?

I think this is better way to handle it. Few comments about patch:

1. can't we directly have the code by having else in below loop.
if (DataDir &&
!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
&head, &tail))

2.
+ if (DataDir == NULL)
+ {
+ ConfigVariable *prev = NULL;
+ for (item = head; item;)
+ {
..
..
+ }

If data_directory is not present in list, then can't we directly return
and leave the other work in function ProcessConfigFile() for second
pass.

3.
I think comments can be improved:
a.
+ In this case,
+ * we should not pick up all the settings except the data_directory
+ * from postgresql.conf yet because they might be overwritten
+ * with the settings in PG_AUTOCONF_FILENAME file which will be
+ * read later.

It would be better if we change it as below:

In this case, we shouldn't pick any settings except the data_directory
from postgresql.conf because they might be overwritten
with the settings in PG_AUTOCONF_FILENAME file which will be
read later.

b.
+Now only the data_directory needs to be picked up to
+ * read PG_AUTOCONF_FILENAME file later.

This part of comment seems to be repetitive as you already mentioned
some part of it in first line as well as at one other location:

+ * Pick up only the data_directory if DataDir is not set,

+ /*
+ * Read the configuration file for the first time. This time only
+ * data_directory parameter is picked up to determine the data directory
+ * so that we can read PG_AUTOCONF_FILENAME file next time.

Please see if you can improve it.

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