On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemer...@gmail.com> wrote: > Hello, > > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coe...@cri.ensmp.fr> wrote: >> >> >> Hello Masahiko, >> >>>> So I would suggest to: >>>> - fix the compilation issue >>>> - leave -l/--log as it is, i.e. use "pgbench_log" as a prefix >>>> - add --log-prefix=... (long option only) for changing this prefix >>> >>> >>> I agree. It's better to add the separated option to specify the prefix >>> of log file instead of changing the existing behaviour. Attached >>> latest patch incorporated review comments. >>> Please review it. >> >> >> Patch applies, compiles and works for me. > > > It works for me as well. > >> >> >> This new option is for benchmarking, so "benchmarking_option_set" should >> be set to true. >> >> To simplify the code, I would suggest to initialize explicitely >> "logfile_prefix" to the default value which is then overriden when the >> option is set, which allows to get rid of the "prefix" variable later. >> >> There is no reason to change the documentation by breaking a line at >> another place if the text is not changed (where ... with 1). >> >> I would suggest to simplify a little bit the documentation: >> "prefix of log file ..." -> >> "default log file prefix that can be changed with <option>...</>" >> >> Otherwise the explanations seem clear enough to me. If a native English >> speaker could check them though, it would be nice. > > > For the explanation of the option --log-prefix, I feel it would be better to > say "Custom prefix for transaction log file. Default is pgbench_log" > > > - <filename>pgbench_log.<replaceable>nnn</></filename>, where > + > <filename><replaceable>pgbench_log</replaceable>.<replaceable>nnn</></filename>, > + where <replaceable>pgbench_log</replaceable> is the prefix of log file > that can > + be changed by specifying <option>--log-prefix</option>, and where > > It could just say "the default prefix pgbench_log can be changed with option > --log-prefix, and " we need not use where again. > > Also the error message is made similar to that of aggregate-interval but I > think the one in sampling-rate is better: > > $ pgbench --log-prefix=chk -t 20 > log file prefix (--log-prefix) is allowed only when actually logging > transactions > > pgbench --sampling-rate=1 -t 20 > log sampling (--sampling-rate) is allowed only when logging transactions > (-l) >
Thank you for reviewing this patch! Attached latest patch incorporated comments. Please review it. Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 285608d..a6fe183 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -614,6 +614,15 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</> </listitem> </varlistentry> + <varlistentry> + <term><option>--log-prefix=<replaceable>prefix</></option></term> + <listitem> + <para> + Custom prefix of transaction log file. Default is <replaceable>pgbench_log</>. + </para> + </listitem> + </varlistentry> + </variablelist> </para> @@ -1121,13 +1130,14 @@ END; With the <option>-l</> option but without the <option>--aggregate-interval</option>, <application>pgbench</> writes the time taken by each transaction to a log file. The log file will be named - <filename>pgbench_log.<replaceable>nnn</></filename>, where - <replaceable>nnn</> is the PID of the <application>pgbench</application> process. - If the <option>-j</> option is 2 or higher, creating multiple worker - threads, each will have its own log file. The first worker will use the - same name for its log file as in the standard single worker case. + <filename><replaceable>pgbench_log</>.<replaceable>nnn</></filename>, + where the default prefix <replaceable>pgbench_log</> can be changed with option + <option>--log-prefix</>, and <replaceable>nnn</> is the PID of the + <application>pgbench</application> process. If the <option>-j</> option is 2 or higher, + creating multiple worker threads, each will have its own log file. The first worker will + use the same name for its log file as in the standard single worker case. The additional log files for the other workers will be named - <filename>pgbench_log.<replaceable>nnn</>.<replaceable>mmm</></filename>, + <filename><replaceable>pgbench_log</>.<replaceable>nnn</>.<replaceable>mmm</></filename>, where <replaceable>mmm</> is a sequential number for each worker starting with 1. </para> diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c index d44cfda..c7c1ac6 100644 --- a/src/bin/pgbench/pgbench.c +++ b/src/bin/pgbench/pgbench.c @@ -180,6 +180,7 @@ char *pghost = ""; char *pgport = ""; char *login = NULL; char *dbName; +char *logfile_prefix = "pgbench_log"; const char *progname; #define WSEP '@' /* weight separator */ @@ -511,6 +512,7 @@ usage(void) " --aggregate-interval=NUM aggregate data over NUM seconds\n" " --progress-timestamp use Unix epoch timestamps for progress\n" " --sampling-rate=NUM fraction of transactions to log (e.g., 0.01 for 1%%)\n" + " --log-prefix=PREFIX prefix of transaction time log file (default: pgbench_log)\n" "\nCommon options:\n" " -d, --debug print debugging output\n" " -h, --host=HOSTNAME database server host or socket directory\n" @@ -3643,6 +3645,7 @@ main(int argc, char **argv) {"sampling-rate", required_argument, NULL, 4}, {"aggregate-interval", required_argument, NULL, 5}, {"progress-timestamp", no_argument, NULL, 6}, + {"log-prefix", required_argument, NULL, 7}, {NULL, 0, NULL, 0} }; @@ -3990,6 +3993,10 @@ main(int argc, char **argv) progress_timestamp = true; benchmarking_option_set = true; break; + case 7: + benchmarking_option_set = true; + logfile_prefix = pg_strdup(optarg); + break; default: fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname); exit(1); @@ -4087,6 +4094,12 @@ main(int argc, char **argv) exit(1); } + if (!use_log && logfile_prefix) + { + fprintf(stderr, "log file prefix (--log-prefix) is allowed only when logging transactions (-l)\n"); + exit(1); + } + if (duration > 0 && agg_interval > duration) { fprintf(stderr, "number of seconds for aggregation (%d) must not be higher than test duration (%d)\n", agg_interval, duration); @@ -4390,9 +4403,10 @@ threadRun(void *arg) char logpath[64]; if (thread->tid == 0) - snprintf(logpath, sizeof(logpath), "pgbench_log.%d", main_pid); + snprintf(logpath, sizeof(logpath), "%s.%d", logfile_prefix, main_pid); else - snprintf(logpath, sizeof(logpath), "pgbench_log.%d.%d", main_pid, thread->tid); + snprintf(logpath, sizeof(logpath), "%s.%d.%d", logfile_prefix, main_pid, thread->tid); + thread->logfile = fopen(logpath, "w"); if (thread->logfile == NULL)
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers