On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <memissemer...@gmail.com> wrote: > Hello Sawada-san, > > On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada.m...@gmail.com> > wrote: >> >> 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. >> > > Thank you for updating the patch. > > I note that the current changes, break the behaviour when we do not use -l > option. > > Since the log_prefix variable is set to default value, the check " if > (!use_log && logfile_prefix)" always returns true. This throws error even > when we have not used the -l and --log-prefix option as shown below. > > $ pgbench -T 50 > log file prefix (--log-prefix) is allowed only when logging transactions > (-l) >
Thanks. Attached latest patch. 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..318b132 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 = NULL; 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); @@ -4388,11 +4401,13 @@ threadRun(void *arg) if (use_log) { char logpath[64]; + char *prefix = logfile_prefix ? logfile_prefix : "pgbench_log"; if (thread->tid == 0) - snprintf(logpath, sizeof(logpath), "pgbench_log.%d", main_pid); + snprintf(logpath, sizeof(logpath), "%s.%d", prefix, main_pid); else - snprintf(logpath, sizeof(logpath), "pgbench_log.%d.%d", main_pid, thread->tid); + snprintf(logpath, sizeof(logpath), "%s.%d.%d", 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