On Fri, Aug 8, 2014 at 11:41 AM, Fujii Masao <masao.fu...@gmail.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
