Re: Patch for 9.1: initdb -C option

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;
>> > 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(at)endpoint(dot)com
>>
>

--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>