On Sun, Aug 10, 2014 at 3:54 PM, Amit Kapila <amit.kapil...@gmail.com> wrote:
> 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:
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
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***************
*** 45,50 **** static unsigned int ConfigFileLineno;
--- 45,52 ----
static const char *GUC_flex_fatal_errmsg;
static sigjmp_buf *GUC_flex_fatal_jmp;
+ static void FreeConfigVariable(ConfigVariable *item);
+
/* flex fails to supply a prototype for yylex, so provide one */
int GUC_yylex(void);
***************
*** 151,164 **** ProcessConfigFile(GucContext context)
* file is in the data directory, we can't read it until the DataDir has
* been set.
*/
! if (DataDir &&
! !ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
! &head, &tail))
{
! /* Syntax error(s) detected in the file, so bail out */
! error = true;
! ErrorConfFile = PG_AUTOCONF_FILENAME;
! goto cleanup_list;
}
/*
--- 153,218 ----
* file is in the data directory, we can't read it until the DataDir has
* been set.
*/
! if (DataDir)
{
! if (!ParseConfigFile(PG_AUTOCONF_FILENAME, NULL, false, 0, elevel,
! &head, &tail))
! {
! /* Syntax error(s) detected in the file, so bail out */
! error = true;
! ErrorConfFile = PG_AUTOCONF_FILENAME;
! goto cleanup_list;
! }
! }
! /*
! * Pick up only the data_directory if DataDir is not set, which
! * means that the configuration file is read for the first time and
! * PG_AUTOCONF_FILENAME file cannot be read yet. 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. OTOH, since it's ensured that data_directory doesn't
! * exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
! * later.
! */
! else
! {
! ConfigVariable *prev = NULL;
!
! for (item = head; item;)
! {
! ConfigVariable *ptr = item;
!
! item = item->next;
! if (strcmp(ptr->name, "data_directory") != 0)
! {
! if (prev == NULL)
! head = ptr->next;
! else
! {
! prev->next = ptr->next;
! /*
! * On removing last item in list, we need to update tail
! * to ensure that list will be maintianed.
! */
! if (prev->next == NULL)
! tail = prev;
! }
! FreeConfigVariable(ptr);
! }
! else
! prev = ptr;
! }
!
! /*
! * Quick exit if data_directory is not present in list.
! *
! * Don't remember when we last successfully loaded the config file in
! * this case because that time will be set soon by subsequent load of
! * the config file.
! */
! if (head == NULL)
! return;
}
/*
***************
*** 677,683 **** ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
*tail_p = prev_item;
}
! pfree(cur_item);
break;
}
}
--- 731,737 ----
*tail_p = prev_item;
}
! FreeConfigVariable(cur_item);
break;
}
}
***************
*** 858,863 **** cleanup:
--- 912,932 ----
}
/*
+ * Free a ConfigVariable
+ */
+ static void
+ FreeConfigVariable(ConfigVariable *item)
+ {
+ if (item != NULL)
+ {
+ pfree(item->name);
+ pfree(item->value);
+ pfree(item->filename);
+ pfree(item);
+ }
+ }
+
+ /*
* Free a list of ConfigVariables, including the names and the values
*/
void
***************
*** 870,879 **** FreeConfigVariables(ConfigVariable *list)
{
ConfigVariable *next = item->next;
! pfree(item->name);
! pfree(item->value);
! pfree(item->filename);
! pfree(item);
item = next;
}
}
--- 939,945 ----
{
ConfigVariable *next = item->next;
! FreeConfigVariable(item);
item = next;
}
}
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***************
*** 4339,4344 **** SelectConfigFiles(const char *userDoption, const char *progname)
--- 4339,4351 ----
return false;
}
+ /*
+ * 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. Then don't
+ * forget to read the configuration file again later to pick up all the
+ * parameters.
+ */
ProcessConfigFile(PGC_POSTMASTER);
/*
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
