Re: postgresql.auto.conf and reload

On Sun, Aug 10, 2014 at 3:54 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> 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:

Thanks for the review! Attached is the updated version of the 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))

Done.

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

Done.

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

Done.

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

Okay, I just remove that line.

While updating the patch, I found that the ConfigVariable which
is removed from list has the fields that the memory has been already
allocated but we forgot to free them. So I included the code to free
them in the patch.

Regards,

--
Fujii Masao