Patch for 9.1: initdb -C option
Hackers,
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation. In my case, I'd been creating multiple db clusters with a script and would have specific overrides that I needed to make. This patch fell out of the desire to make this a little cleaner. Please review and comment.
From the commit message:
This is a simple mechanism to allow you to provide explicit overrides
to any GUC at initdb time. As a basic example, consider the case
where you are programmatically generating multiple db clusters in
order to test various configurations:
$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster -C "port = 1234$cluster" -C 'max_connections = 10' -C shared_buffers=1M;
done
A possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C 'port
1234', -C port=1234, and -C 'port = 1234' to all be ultimately output
as 'port = 1234' in the final output. This would be consistent with
postmaster's parsing.
The -C flag was chosen to be a mnemonic for "config".
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
Attachments:
0001-Add-C-option-to-initdb-to-allow-invocation-time-GUC-.patchapplication/octet-stream; name=0001-Add-C-option-to-initdb-to-allow-invocation-time-GUC-.patch; x-unix-mode=0644Download
From fa77fb314ddac3d047ec34809479f506a172c56c Mon Sep 17 00:00:00 2001
From: David Christensen <david@endpoint.com>
Date: Sun, 28 Mar 2010 23:54:09 -0500
Subject: [PATCH] Add -C option to initdb to allow invocation-time GUC appending to postgresql.conf
This is a simple mechanism to allow you to provide explicit overrides
to any GUC at initdb time. As a basic example, consider the case
where you are programmatically generating multiple db clusters in
order to test various configurations:
$ for cluster in 1 2 3 4 5 6;
> do initdb -D data$cluster -C "port = 1234$cluster" -C 'max_connections = 10' -C shared_buffers=1M;
> done
A possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C 'port
1234', -C port=1234, and -C 'port = 1234' to all be ultimately output
as 'port = 1234' in the final output. This would be consistent with
postmaster's parsing.
The -C flag was chosen to be a mnemonic for "config".
---
src/bin/initdb/initdb.c | 109 ++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 108 insertions(+), 1 deletions(-)
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f40ad87..4855f99 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -111,6 +111,7 @@ static char infoversion[100];
static bool caught_signal = false;
static bool output_failed = false;
static int output_errno = 0;
+static char **append_config_buffer;
/* defaults */
static int n_connections = 10;
@@ -271,6 +272,27 @@ xstrdup(const char *s)
return result;
}
+/* like xstrdup, but appends a newline to the duplicated string*/
+static char *
+xstrdupln(const char *s)
+{
+ char *result;
+ int len = strlen(s);
+
+ result = (char*)malloc(len+2);
+ if (!result)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+ memcpy(result,s,len);
+
+ result[len] = '\n';
+ result[len+1] = 0;
+
+ return result;
+}
+
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
@@ -417,6 +439,79 @@ readfile(const char *path)
}
/*
+ * return a char** created by appending the source** with the dest**
+ *
+ * adds newlines to the end of any line which are missing one.
+ */
+static char **
+append_lines(char **src, char **lines)
+{
+ char **buf;
+ int i;
+ int src_lines = 0;
+ int app_lines = 0;
+
+ for (i=0; src[i]; i++)
+ src_lines++;
+ for (i=0; lines[i]; i++)
+ app_lines++;
+
+ buf = (char**) pg_malloc((src_lines + app_lines + 1) * (sizeof(char *)));
+
+ memcpy(buf,src,(sizeof(char *))*src_lines);
+
+ for (i=0; i<app_lines; i++)
+ buf[src_lines+i] = xstrdup(lines[i]);
+ buf[src_lines + app_lines] = 0; // fill in the last slot
+
+ return buf;
+}
+
+/*
+ * append a single line to the char** buffer. adds a trailing newline if one does not exist.
+ */
+static char **
+append_line(char **src, char *line)
+{
+ char **buf;
+ int i;
+ int src_lines = 0;
+
+ if (!line)
+ return src;
+
+ for (i=0; src[i]; i++)
+ src_lines++;
+
+ /* we assume that anything existing in the buffer already has been
+ * xstrdup'd, and so only xstrdup new lines
+ */
+ buf = (char**) pg_malloc((src_lines + 2) * (sizeof(char *)));
+ memcpy(buf,src,(sizeof(char *))*src_lines);
+
+ buf[src_lines] = xstrdupln(line);
+ buf[src_lines+1] = 0;
+
+ return buf;
+}
+
+/*
+ * append a configuration line to the config buffer. If not already set, add a standard header as the initial contents.
+ */
+static void
+append_config_line(char *line)
+{
+ if (!append_config_buffer)
+ {
+ append_config_buffer = (char**)pg_malloc(sizeof(char*)*2);
+ append_config_buffer[0] = xstrdupln("\n## initdb -C customizations start here");
+ append_config_buffer[1] = 0;
+ }
+
+ append_config_buffer = append_line(append_config_buffer,line);
+}
+
+/*
* write an array of lines to a file
*
* This is only used to write text files. Use fopen "w" not PG_BINARY_W
@@ -1163,9 +1258,15 @@ setup_config(void)
"#default_text_search_config = 'pg_catalog.simple'",
repltok);
+ if (append_config_buffer)
+ {
+ conflines = append_lines(conflines, append_config_buffer);
+ }
+
snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
writefile(path, conflines);
+
chmod(path, 0600);
free(conflines);
@@ -2396,6 +2497,8 @@ usage(const char *progname)
printf(_(" -X, --xlogdir=XLOGDIR location for the transaction log directory\n"));
printf(_("\nLess commonly used options:\n"));
printf(_(" -d, --debug generate lots of debugging output\n"));
+ printf(_(" -C, --append-config=LINE append the provided line to the postgresql.conf file\n"
+ " (can be used multiple times)\n"));
printf(_(" -L DIRECTORY where to find the input files\n"));
printf(_(" -n, --noclean do not clean up after errors\n"));
printf(_(" -s, --show show internal settings\n"));
@@ -2436,6 +2539,7 @@ main(int argc, char *argv[])
{"show", no_argument, NULL, 's'},
{"noclean", no_argument, NULL, 'n'},
{"xlogdir", required_argument, NULL, 'X'},
+ {"append-config", required_argument, NULL, 'C'},
{NULL, 0, NULL, 0}
};
@@ -2488,7 +2592,7 @@ main(int argc, char *argv[])
/* process command-line options */
- while ((c = getopt_long(argc, argv, "dD:E:L:nU:WA:sT:X:", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "C:dD:E:L:nU:WA:sT:X:", long_options, &option_index)) != -1)
{
switch (c)
{
@@ -2554,6 +2658,9 @@ main(int argc, char *argv[])
case 'X':
xlog_dir = xstrdup(optarg);
break;
+ case 'C':
+ append_config_line(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
--
1.6.4.rc3
initdb-dash-C.diffapplication/octet-stream; name=initdb-dash-C.diff; x-unix-mode=0644Download
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index f40ad87..4855f99 100644
*** a/src/bin/initdb/initdb.c
--- b/src/bin/initdb/initdb.c
*************** static char infoversion[100];
*** 111,116 ****
--- 111,117 ----
static bool caught_signal = false;
static bool output_failed = false;
static int output_errno = 0;
+ static char **append_config_buffer;
/* defaults */
static int n_connections = 10;
*************** xstrdup(const char *s)
*** 271,276 ****
--- 272,298 ----
return result;
}
+ /* like xstrdup, but appends a newline to the duplicated string*/
+ static char *
+ xstrdupln(const char *s)
+ {
+ char *result;
+ int len = strlen(s);
+
+ result = (char*)malloc(len+2);
+ if (!result)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+ memcpy(result,s,len);
+
+ result[len] = '\n';
+ result[len+1] = 0;
+
+ return result;
+ }
+
/*
* make a copy of the array of lines, with token replaced by replacement
* the first time it occurs on each line.
*************** readfile(const char *path)
*** 417,422 ****
--- 439,517 ----
}
/*
+ * return a char** created by appending the source** with the dest**
+ *
+ * adds newlines to the end of any line which are missing one.
+ */
+ static char **
+ append_lines(char **src, char **lines)
+ {
+ char **buf;
+ int i;
+ int src_lines = 0;
+ int app_lines = 0;
+
+ for (i=0; src[i]; i++)
+ src_lines++;
+ for (i=0; lines[i]; i++)
+ app_lines++;
+
+ buf = (char**) pg_malloc((src_lines + app_lines + 1) * (sizeof(char *)));
+
+ memcpy(buf,src,(sizeof(char *))*src_lines);
+
+ for (i=0; i<app_lines; i++)
+ buf[src_lines+i] = xstrdup(lines[i]);
+ buf[src_lines + app_lines] = 0; // fill in the last slot
+
+ return buf;
+ }
+
+ /*
+ * append a single line to the char** buffer. adds a trailing newline if one does not exist.
+ */
+ static char **
+ append_line(char **src, char *line)
+ {
+ char **buf;
+ int i;
+ int src_lines = 0;
+
+ if (!line)
+ return src;
+
+ for (i=0; src[i]; i++)
+ src_lines++;
+
+ /* we assume that anything existing in the buffer already has been
+ * xstrdup'd, and so only xstrdup new lines
+ */
+ buf = (char**) pg_malloc((src_lines + 2) * (sizeof(char *)));
+ memcpy(buf,src,(sizeof(char *))*src_lines);
+
+ buf[src_lines] = xstrdupln(line);
+ buf[src_lines+1] = 0;
+
+ return buf;
+ }
+
+ /*
+ * append a configuration line to the config buffer. If not already set, add a standard header as the initial contents.
+ */
+ static void
+ append_config_line(char *line)
+ {
+ if (!append_config_buffer)
+ {
+ append_config_buffer = (char**)pg_malloc(sizeof(char*)*2);
+ append_config_buffer[0] = xstrdupln("\n## initdb -C customizations start here");
+ append_config_buffer[1] = 0;
+ }
+
+ append_config_buffer = append_line(append_config_buffer,line);
+ }
+
+ /*
* write an array of lines to a file
*
* This is only used to write text files. Use fopen "w" not PG_BINARY_W
*************** setup_config(void)
*** 1163,1171 ****
--- 1258,1272 ----
"#default_text_search_config = 'pg_catalog.simple'",
repltok);
+ if (append_config_buffer)
+ {
+ conflines = append_lines(conflines, append_config_buffer);
+ }
+
snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
writefile(path, conflines);
+
chmod(path, 0600);
free(conflines);
*************** usage(const char *progname)
*** 2396,2401 ****
--- 2497,2504 ----
printf(_(" -X, --xlogdir=XLOGDIR location for the transaction log directory\n"));
printf(_("\nLess commonly used options:\n"));
printf(_(" -d, --debug generate lots of debugging output\n"));
+ printf(_(" -C, --append-config=LINE append the provided line to the postgresql.conf file\n"
+ " (can be used multiple times)\n"));
printf(_(" -L DIRECTORY where to find the input files\n"));
printf(_(" -n, --noclean do not clean up after errors\n"));
printf(_(" -s, --show show internal settings\n"));
*************** main(int argc, char *argv[])
*** 2436,2441 ****
--- 2539,2545 ----
{"show", no_argument, NULL, 's'},
{"noclean", no_argument, NULL, 'n'},
{"xlogdir", required_argument, NULL, 'X'},
+ {"append-config", required_argument, NULL, 'C'},
{NULL, 0, NULL, 0}
};
*************** main(int argc, char *argv[])
*** 2488,2494 ****
/* process command-line options */
! while ((c = getopt_long(argc, argv, "dD:E:L:nU:WA:sT:X:", long_options, &option_index)) != -1)
{
switch (c)
{
--- 2592,2598 ----
/* process command-line options */
! while ((c = getopt_long(argc, argv, "C:dD:E:L:nU:WA:sT:X:", long_options, &option_index)) != -1)
{
switch (c)
{
*************** main(int argc, char *argv[])
*** 2554,2559 ****
--- 2658,2666 ----
case 'X':
xlog_dir = xstrdup(optarg);
break;
+ case 'C':
+ append_config_line(optarg);
+ break;
default:
/* getopt_long already emitted a complaint */
fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
David Christensen <david@endpoint.com> wrote:
Enclosed is a patch to add a -C option to initdb to allow you to easily
append configuration directives to the generated postgresql.conf file
Why don't you use just "echo 'options' >> $PGDATA/postgresql.conf" ?
Could you explain where the -C options is better than initdb + echo?
Regards,
---
Takahiro Itagaki
NTT Open Source Software Center
David Christensen wrote:
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation.
We had a patch not quite make it for 9.0 that switched over the
postgresql.conf file to make it easy to scan a whole directory looking
for configuration files:
http://archives.postgresql.org/message-id/9837222c0910240641p7d75e2a4u2cfa6c1b5e603d84@mail.gmail.com
The idea there was to eventually reduce the amount of postgresql.conf
hacking that initdb and other tools have to do. Your patch would add
more code into a path that I'd like to see reduced significantly.
That implementation would make something easy enough for your use case
too (below untested but show the general idea):
$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster
(
cat <<EOF
port = 1234$cluster;
max_connections = 10;
shared_buffers=1M;
EOF
) > data$cluster/conf.d/99clustersetup
done
This would actually work just fine for what you're doing right now if
you used ">> data$cluster/postgresql.conf" for that next to last line
there. There would be duplicates, which I'm guessing is what you wanted
to avoid with this patch, but the later values set for the parameters
added to the end would win and be the active ones.
--
Greg Smith 2ndQuadrant US Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com www.2ndQuadrant.us
On mån, 2010-03-29 at 00:04 -0500, David Christensen wrote:
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation.
I like this idea, but please use small -c for consistency with the
postgres program.
I have added this to the 9.1 commit-fest:
https://commitfest.postgresql.org/action/commitfest_view?id=6
---------------------------------------------------------------------------
David Christensen wrote:
Hackers,
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation. In my case, I'd been creating multiple db clusters with a script and would have specific overrides that I needed to make. This patch fell out of the desire to make this a little cleaner. Please review and comment.
From the commit message:
This is a simple mechanism to allow you to provide explicit overrides
to any GUC at initdb time. As a basic example, consider the case
where you are programmatically generating multiple db clusters in
order to test various configurations:$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster -C "port = 1234$cluster" -C 'max_connections = 10' -C shared_buffers=1M;
doneA possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C 'port
1234', -C port=1234, and -C 'port = 1234' to all be ultimately output
as 'port = 1234' in the final output. This would be consistent with
postmaster's parsing.The -C flag was chosen to be a mnemonic for "config".
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
[ Attachment, skipping... ]
[ Attachment, skipping... ]
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
David,
I'd like to volunteer reviewing your patch at first in this commit fest.
We already had a few comments on the list before. I want to see your
opinion for the suggestions prior to code reviews.
Itagaki-san suggested:
| > Enclosed is a patch to add a -C option to initdb to allow you to easily
| > append configuration directives to the generated postgresql.conf file
| Why don't you use just "echo 'options' >> $PGDATA/postgresql.conf" ?
| Could you explain where the -C options is better than initdb + echo?
Greg suggested:
| We had a patch not quite make it for 9.0 that switched over the postgresql.conf
| file to make it easy to scan a whole directory looking for configuration files:
| http://archives.postgresql.org/message-id/9837222c0910240641p7d75e2a4u2cfa6c1b5e603d84@mail.gmail.com
|
| The idea there was to eventually reduce the amount of postgresql.conf hacking
| that initdb and other tools have to do. Your patch would add more code into
| a path that I'd like to see reduced significantly.
|
| That implementation would make something easy enough for your use case too
| (below untested but show the general idea):
|
| $ for cluster in 1 2 3 4 5 6;
| do initdb -D data$cluster
| (
| cat <<EOF
| port = 1234$cluster;
| max_connections = 10;
| shared_buffers=1M;
| EOF
| ) > data$cluster/conf.d/99clustersetup
| done
|
| This would actually work just fine for what you're doing right now if you used
| ">> data$cluster/postgresql.conf" for that next to last line there.
| There would be duplicates, which I'm guessing is what you wanted to avoid with
| this patch, but the later values set for the parameters added to the end would
| win and be the active ones.
Peter suggested:
| > Enclosed is a patch to add a -C option to initdb to allow you to easily
| > append configuration directives to the generated postgresql.conf file
| > for use in programmatic generation.
| I like this idea, but please use small -c for consistency with the
| postgres program.
It seems to me what Greg suggested is a recent trend. Additional configurations
within separated files enables to install/uninstall third-party plugins easily
from the perspective of packagers, rather than consolidated configuration.
However, $PGDATA/postgresql.conf is created on initdb, so it does not belong
to a certain package. I don't have certainty whether the recent trend is also
suitable for us, or not.
Thanks,
(2010/03/29 14:04), David Christensen wrote:
Hackers,
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation. In my case, I'd been creating multiple db clusters with a script and would have specific overrides that I needed to make. This patch fell out of the desire to make this a little cleaner. Please review and comment.
From the commit message:
This is a simple mechanism to allow you to provide explicit overrides
to any GUC at initdb time. As a basic example, consider the case
where you are programmatically generating multiple db clusters in
order to test various configurations:$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster -C "port = 1234$cluster" -C 'max_connections = 10' -C shared_buffers=1M;
doneA possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C 'port
1234', -C port=1234, and -C 'port = 1234' to all be ultimately output
as 'port = 1234' in the final output. This would be consistent with
postmaster's parsing.The -C flag was chosen to be a mnemonic for "config".
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
--
KaiGai Kohei <kaigai@ak.jp.nec.com>
Top posting. On purpose. :p
This patch seems to be foundering in a sea of opinions. It seems
that everybody wants to do *something* about this, but what?
For one more opinion, my shop has chosen to never touch the default
postgresql.conf file any more, beyond adding one line to the bottom
of it which is an include directive, to bring in our overrides.
What will make everyone happy here?
-Kevin
KaiGai Kohei <kaigai@ak.jp.nec.com> wrote:
(2010/03/29 14:04), David Christensen wrote:
Enclosed is a patch to add a -C option to initdb to allow you to
easily append configuration directives to the generated
postgresql.conf file for use in programmatic generation. In my
case, I'd been creating multiple db clusters with a script and
would have specific overrides that I needed to make. This patch
fell out of the desire to make this a little cleaner.
Please review and comment.From the commit message:
This is a simple mechanism to allow you to provide explicit
overrides to any GUC at initdb time. As a basic example,
consider the case where you are programmatically generating
multiple db clusters in order to test various
configurations:$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster -C "port = 1234$cluster" \
-C 'max_connections = 10' -C shared_buffers=1M;
doneA possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C
'port 1234', -C port=1234, and -C 'port = 1234' to all be
ultimately output as 'port = 1234' in the final output.
This would be consistent with postmaster's parsing.The -C flag was chosen to be a mnemonic for "config".
I'd like to volunteer reviewing your patch at first in this commit
fest.We already had a few comments on the list before. I want to see
your opinion for the suggestions prior to code reviews.Itagaki-san suggested:
| Why don't you use just "echo 'options' \
| >>$PGDATA/postgresql.conf" ?
| Could you explain where the -C options is better than initdb +
| echo?Greg suggested:
| We had a patch not quite make it for 9.0 that switched over the
| postgresql.conf file to make it easy to scan a whole directory
| looking for configuration files:
|
Show quoted text
|
| The idea there was to eventually reduce the amount of
| postgresql.conf hacking that initdb and other tools have to do.
| Your patch would add more code into a path that I'd like to see
| reduced significantly.
|
| That implementation would make something easy enough for your
| use case too (below untested but show the general idea):
|
| $ for cluster in 1 2 3 4 5 6;
| do initdb -D data$cluster
| (
| cat <<EOF
| port = 1234$cluster;
| max_connections = 10;
| shared_buffers=1M;
| EOF
| ) > data$cluster/conf.d/99clustersetup
| done
|
| This would actually work just fine for what you're doing right
| now if you used ">> data$cluster/postgresql.conf" for that next
| to last line there. There would be duplicates, which I'm
| guessing is what you wanted to avoid with this patch, but the
| later values set for the parameters added to the end would win
| and be the active ones.Peter suggested:
| I like this idea, but please use small -c for consistency with
| the postgres program.It seems to me what Greg suggested is a recent trend. Additional
configurations within separated files enables to install/uninstall
third-party plugins easily from the perspective of packagers,
rather than consolidated configuration.However, $PGDATA/postgresql.conf is created on initdb, so it does
not belong to a certain package. I don't have certainty whether
the recent trend is also suitable for us, or not.
On Jul 20, 2010, at 5:00 PM, Kevin Grittner wrote:
Top posting. On purpose. :p
This patch seems to be foundering in a sea of opinions. It seems
that everybody wants to do *something* about this, but what?For one more opinion, my shop has chosen to never touch the default
postgresql.conf file any more, beyond adding one line to the bottom
of it which is an include directive, to bring in our overrides.What will make everyone happy here?
So you'll now issue:
$ initdb ... -C 'include localconfig.conf' ? :-)
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
David Christensen <david@endpoint.com> wrote:
On Jul 20, 2010, at 5:00 PM, Kevin Grittner wrote:
my shop has chosen to never touch the default postgresql.conf
file any more, beyond adding one line to the bottom of it which
is an include directive, to bring in our overrides.
So you'll now issue:
$ initdb ... -C 'include localconfig.conf' ? :-)
Yeah, that would cover us. I'm wondering if it is flexible enough
to serve everyone else so well. I hesitate to suggest it, but
perhaps it would, in combination with the include directive
supporting a directory name to mean all files in the directory? Or
maybe if it supported wildcards?
-Kevin
(2010/07/21 7:33), Kevin Grittner wrote:
David Christensen<david@endpoint.com> wrote:
On Jul 20, 2010, at 5:00 PM, Kevin Grittner wrote:
my shop has chosen to never touch the default postgresql.conf
file any more, beyond adding one line to the bottom of it which
is an include directive, to bring in our overrides.So you'll now issue:
$ initdb ... -C 'include localconfig.conf' ? :-)
Yeah, that would cover us. I'm wondering if it is flexible enough
to serve everyone else so well. I hesitate to suggest it, but
perhaps it would, in combination with the include directive
supporting a directory name to mean all files in the directory? Or
maybe if it supported wildcards?
I reminded that David introduced the following example as a usage of
this feature.
$ for cluster in 1 2 3 4 5 6;
do
initdb -D data$cluster -C "port = 1234$cluster" \
-C 'max_connections = 10' \
-C shared_buffers=1M;
done
In this case, it tries to set up six database clusters with constant
max_connections and shared_buffers, but individual port numbers for
each database clusters.
Even if we support include directive here, it may not help to describe
the postgresql.conf with a smart way.
Then, how about the Itagaki-san's suggestion?
Itagaki-san suggested:
| > Enclosed is a patch to add a -C option to initdb to allow you to easily
| > append configuration directives to the generated postgresql.conf file
| Why don't you use just "echo 'options' >> $PGDATA/postgresql.conf" ?
| Could you explain where the -C options is better than initdb + echo?
As long as you don't need any special configuration during the initial
setting up launched by initdb, indeed, it seems to me we can edit the
postgresql.conf later.
Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>
On Tue, Jul 20, 2010 at 6:00 PM, Kevin Grittner
<Kevin.Grittner@wicourts.gov> wrote:
What will make everyone happy here?
Nothing.
But on a more serious note, the basic dilemma with this patch is
whether it's useful enough to justify the extra code. I think it's
pretty clearly harmless (modulo the fact that it might have bugs), but
is it useful enough to bother with?
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
David,
I checked your patch. Then, there are a few comments in the code.
Apart from the discussion in this thread, would you fix them please.
| *** a/src/bin/initdb/initdb.c
| --- b/src/bin/initdb/initdb.c
| *************** static char infoversion[100];
| *** 111,116 ****
| --- 111,117 ----
| static bool caught_signal = false;
| static bool output_failed = false;
| static int output_errno = 0;
| + static char **append_config_buffer;
|
| /* defaults */
| static int n_connections = 10;
It should be initialized by NULL.
| + static char **
| + append_line(char **src, char *line)
| + {
| + char **buf;
| + int i;
| + int src_lines = 0;
| +
| + if (!line)
| + return src;
| +
| + for (i=0; src[i]; i++)
| + src_lines++;
| +
| + /* we assume that anything existing in the buffer already has been
| + * xstrdup'd, and so only xstrdup new lines
| + */
| + buf = (char**) pg_malloc((src_lines + 2) * (sizeof(char *)));
| + memcpy(buf,src,(sizeof(char *))*src_lines);
| +
| + buf[src_lines] = xstrdupln(line);
| + buf[src_lines+1] = 0;
| +
| + return buf;
| + }
The append_line() is just append one lines on the tail of append_config_buffer
array. Why is it needed to compute number of elements for each invocations?
If we have a static variable to hold number of elements in append_config_buffer,
we can just implement it as follows:
append_config_buffer[append_config_buffer_index++] = xstrdupln(line);
append_config_buffer[append_config_buffer_index] = NULL;
And, it calls pg_malloc() for each invocations, but it may be costful.
Could you allocate 100 elements at first, then reallocate it only when
append_config_buffer_index reaches the boundary of the array?
And, don't use 0 for pointers, instead of NULL.
Anyway, it is an obvious feature, and seems to me works fine.
However, it is not clear for me how do we make progress this feature.
If we support a command to include other configuration, it also needs
to patch on the postgresql backend, not only initdb.
In my personal opinion, as long as you don't need any special configuration
under the single user mode or bootstraping mode launched by initdb,
we can modify it using shell script or others later.
It seems to me we need to make clear where is our consensus at first. :(
Thanks,
(2010/07/15 9:16), KaiGai Kohei wrote:
David,
I'd like to volunteer reviewing your patch at first in this commit fest.
We already had a few comments on the list before. I want to see your
opinion for the suggestions prior to code reviews.Itagaki-san suggested:
|> Enclosed is a patch to add a -C option to initdb to allow you to easily
|> append configuration directives to the generated postgresql.conf file
| Why don't you use just "echo 'options'>> $PGDATA/postgresql.conf" ?
| Could you explain where the -C options is better than initdb + echo?Greg suggested:
| We had a patch not quite make it for 9.0 that switched over the postgresql.conf
| file to make it easy to scan a whole directory looking for configuration files:
| http://archives.postgresql.org/message-id/9837222c0910240641p7d75e2a4u2cfa6c1b5e603d84@mail.gmail.com
|
| The idea there was to eventually reduce the amount of postgresql.conf hacking
| that initdb and other tools have to do. Your patch would add more code into
| a path that I'd like to see reduced significantly.
|
| That implementation would make something easy enough for your use case too
| (below untested but show the general idea):
|
| $ for cluster in 1 2 3 4 5 6;
| do initdb -D data$cluster
| (
| cat<<EOF
| port = 1234$cluster;
| max_connections = 10;
| shared_buffers=1M;
| EOF
| )> data$cluster/conf.d/99clustersetup
| done
|
| This would actually work just fine for what you're doing right now if you used
| ">> data$cluster/postgresql.conf" for that next to last line there.
| There would be duplicates, which I'm guessing is what you wanted to avoid with
| this patch, but the later values set for the parameters added to the end would
| win and be the active ones.Peter suggested:
|> Enclosed is a patch to add a -C option to initdb to allow you to easily
|> append configuration directives to the generated postgresql.conf file
|> for use in programmatic generation.
| I like this idea, but please use small -c for consistency with the
| postgres program.It seems to me what Greg suggested is a recent trend. Additional configurations
within separated files enables to install/uninstall third-party plugins easily
from the perspective of packagers, rather than consolidated configuration.However, $PGDATA/postgresql.conf is created on initdb, so it does not belong
to a certain package. I don't have certainty whether the recent trend is also
suitable for us, or not.Thanks,
(2010/03/29 14:04), David Christensen wrote:
Hackers,
Enclosed is a patch to add a -C option to initdb to allow you to easily append configuration directives to the generated postgresql.conf file for use in programmatic generation. In my case, I'd been creating multiple db clusters with a script and would have specific overrides that I needed to make. This patch fell out of the desire to make this a little cleaner. Please review and comment.
From the commit message:
This is a simple mechanism to allow you to provide explicit overrides
to any GUC at initdb time. As a basic example, consider the case
where you are programmatically generating multiple db clusters in
order to test various configurations:$ for cluster in 1 2 3 4 5 6;
do initdb -D data$cluster -C "port = 1234$cluster" -C 'max_connections = 10' -C shared_buffers=1M;
doneA possible future improvement would be to provide some basic
formatting corrections to allow specificications such as -C 'port
1234', -C port=1234, and -C 'port = 1234' to all be ultimately output
as 'port = 1234' in the final output. This would be consistent with
postmaster's parsing.The -C flag was chosen to be a mnemonic for "config".
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
--
KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/22 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Anyway, it is an obvious feature, and seems to me works fine.
So this makes it sound like you like the feature.
However, it is not clear for me how do we make progress this feature.
If we support a command to include other configuration, it also needs
to patch on the postgresql backend, not only initdb.
I don't know what this means.
In my personal opinion, as long as you don't need any special configuration
under the single user mode or bootstraping mode launched by initdb,
we can modify it using shell script or others later.
But here it sounds like you're saying we don't need the feature
because may as well just edit postgresql.conf by hand.
So I'm confused.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
(2010/07/23 13:00), Robert Haas wrote:
2010/7/22 KaiGai Kohei<kaigai@ak.jp.nec.com>:
Anyway, it is an obvious feature, and seems to me works fine.
So this makes it sound like you like the feature.
However, it is not clear for me how do we make progress this feature.
If we support a command to include other configuration, it also needs
to patch on the postgresql backend, not only initdb.I don't know what this means.
In my personal opinion, as long as you don't need any special configuration
under the single user mode or bootstraping mode launched by initdb,
we can modify it using shell script or others later.But here it sounds like you're saying we don't need the feature
because may as well just edit postgresql.conf by hand.So I'm confused.
Sorry for the confusion.
What I wanted to say is the patch itself is fine but we need to make consensus
before the detailed code reviewing.
Thanks,
--
KaiGai Kohei <kaigai@ak.jp.nec.com>
2010/7/23 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Sorry for the confusion.
What I wanted to say is the patch itself is fine but we need to make consensus
before the detailed code reviewing.
I guess we probably need some more people to express an opinion, then.
Do you have one?
I'm not sure I do, yet. I'd like to hear the patch author's response
to Itagaki Takahiro's question upthread: "Why don't you use just "echo
'options' >> $PGDATA/postgresql.conf" ? Could you explain where the
-C options is better than initdb + echo?"
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company
On Jul 23, 2010, at 6:36 AM, Robert Haas wrote:
2010/7/23 KaiGai Kohei <kaigai@ak.jp.nec.com>:
Sorry for the confusion.
What I wanted to say is the patch itself is fine but we need to make consensus
before the detailed code reviewing.I guess we probably need some more people to express an opinion, then.
Do you have one?I'm not sure I do, yet. I'd like to hear the patch author's response
to Itagaki Takahiro's question upthread: "Why don't you use just "echo
'options' >> $PGDATA/postgresql.conf" ? Could you explain where the
-C options is better than initdb + echo?"
At this point, I have no real preference for this patch; it is just as easy to echo line >> datadir/postgresql.conf, so perhaps that makes this patch somewhat pointless. I suppose there's a shaky argument to be made for Windows compatibility, but I'm sure there's also an equivalent functionality to be found in the windows shell.
Reception to this idea has seemed pretty lukewarm, although I think Peter expressed some interest. Some of the previous linked correspondence in the review referred to some of the proposed split configuration file mechanisms. My particular implementation is fairly limited to the idea of a single configuration file, so compared to some of the other proposed approaches including split .conf files, it may not cover the same ground.
Like I said in the original submission, I found it helpful for the programmatic configuration of a number of simultaneous node, but if it's not generally useful to the community at large, I'll understand if it's punted.
Regards,
David
--
David Christensen
End Point Corporation
david@endpoint.com
David Christensen escreveu:
Like I said in the original submission, I found it helpful for the programmatic configuration of a number of simultaneous node, but if it's not generally useful to the community at large, I'll understand if it's punted.
I'm afraid it is the only use case for this new option. If it is, it doesn't
deserve a new option. We can live with echo + initdb for those cases.
--
Euler Taveira de Oliveira
http://www.timbira.com/
David Christensen <david@endpoint.com> wrote:
At this point, I have no real preference for this patch; it is
just as easy to echo line >> datadir/postgresql.conf, so perhaps
that makes this patch somewhat pointless.
On reflection, I'm inclined to agree.
I suppose there's a shaky argument to be made for Windows
compatibility, but I'm sure there's also an equivalent
functionality to be found in the windows shell.
The Windows command shell supports the echo command and >> to
append.
Like I said in the original submission, I found it helpful for
the programmatic configuration of a number of simultaneous node,
but if it's not generally useful to the community at large, I'll
understand if it's punted.
Unless someone can show a significant benefit beyond >> appends,
I'll do that in a couple days.
-Kevin