Re: [HACKERS] postgresql.auto.conf and reload
On Tue, Aug 12, 2014 at 1:23 PM, Amit Kapila amit.kapil...@gmail.com wrote:
On Mon, Aug 11, 2014 at 11:08 PM, Fujii Masao masao.fu...@gmail.com wrote:
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.
Yes, that is right.
! /*
! * 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
It is bit incovinient to read this part of code, some other places in
same file use comment inside else construct which seems to be
better. one example is as below:
Yep, updated that way.
else
{
/*
* ordinary variable, append to list. For multiple items of
* same parameter, retain only which comes later.
*/
I have marked this as Ready For Committer.
Thanks for the review! Committed.
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
Re: [HACKERS] postgresql.auto.conf and reload
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;
Re: [HACKERS] postgresql.auto.conf and reload
On Mon, Aug 11, 2014 at 11:08 PM, Fujii Masao masao.fu...@gmail.com wrote:
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.
Yes, that is right.
! /*
! * 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
It is bit incovinient to read this part of code, some other places in
same file use comment inside else construct which seems to be
better. one example is as below:
else
{
/*
* ordinary variable, append to list. For multiple items of
* same parameter, retain only which comes later.
*/
I have marked this as Ready For Committer.
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
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
Re: [HACKERS] postgresql.auto.conf and reload
On Sun, Aug 10, 2014 at 12:24 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: 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)) I think for this we need to change the above condition a bit. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Fri, Aug 8, 2014 at 1:19 PM, Amit Kapila amit.kapil...@gmail.com wrote:
On Thu, Aug 7, 2014 at 12:36 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Thu, Aug 7, 2014 at 12:36 PM, Amit Kapila amit.kapil...@gmail.com
wrote:
On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao masao.fu...@gmail.com
wrote:
What about picking up only data_directory at the first pass?
I think that will workout and for that I think we might need to add
few checks during ProcessConfigFile. Do you want to address it
during parsing of config file or during setting of values?
I prefer during paring. The attached patch does that. In this patch,
the first call of ProcessConfigFile() picks up only data_directory. One
concern of this patch is that the logic is a bit ugly.
I think handling 'data_directory' in ParseConfigFp() looks bit out of
place as this API is used to parse other config files as well like
recovery.conf. I agree that for all other paths DataDir might be
already set due to which this new path will never be executed, still
from code maintenance point of view it would have been better if we
can find a way to handle it in a place where we are processing
the server's main config file (postgresql.conf).
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?
Regards,
--
Fujii Masao
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***
*** 162,167 ProcessConfigFile(GucContext context)
--- 162,209
}
/*
+ * 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 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. OTOH, since it's ensured that data_directory doesn't
+ * exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
+ * later. Now only the data_directory needs to be picked up to
+ * read PG_AUTOCONF_FILENAME file later.
+ */
+ if (DataDir == NULL)
+ {
+ 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;
+ }
+ pfree(ptr);
+ }
+ else
+ prev = ptr;
+ }
+ }
+
+ /*
* Mark all extant GUC variables as not present in the config file.
* We need this so that we can tell below which ones have been removed
* from the file since we last processed it.
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 4342,4347 SelectConfigFiles(const char *userDoption, const char *progname)
--- 4342,4354
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
Re: [HACKERS] postgresql.auto.conf and reload
On Thu, Aug 7, 2014 at 12:36 PM, Amit Kapila amit.kapil...@gmail.com wrote:
On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao masao.fu...@gmail.com wrote:
On Tue, Jul 29, 2014 at 9:26 PM, Amit Kapila amit.kapil...@gmail.com
wrote:
The reason is that during startup DataDir is not set by the time server
processes config file due to which we process .auto.conf file in second
pass. I think ideally it should ignore the invalid setting in such a
case
as it does during reload, however currently there is no good way to
process .auto.conf incase DataDir is not set, so we handle it
separately
in second pass.
What about picking up only data_directory at the first pass?
I think that will workout and for that I think we might need to add
few checks during ProcessConfigFile. Do you want to address it
during parsing of config file or during setting of values?
I prefer during paring. The attached patch does that. In this patch,
the first call of ProcessConfigFile() picks up only data_directory. One
concern of this patch is that the logic is a bit ugly. Do you have any
other better idea?
Regards,
--
Fujii Masao
*** a/src/backend/utils/misc/guc-file.l
--- b/src/backend/utils/misc/guc-file.l
***
*** 648,653 ParseConfigFp(FILE *fp, const char *config_file, int depth, int elevel,
--- 648,672
else
{
/*
+ * 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 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. OTOH, since it's ensured that data_directory doesn't
+ * exist in PG_AUTOCONF_FILENAME file, it will never be overwritten
+ * later. Now only the data_directory needs to be picked up to
+ * read PG_AUTOCONF_FILENAME file later.
+ */
+ if (DataDir == NULL strcmp(opt_name, data_directory) != 0)
+ {
+ if (token == 0)
+ break;
+ continue;
+ }
+
+ /*
* ordinary variable, append to list. For multiple items of
* same parameter, retain only which comes later.
*/
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
***
*** 4342,4347 SelectConfigFiles(const char *userDoption, const char *progname)
--- 4342,4354
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
Re: [HACKERS] postgresql.auto.conf and reload
On Thu, Aug 7, 2014 at 12:28 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Aug 6, 2014 at 11:39 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Aug 5, 2014 at 12:49 PM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? I don't find that to be a particularly good idea. In the first place, it introduces extra complication and a surprising difference in the behavior of different GUCs. In the second place, I thought part of the point of this patch was to suppress log messages complaining about invalid values that then weren't actually used for anything. That issue exists just as much for non-POSTMASTER variables. (IOW, value cannot be changed now is not the only log message we're trying to suppress.) Yep, sounds reasonable. This makes me think that we can suppress such invalid message of the parameters which are actually not used at not only conf file reload but also *postmaster startup*. That's another story, though. Anyway, barring any objection, I will commit Amit's patch. Applied the slightly-modified version! Thanks. There is a commitfest entry [1] for this patch, do you want some thing more to be addressed or shall we mark that as committed. [1] https://commitfest.postgresql.org/action/patch_view?id=1500 Yeah, let's mark this as committed because your patch has been committed and the originally-reported problem has been fixed. We are now discussing another patch, but I will add that as new CF entry. 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
Re: [HACKERS] postgresql.auto.conf and reload
On Thu, Aug 7, 2014 at 12:36 PM, Fujii Masao masao.fu...@gmail.com wrote: On Thu, Aug 7, 2014 at 12:36 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao masao.fu...@gmail.com wrote: What about picking up only data_directory at the first pass? I think that will workout and for that I think we might need to add few checks during ProcessConfigFile. Do you want to address it during parsing of config file or during setting of values? I prefer during paring. The attached patch does that. In this patch, the first call of ProcessConfigFile() picks up only data_directory. One concern of this patch is that the logic is a bit ugly. I think handling 'data_directory' in ParseConfigFp() looks bit out of place as this API is used to parse other config files as well like recovery.conf. I agree that for all other paths DataDir might be already set due to which this new path will never be executed, still from code maintenance point of view it would have been better if we can find a way to handle it in a place where we are processing the server's main config file (postgresql.conf). With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Tue, Aug 5, 2014 at 12:49 PM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? I don't find that to be a particularly good idea. In the first place, it introduces extra complication and a surprising difference in the behavior of different GUCs. In the second place, I thought part of the point of this patch was to suppress log messages complaining about invalid values that then weren't actually used for anything. That issue exists just as much for non-POSTMASTER variables. (IOW, value cannot be changed now is not the only log message we're trying to suppress.) Yep, sounds reasonable. This makes me think that we can suppress such invalid message of the parameters which are actually not used at not only conf file reload but also *postmaster startup*. That's another story, though. Anyway, barring any objection, I will commit Amit's patch. Applied the slightly-modified version! 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
Re: [HACKERS] postgresql.auto.conf and reload
On Tue, Jul 29, 2014 at 9:26 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Mon, Jul 28, 2014 at 11:33 PM, Fujii Masao masao.fu...@gmail.com wrote: There is other side effect on this patch. With the patch, when reloading the configurartion file, the server cannot warm an invalid setting value if it's not the last setting of the parameter. This may cause problematic situation as follows. 1. ALTER SYSTEM SET work_mem TO '1024kB'; 2. Reload the configuration file --- success 3. Then, a user accidentally adds work_mem = '2048KB' into postgresql.conf The setting value '2048KB' is invalid, and the unit should be 'kB' instead of 'KB'. 4. Reload the configuration file --- success The invalid setting is ignored because the setting of work_mem in postgresql.auto.conf is preferred. So a user cannot notice that postgresql.conf has an invalid setting. 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to start up because of such an invalid setting. When PostgreSQL starts up, the last setting is preferred. But all the settings are checked. The reason is that during startup DataDir is not set by the time server processes config file due to which we process .auto.conf file in second pass. I think ideally it should ignore the invalid setting in such a case as it does during reload, however currently there is no good way to process .auto.conf incase DataDir is not set, so we handle it separately in second pass. What about picking up only data_directory at the first pass? 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
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Aug 6, 2014 at 11:39 AM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Aug 5, 2014 at 12:49 PM, Fujii Masao masao.fu...@gmail.com wrote: On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? I don't find that to be a particularly good idea. In the first place, it introduces extra complication and a surprising difference in the behavior of different GUCs. In the second place, I thought part of the point of this patch was to suppress log messages complaining about invalid values that then weren't actually used for anything. That issue exists just as much for non-POSTMASTER variables. (IOW, value cannot be changed now is not the only log message we're trying to suppress.) Yep, sounds reasonable. This makes me think that we can suppress such invalid message of the parameters which are actually not used at not only conf file reload but also *postmaster startup*. That's another story, though. Anyway, barring any objection, I will commit Amit's patch. Applied the slightly-modified version! Thanks. There is a commitfest entry [1] for this patch, do you want some thing more to be addressed or shall we mark that as committed. [1] https://commitfest.postgresql.org/action/patch_view?id=1500 With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Aug 6, 2014 at 1:32 PM, Fujii Masao masao.fu...@gmail.com wrote: On Tue, Jul 29, 2014 at 9:26 PM, Amit Kapila amit.kapil...@gmail.com wrote: The reason is that during startup DataDir is not set by the time server processes config file due to which we process .auto.conf file in second pass. I think ideally it should ignore the invalid setting in such a case as it does during reload, however currently there is no good way to process .auto.conf incase DataDir is not set, so we handle it separately in second pass. What about picking up only data_directory at the first pass? I think that will workout and for that I think we might need to add few checks during ProcessConfigFile. Do you want to address it during parsing of config file or during setting of values? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Fujii Masao masao.fu...@gmail.com writes: The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? I don't find that to be a particularly good idea. In the first place, it introduces extra complication and a surprising difference in the behavior of different GUCs. In the second place, I thought part of the point of this patch was to suppress log messages complaining about invalid values that then weren't actually used for anything. That issue exists just as much for non-POSTMASTER variables. (IOW, value cannot be changed now is not the only log message we're trying to suppress.) 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
Re: [HACKERS] postgresql.auto.conf and reload
On Tue, Jul 29, 2014 at 3:33 AM, Josh Berkus j...@agliodbs.com wrote: On 07/28/2014 11:03 AM, Fujii Masao wrote: On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila amit.kapil...@gmail.com wrote: Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. ISTM that the patch works fine. Only concern is that the logic needs O(n^2) comparison, which may cause performance problem. But n in O(n^2) is the number of uncommented parameters and I don't think it's so big, ISTM I can live with the logic... Thanks for reviewing the patch. I also think that having O(n^2) comparisons should not be a problem in this logic as it will be processed only during load/parse of config file which we don't do in performance sensitive path. Yep. There is other side effect on this patch. With the patch, when reloading the configurartion file, the server cannot warm an invalid setting value if it's not the last setting of the parameter. This may cause problematic situation as follows. 1. ALTER SYSTEM SET work_mem TO '1024kB'; 2. Reload the configuration file --- success 3. Then, a user accidentally adds work_mem = '2048KB' into postgresql.conf The setting value '2048KB' is invalid, and the unit should be 'kB' instead of 'KB'. 4. Reload the configuration file --- success The invalid setting is ignored because the setting of work_mem in postgresql.auto.conf is preferred. So a user cannot notice that postgresql.conf has an invalid setting. 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to start up because of such an invalid setting. When PostgreSQL starts up, the last setting is preferred. But all the settings are checked. Can we live with this issue? I'd think so, yes. That's pretty extreme corner-case. Yes, that's corner-case. But I'd like to adopt the safer approach if we can implement that easily. The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? 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
Re: [HACKERS] postgresql.auto.conf and reload
On Mon, Aug 4, 2014 at 11:52 PM, Tom Lane t...@sss.pgh.pa.us wrote: Fujii Masao masao.fu...@gmail.com writes: The patch chooses the last settings for every parameters and ignores the former settings. But I don't think that every parameters need to be processed this way. That is, we can change the patch so that only PGC_POSTMASTER parameters are processed that way. The invalid settings in the parameters except PGC_POSTMASTER can be checked by pg_ctl reload as they are now. Also this approach can reduce the number of comparison to choose the last setting, i.e., n in O(n^2) is the number of uncommented *PGC_POSTMASTER* parameters (not every parameters). Thought? I don't find that to be a particularly good idea. In the first place, it introduces extra complication and a surprising difference in the behavior of different GUCs. In the second place, I thought part of the point of this patch was to suppress log messages complaining about invalid values that then weren't actually used for anything. That issue exists just as much for non-POSTMASTER variables. (IOW, value cannot be changed now is not the only log message we're trying to suppress.) Yep, sounds reasonable. This makes me think that we can suppress such invalid message of the parameters which are actually not used at not only conf file reload but also *postmaster startup*. That's another story, though. Anyway, barring any objection, I will commit Amit's patch. 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
Re: [HACKERS] postgresql.auto.conf and reload
On Mon, Jul 28, 2014 at 11:33 PM, Fujii Masao masao.fu...@gmail.com wrote: There is other side effect on this patch. With the patch, when reloading the configurartion file, the server cannot warm an invalid setting value if it's not the last setting of the parameter. This may cause problematic situation as follows. 1. ALTER SYSTEM SET work_mem TO '1024kB'; 2. Reload the configuration file --- success 3. Then, a user accidentally adds work_mem = '2048KB' into postgresql.conf The setting value '2048KB' is invalid, and the unit should be 'kB' instead of 'KB'. 4. Reload the configuration file --- success The invalid setting is ignored because the setting of work_mem in postgresql.auto.conf is preferred. So a user cannot notice that postgresql.conf has an invalid setting. 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to start up because of such an invalid setting. When PostgreSQL starts up, the last setting is preferred. But all the settings are checked. The reason is that during startup DataDir is not set by the time server processes config file due to which we process .auto.conf file in second pass. I think ideally it should ignore the invalid setting in such a case as it does during reload, however currently there is no good way to process .auto.conf incase DataDir is not set, so we handle it separately in second pass. Can we live with this issue? If you are worried about the Reload success case, it will anyway fail later on if user actually wants to set it via postgresql.conf because in such a case user has to remove the setting set by Alter System using Alter System param_name To Default. Incase he doesn't have any such intention, then I think ignoring such invalid params is not a concerning issue. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila amit.kapil...@gmail.com wrote: Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. ISTM that the patch works fine. Only concern is that the logic needs O(n^2) comparison, which may cause performance problem. But n in O(n^2) is the number of uncommented parameters and I don't think it's so big, ISTM I can live with the logic... Thanks for reviewing the patch. I also think that having O(n^2) comparisons should not be a problem in this logic as it will be processed only during load/parse of config file which we don't do in performance sensitive path. Yep. There is other side effect on this patch. With the patch, when reloading the configurartion file, the server cannot warm an invalid setting value if it's not the last setting of the parameter. This may cause problematic situation as follows. 1. ALTER SYSTEM SET work_mem TO '1024kB'; 2. Reload the configuration file --- success 3. Then, a user accidentally adds work_mem = '2048KB' into postgresql.conf The setting value '2048KB' is invalid, and the unit should be 'kB' instead of 'KB'. 4. Reload the configuration file --- success The invalid setting is ignored because the setting of work_mem in postgresql.auto.conf is preferred. So a user cannot notice that postgresql.conf has an invalid setting. 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to start up because of such an invalid setting. When PostgreSQL starts up, the last setting is preferred. But all the settings are checked. Can we live with this issue? 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
Re: [HACKERS] postgresql.auto.conf and reload
On 07/28/2014 11:03 AM, Fujii Masao wrote: On Sat, Jul 26, 2014 at 1:07 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila amit.kapil...@gmail.com wrote: Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. ISTM that the patch works fine. Only concern is that the logic needs O(n^2) comparison, which may cause performance problem. But n in O(n^2) is the number of uncommented parameters and I don't think it's so big, ISTM I can live with the logic... Thanks for reviewing the patch. I also think that having O(n^2) comparisons should not be a problem in this logic as it will be processed only during load/parse of config file which we don't do in performance sensitive path. Yep. There is other side effect on this patch. With the patch, when reloading the configurartion file, the server cannot warm an invalid setting value if it's not the last setting of the parameter. This may cause problematic situation as follows. 1. ALTER SYSTEM SET work_mem TO '1024kB'; 2. Reload the configuration file --- success 3. Then, a user accidentally adds work_mem = '2048KB' into postgresql.conf The setting value '2048KB' is invalid, and the unit should be 'kB' instead of 'KB'. 4. Reload the configuration file --- success The invalid setting is ignored because the setting of work_mem in postgresql.auto.conf is preferred. So a user cannot notice that postgresql.conf has an invalid setting. 5. Failover on shared-disk HA configuration happens, then PostgreSQL fails to start up because of such an invalid setting. When PostgreSQL starts up, the last setting is preferred. But all the settings are checked. Can we live with this issue? I'd think so, yes. That's pretty extreme corner-case. Also, it's my perspective that users who change conf by concurrently editing pg.conf *and* doing ALTER SYSTEM SET are hopeless anyway. There's simply no way we can protect them from themselves. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Jul 9, 2014 at 10:14 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. That is the reason, why I have suggested up-thread that uncommented values should go to postgresql.auto.conf, that will avoid any such observations for a well-behaved user. Uh, what? That sounds like you are proposing that postgresql.conf itself is a dead letter. Which is not going to fly. We had that conversation already. It might sound like that but honestly my intention was to just ease the use for users who just want to rely on Alter System. The right way to fix this is just to avoid processing entries that get overridden later in the configuration file scan. That won't cause anyone to get upset about how their old habits no longer work. Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. ISTM that the patch works fine. Only concern is that the logic needs O(n^2) comparison, which may cause performance problem. But n in O(n^2) is the number of uncommented parameters and I don't think it's so big, ISTM I can live with the logic... 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
Re: [HACKERS] postgresql.auto.conf and reload
On Fri, Jul 25, 2014 at 6:11 PM, Fujii Masao masao.fu...@gmail.com wrote: On Wed, Jul 9, 2014 at 11:05 PM, Amit Kapila amit.kapil...@gmail.com wrote: Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. ISTM that the patch works fine. Only concern is that the logic needs O(n^2) comparison, which may cause performance problem. But n in O(n^2) is the number of uncommented parameters and I don't think it's so big, ISTM I can live with the logic... Thanks for reviewing the patch. I also think that having O(n^2) comparisons should not be a problem in this logic as it will be processed only during load/parse of config file which we don't do in performance sensitive path. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Tom Lane wrote: No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. Just checking -- you didn't get around to dealing with this, right? Not yet... do you want it? I might give it a try, but not just yet. I'll let you know if I attempt anything. -- Álvaro Herrerahttp://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: [HACKERS] postgresql.auto.conf and reload
On Wed, Jul 23, 2014 at 11:02 PM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Tom Lane wrote: Alvaro Herrera alvhe...@2ndquadrant.com writes: Tom Lane wrote: No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. Just checking -- you didn't get around to dealing with this, right? Not yet... do you want it? I might give it a try, but not just yet. I'll let you know if I attempt anything. I am not sure you have noticed or not, but I have posted a patch upthread to address this issue. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Tom Lane wrote: Josh Berkus j...@agliodbs.com writes: 2. *Then*, in a second pass, enforce requirements like can't be changed except at server start. This would also make conf.d much more useful; I wouldn't have to worry as much about overlapping config settings. Sounds like a 9.5 feature, though. No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. Just checking -- you didn't get around to dealing with this, right? -- Álvaro Herrerahttp://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: [HACKERS] postgresql.auto.conf and reload
Alvaro Herrera alvhe...@2ndquadrant.com writes: Tom Lane wrote: No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. Just checking -- you didn't get around to dealing with this, right? Not yet... do you want it? 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
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jul 9, 2014 at 10:14 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. That is the reason, why I have suggested up-thread that uncommented values should go to postgresql.auto.conf, that will avoid any such observations for a well-behaved user. Uh, what? That sounds like you are proposing that postgresql.conf itself is a dead letter. Which is not going to fly. We had that conversation already. It might sound like that but honestly my intention was to just ease the use for users who just want to rely on Alter System. The right way to fix this is just to avoid processing entries that get overridden later in the configuration file scan. That won't cause anyone to get upset about how their old habits no longer work. Okay. As mentioned upthread, I have fixed by ensuring that for duplicate config params, retain only which comes later during parsing. I think it might have been bit simpler to fix, if we try to fix after parsing is complete, but I think for that we might need to replicate the logic at multiple places. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com avoid_processing_dup_config_params_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
Re: Tom Lane 2014-07-08 28635.1404844...@sss.pgh.pa.us Sounds like a 9.5 feature, though. No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. I'd like to throw in again that imho this should include ALTER SYSTEM RESET (ALL) in 9.4. It wouldn't make much sense to delay this to 9.5, when it's simple functionality that can easily be tested and hence shouldn't break anything in 9.4. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On 07/08/2014 08:18 PM, Amit Kapila wrote: Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. That is the reason, why I have suggested up-thread that uncommented values should go to postgresql.auto.conf, that will avoid any such observations for a well-behaved user Not an effective solution for three reasons: 1) Some users will copy over their older pg.conf's to 9.4, which will already have shared_buffers uncommented; 2) Some distros ship their own pg.conf; 3) Doesn't solve the issue of overlapping files in conf.d, which is the same problem. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
On 07/08/2014 06:10 PM, Mark Kirkwood wrote: On 09/07/14 05:13, Josh Berkus wrote: On 07/06/2014 01:27 AM, Christoph Berg wrote: Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. There is no reasonable way for us to prevent issues for users who are manually changing both pg.conf and pg.auto.conf. Users should stick to one or the other method of management (or, thirdly, using conf.d); if they mix methods, we can't prevent confusion at restart time and we shouldn't try. Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. Actually, my response was based on misreading Berg's suggestion; I thought he was suggesting that we would try to disentangle manual changes to both, whereas he was suggesting the opposite. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
Josh Berkus j...@agliodbs.com writes: On 07/08/2014 10:07 AM, Robert Haas wrote: I haven't looked at the code in this area too carefully, but it seems to me like the flow ought to be: 1. Read all of the config files and determine what the final value present in each config file is. AFAICS we only care about the final value, not the final-value-in-each- config-file. 2. *Then*, in a second pass, enforce requirements like can't be changed except at server start. +1 This would also make conf.d much more useful; I wouldn't have to worry as much about overlapping config settings. Sounds like a 9.5 feature, though. No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. 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
Re: [HACKERS] postgresql.auto.conf and reload
On Sat, Jul 5, 2014 at 10:52 AM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: Please find the patch attached to address the above concern. I have updated docs, so that users can be aware of such behaviour. I'm in the camp that says that we need to do something about this, not just claim that it's operating as designed. AFAICS, the *entire* argument for having ALTER SYSTEM at all is ease of use; but this behavior is not what I would call facilitating ease of use. In particular, if you are conveniently able to edit postgresql.conf, what the heck do you need ALTER SYSTEM for? One possible answer is to modify guc-file.l so that only the last value supplied for any one GUC gets processed. The naive way of doing that would be O(N^2) in the number of uncommented settings, which might or might not be a problem; if it is, we could no doubt devise a smarter data structure. I've been really annoyed by the current behavior even on older releases because I have a script, which I use frequently, that does this: initdb cat $PGDATA/postgresql.conf EOM shared_buffers=8GB another setting another setting another setting EOM Now, sometimes while experimenting, I will change a setting in postgresql.conf and reload the config. At which point it complains that it can't change shared_buffers at runtime. It does this because the line from initdb is in the file, and so is the one I added. But I'm *not* trying to change shared_buffers. Sure, the first value in the config file doesn't match the current value, but the *last* line in the config file is the one that's supposed to control, so why is it complaining about the earlier line? I haven't looked at the code in this area too carefully, but it seems to me like the flow ought to be: 1. Read all of the config files and determine what the final value present in each config file is. 2. *Then*, in a second pass, enforce requirements like can't be changed except at server start. -- 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
Re: [HACKERS] postgresql.auto.conf and reload
On 07/08/2014 10:07 AM, Robert Haas wrote: I haven't looked at the code in this area too carefully, but it seems to me like the flow ought to be: 1. Read all of the config files and determine what the final value present in each config file is. 2. *Then*, in a second pass, enforce requirements like can't be changed except at server start. +1 This would also make conf.d much more useful; I wouldn't have to worry as much about overlapping config settings. Sounds like a 9.5 feature, though. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
On 07/06/2014 01:27 AM, Christoph Berg wrote: Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. There is no reasonable way for us to prevent issues for users who are manually changing both pg.conf and pg.auto.conf. Users should stick to one or the other method of management (or, thirdly, using conf.d); if they mix methods, we can't prevent confusion at restart time and we shouldn't try. -- Josh Berkus PostgreSQL Experts Inc. http://pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
On 09/07/14 05:13, Josh Berkus wrote: On 07/06/2014 01:27 AM, Christoph Berg wrote: Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. There is no reasonable way for us to prevent issues for users who are manually changing both pg.conf and pg.auto.conf. Users should stick to one or the other method of management (or, thirdly, using conf.d); if they mix methods, we can't prevent confusion at restart time and we shouldn't try. Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. Regards Mark -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
On Tue, Jul 8, 2014 at 11:59 PM, Tom Lane t...@sss.pgh.pa.us wrote: No, ALTER SYSTEM is there now and it needs to work right in its first release. I will go fix this if nobody else does. I am planing to provide an initial patch for this issue in a day or so, hope that is not too late. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: On 09/07/14 05:13, Josh Berkus wrote: Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. There is no reasonable way for us to prevent issues for users who are manually changing both pg.conf and pg.auto.conf. Users should stick to one or the other method of management (or, thirdly, using conf.d); if they mix methods, we can't prevent confusion at restart time and we shouldn't try. Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. That is the reason, why I have suggested up-thread that uncommented values should go to postgresql.auto.conf, that will avoid any such observations for a well-behaved user. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Amit Kapila amit.kapil...@gmail.com writes: On Wed, Jul 9, 2014 at 6:40 AM, Mark Kirkwood mark.kirkw...@catalyst.net.nz wrote: Yes, but even well behaved users will see this type of error, because initdb uncomments certain values (ones that are dead certs for being changed via ALTER SYSTEM subsequently like shared_buffers), and then - bang! your next reload gets that your postgresql.conf contains errors message. That is the reason, why I have suggested up-thread that uncommented values should go to postgresql.auto.conf, that will avoid any such observations for a well-behaved user. Uh, what? That sounds like you are proposing that postgresql.conf itself is a dead letter. Which is not going to fly. We had that conversation already. The right way to fix this is just to avoid processing entries that get overridden later in the configuration file scan. That won't cause anyone to get upset about how their old habits no longer work. 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
Re: [HACKERS] postgresql.auto.conf and reload
Re: Amit Kapila 2014-07-06 CAA4eK1K=2WCD5ur8c-34NOw+XKg57Q4k0SajwSQXcwciD-=+0...@mail.gmail.com Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. That will make editing postgresql.conf totally useless, because users will always need to check if their new value isn't overridden by something in auto.conf. This will affect *all* users, as everyone needs to bump shared_buffers, which is set by initdb. We'd get 10^X complaints from admins who got confused where their config actually is. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On Sun, Jul 6, 2014 at 1:57 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-07-06 CAA4eK1K=2WCD5ur8c-34NOw+XKg57Q4k0SajwSQXcwciD-=+0...@mail.gmail.com Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. That will make editing postgresql.conf totally useless, because users will always need to check if their new value isn't overridden by something in auto.conf. This will affect *all* users, as everyone needs to bump shared_buffers, which is set by initdb. We'd get 10^X complaints from admins who got confused where their config actually is. Users/admins can always confirm that from pg_settings (sourcefile can be used to find that out). Another thing is that if user is using mixed way (Alter System and manually editing postgresql.conf) to change the configuration parameters, then he needs to be bit careful about setting the same parameter in both ways. Example: 1. Default value of autovacuum_vacuum_cost_delay = 20 2. Edit postgresql.conf and change it to 30 3. Perform Reload 4. Check the value by Show command, it will be 30 5. Alter System set autovacuum_vacuum_cost_delay =40 6. Perform Reload 7. Check the value by Show command, it will be 40 8. Alter System set autovacuum_vacuum_cost_delay = Default; 9 Perform Reload 10 Check the value by Show command, it will be 30 In Step-10, ideally he can expect default value for parameter which is 20, however the actual value will be 30 because of editing done in Step-2. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Amit Kapila amit.kapil...@gmail.com writes: Please find the patch attached to address the above concern. I have updated docs, so that users can be aware of such behaviour. I'm in the camp that says that we need to do something about this, not just claim that it's operating as designed. AFAICS, the *entire* argument for having ALTER SYSTEM at all is ease of use; but this behavior is not what I would call facilitating ease of use. In particular, if you are conveniently able to edit postgresql.conf, what the heck do you need ALTER SYSTEM for? One possible answer is to modify guc-file.l so that only the last value supplied for any one GUC gets processed. The naive way of doing that would be O(N^2) in the number of uncommented settings, which might or might not be a problem; if it is, we could no doubt devise a smarter data structure. 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
Re: [HACKERS] postgresql.auto.conf and reload
On Sat, Jul 5, 2014 at 8:22 PM, Tom Lane t...@sss.pgh.pa.us wrote: Amit Kapila amit.kapil...@gmail.com writes: Please find the patch attached to address the above concern. I have updated docs, so that users can be aware of such behaviour. I'm in the camp that says that we need to do something about this, not just claim that it's operating as designed. AFAICS, the *entire* argument for having ALTER SYSTEM at all is ease of use; but this behavior is not what I would call facilitating ease of use. In particular, if you are conveniently able to edit postgresql.conf, what the heck do you need ALTER SYSTEM for? One possible answer is to modify guc-file.l so that only the last value supplied for any one GUC gets processed. Another could be that during initdb all the uncommented settings be written to postgresql.auto.conf file rather than to postgresql.conf. I think we can do this by changing code in initdb.c-setup_config(). This will ensure that unless user is changing settings in a mixed way (some by Alter System and some manually by editing postgresql.conf), there will no such problem. The naive way of doing that would be O(N^2) in the number of uncommented settings, which might or might not be a problem; if it is, we could no doubt devise a smarter data structure. Okay. To implement it, we can make sure during parsing Configfile that only unique element can be present in list. We can modify function ParseConfigFp() to achieve this functionality. Another way could be that after the list is formed, we can eliminate duplicate entries from it, we might need to do this at multiple places. Do you have anything else in mind? With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Fri, Jun 27, 2014 at 9:11 AM, Amit Kapila amit.kapil...@gmail.com wrote: On Thu, Jun 26, 2014 at 1:49 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-26 CAA4eK1+mUTjc=GXJK3bYtSwV2BmBni= phevbqlqkhduv9cw...@mail.gmail.com How about adding a note in Alter System so that users are aware of such behaviour and can ensure that they don't have duplicate entries? If the behavior isn't going to change, that issue need to be documented, sure. I will send a patch to address this unless someone comes with a better way to address this and if no one objects to adding a note in Alter System documentation. Please find the patch attached to address the above concern. I have updated docs, so that users can be aware of such behaviour. I will add this patch for next CF to avoid getting lost, however I believe it should be done for 9.4. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com alter_system_postmaster_params_v1.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgresql.auto.conf and reload
Re: Amit Kapila 2014-06-26 CAA4eK1+mUTjc=GXJK3bYtSwV2BmBni=phevbqlqkhduv9cw...@mail.gmail.com On Wed, Jun 25, 2014 at 7:52 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-25 caa4ek1+f9ztogvvw-wyj2+vt0k8_jxtziqhp8ivb7wdo1w1...@mail.gmail.com I think maintaining values both in postgresql.conf and by Alter System is not advisable. Possibly, but then the system should be warning about all options, not just the restart-only ones. And it should warn at startup, not at reload time. How about adding a note in Alter System so that users are aware of such behaviour and can ensure that they don't have duplicate entries? If the behavior isn't going to change, that issue need to be documented, sure. Clearly such warnings indicate that there are conflicting settings, so user can take appropriate action to avoid it. I don't think conflicting settings in postgresql.auto.conf are a user error, or misconfiguration. They are just normal use of ALTER SYSTEM. Of course the user might want to eventually consolidate their config, but we shouldn't treat the situation as bogus. Frankly what bugs me most about this is that the warnings occur only at reload time, not at startup. If the server thinks something is wrong with my config, it should tell me rightaway. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On Thu, Jun 26, 2014 at 1:49 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-26 CAA4eK1+mUTjc=GXJK3bYtSwV2BmBni= phevbqlqkhduv9cw...@mail.gmail.com On Wed, Jun 25, 2014 at 7:52 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-25 caa4ek1+f9ztogvvw-wyj2+vt0k8_jxtziqhp8ivb7wdo1w1...@mail.gmail.com I think maintaining values both in postgresql.conf and by Alter System is not advisable. Possibly, but then the system should be warning about all options, not just the restart-only ones. And it should warn at startup, not at reload time. How about adding a note in Alter System so that users are aware of such behaviour and can ensure that they don't have duplicate entries? If the behavior isn't going to change, that issue need to be documented, sure. I will send a patch to address this unless someone comes with a better way to address this and if no one objects to adding a note in Alter System documentation. Clearly such warnings indicate that there are conflicting settings, so user can take appropriate action to avoid it. I don't think conflicting settings in postgresql.auto.conf are a user error, or misconfiguration. They are just normal use of ALTER SYSTEM. Of course the user might want to eventually consolidate their config, but we shouldn't treat the situation as bogus. Frankly what bugs me most about this is that the warnings occur only at reload time, not at startup. If the server thinks something is wrong with my config, it should tell me rightaway. As per current design/code, server don't treat duplicate entries via config file's as a problem, rather the last one is given preference. So in the case you are mentioning, it gives warning at reload time as it encounter's a different value than current value for PGC_POSTMASTER parameter. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
[HACKERS] postgresql.auto.conf and reload
I've just run into this: $ psql -p 5433 (that port is configured in postgresql.conf) # alter system set port = 5494; ... restart the server $ psql -p 5494 # select pg_reload_conf(); 2014-06-25 14:22:07 CEST [11297-4] LOG: received SIGHUP, reloading configuration files 2014-06-25 14:22:07 CEST [11297-5] LOG: parameter port cannot be changed without restarting the server 2014-06-25 14:22:07 CEST [11297-6] LOG: configuration file /etc/postgresql/9.4/main/postgresql.conf contains errors; unaffected changes were applied I think reloading shouldn't be nagging me about parameters in postgresql.conf that are going to be overwritten in postgresql.auto.conf. Reloading again will issue the warnings again... It's true that my config has two contradicting settings in there now, but if that's not an issue at server start, it shouldn't be one for reloading either. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jun 25, 2014 at 6:11 PM, Christoph Berg c...@df7cb.de wrote: I've just run into this: $ psql -p 5433 (that port is configured in postgresql.conf) # alter system set port = 5494; ... restart the server $ psql -p 5494 # select pg_reload_conf(); 2014-06-25 14:22:07 CEST [11297-4] LOG: received SIGHUP, reloading configuration files 2014-06-25 14:22:07 CEST [11297-5] LOG: parameter port cannot be changed without restarting the server 2014-06-25 14:22:07 CEST [11297-6] LOG: configuration file /etc/postgresql/9.4/main/postgresql.conf contains errors; unaffected changes were applied This will happen without Alter System as well, if you change the value of port in postgresql.conf and try to load conf file with SIGHUP. You cannot reload PGC_POSTMASTER parameters without server restart. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Hi, On Wed, 2014-06-25 at 18:42 +0530, Amit Kapila wrote: This will happen without Alter System as well, if you change the value of port in postgresql.conf and try to load conf file with SIGHUP. You cannot reload PGC_POSTMASTER parameters without server restart. Ok, but Christoph already started the server (successfully) with port 5494, so the pg_reload_conf() should ignore it in the next attempt. Regards, -- Devrim GÜNDÜZ Principal Systems Engineer @ EnterpriseDB: http://www.enterprisedb.com PostgreSQL Danışmanı/Consultant, Red Hat Certified Engineer Twitter: @DevrimGunduz , @DevrimGunduzTR signature.asc Description: This is a digitally signed message part
Re: [HACKERS] postgresql.auto.conf and reload
Re: Amit Kapila 2014-06-25 caa4ek1log98jvfov9wztqpcdewja+5jr54ttpkiz3xbngjy...@mail.gmail.com On Wed, Jun 25, 2014 at 6:11 PM, Christoph Berg c...@df7cb.de wrote: I've just run into this: $ psql -p 5433 (that port is configured in postgresql.conf) # alter system set port = 5494; ... restart the server $ psql -p 5494 # select pg_reload_conf(); 2014-06-25 14:22:07 CEST [11297-4] LOG: received SIGHUP, reloading configuration files 2014-06-25 14:22:07 CEST [11297-5] LOG: parameter port cannot be changed without restarting the server 2014-06-25 14:22:07 CEST [11297-6] LOG: configuration file /etc/postgresql/9.4/main/postgresql.conf contains errors; unaffected changes were applied This will happen without Alter System as well, if you change the value of port in postgresql.conf and try to load conf file with SIGHUP. You cannot reload PGC_POSTMASTER parameters without server restart. That's not the issue. I did restart the server, which didn't log any problems, yet reload keeps complaining indefinitely. This makes ALTER SYSTEM not-so-nice-to-use to override parameters already set in postgresql.conf. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jun 25, 2014 at 6:47 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-25 caa4ek1log98jvfov9wztqpcdewja+5jr54ttpkiz3xbngjy...@mail.gmail.com This will happen without Alter System as well, if you change the value of port in postgresql.conf and try to load conf file with SIGHUP. You cannot reload PGC_POSTMASTER parameters without server restart. That's not the issue. I did restart the server, which didn't log any problems, yet reload keeps complaining indefinitely. This makes ALTER SYSTEM not-so-nice-to-use to override parameters already set in postgresql.conf. The main reason behind such such a behaviour after restart is that there are duplicate entries, one in postgresql.conf and another in postgresql.conf. It always first read postgresql.conf and then .auto file and applies the changed parameters one by one, so when it reads a different value than current setting, it can lead to such messages. During processing of config files it doesn't try to eliminate duplicate entries. You can observe same behaviour incase you have another conf file (special.conf, containing conflicting settings) and include that in postgresql.conf. One way could be that while processing if we could eliminate duplicate entries, then it will not lead to such messages, but I think that is existing mechanism and not introduced by Alter System, so changing just for Alter System might impact some existing users. I think maintaining values both in postgresql.conf and by Alter System is not advisable. I am not sure if this addresses your concern completely, but I thinking changing some existing mechanism (maintaining duplicate entries during processing of config files) at this point might be risky. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jun 25, 2014 at 7:45 PM, Amit Kapila amit.kapil...@gmail.com wrote: On Wed, Jun 25, 2014 at 6:47 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-25 caa4ek1log98jvfov9wztqpcdewja+5jr54ttpkiz3xbngjy...@mail.gmail.com This will happen without Alter System as well, if you change the value of port in postgresql.conf and try to load conf file with SIGHUP. You cannot reload PGC_POSTMASTER parameters without server restart. That's not the issue. I did restart the server, which didn't log any problems, yet reload keeps complaining indefinitely. This makes ALTER SYSTEM not-so-nice-to-use to override parameters already set in postgresql.conf. The main reason behind such such a behaviour after restart is that there are duplicate entries, one in postgresql.conf and another in postgresql.conf. Sorry, I mean to say one in postgresql.conf and another in postgresql.auto.conf With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] postgresql.auto.conf and reload
Re: Amit Kapila 2014-06-25 caa4ek1+f9ztogvvw-wyj2+vt0k8_jxtziqhp8ivb7wdo1w1...@mail.gmail.com The main reason behind such such a behaviour after restart is that there are duplicate entries, one in postgresql.conf and another in postgresql.conf. It always first read postgresql.conf and then .auto file and applies the changed parameters one by one, so when it reads a different value than current setting, it can lead to such messages. During processing of config files it doesn't try to eliminate duplicate entries. You can observe same behaviour incase you have another conf file (special.conf, containing conflicting settings) and include that in postgresql.conf. Or even two statements for the same guc in postgresql.conf itself. One way could be that while processing if we could eliminate duplicate entries, then it will not lead to such messages, but I think that is existing mechanism and not introduced by Alter System, so changing just for Alter System might impact some existing users. Yes, it's the same mechanism as before, but now we have a tool that was specifically meant to be used to set and override postgresql.conf parameters, so duplicate entries in postgresql.conf and postgresql.auto.conf are no longer user error (or weirdness), but expected. The system should deal with it. I think maintaining values both in postgresql.conf and by Alter System is not advisable. Possibly, but then the system should be warning about all options, not just the restart-only ones. And it should warn at startup, not at reload time. I am not sure if this addresses your concern completely, but I thinking changing some existing mechanism (maintaining duplicate entries during processing of config files) at this point might be risky. Not sure how intrusive a fix would be - collect all settings during config parse, and only warn once everything has been seen, instead of emitting warnings line-by-line. Christoph -- c...@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
Re: [HACKERS] postgresql.auto.conf and reload
On Wed, Jun 25, 2014 at 7:52 PM, Christoph Berg c...@df7cb.de wrote: Re: Amit Kapila 2014-06-25 caa4ek1+f9ztogvvw-wyj2+vt0k8_jxtziqhp8ivb7wdo1w1...@mail.gmail.com I think maintaining values both in postgresql.conf and by Alter System is not advisable. Possibly, but then the system should be warning about all options, not just the restart-only ones. And it should warn at startup, not at reload time. How about adding a note in Alter System so that users are aware of such behaviour and can ensure that they don't have duplicate entries? Clearly such warnings indicate that there are conflicting settings, so user can take appropriate action to avoid it. I am not sure if this addresses your concern completely, but I thinking changing some existing mechanism (maintaining duplicate entries during processing of config files) at this point might be risky. Not sure how intrusive a fix would be - collect all settings during config parse, and only warn once everything has been seen, instead of emitting warnings line-by-line. As per my understanding, it is more appropriate to eliminate duplicate entries. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com
