Specifying the log file name of pgbench -l option
Hi all,
The log file generated by pgbench -l option is fixed file name
'pgbench_log.<pid>.<thread id>'. And it's a little complicated for the
script that runs pgbench repeatedly to identify the log file name.
Attached patch make it possible to specify the log file name. I think
it's useful for the use who want to run pgbench repeatedly in script
and collects and analyze the result.
The one thing I concern is that this patch changes -l option so that
it requires argument.
But changing its behavior would be good rather than adding new option.
Please give me feedback.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
pgbench_log_file_name.patchbinary/octet-stream; name=pgbench_log_file_name.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 285608d..b0a9987 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -351,11 +351,11 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
</varlistentry>
<varlistentry>
- <term><option>-l</option></term>
- <term><option>--log</option></term>
+ <term><option>-l</> <replaceable>filename</></term>
+ <term><option>--log</> <replaceable>filename</></term>
<listitem>
<para>
- Write the time taken by each transaction to a log file.
+ Write the time taken by each transaction to <replaceable>filename</>.
See below for details.
</para>
</listitem>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d44cfda..9f3ae53 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;
const char *progname;
#define WSEP '@' /* weight separator */
@@ -496,7 +497,7 @@ usage(void)
" -D, --define=VARNAME=VALUE\n"
" define variable for use by custom script\n"
" -j, --jobs=NUM number of threads (default: 1)\n"
- " -l, --log write transaction times to log file\n"
+ " -l, --log=FILENAME write transaction times to log file\n"
" -L, --latency-limit=NUM count transactions lasting more than NUM ms as late\n"
" -M, --protocol=simple|extended|prepared\n"
" protocol for submitting queries (default: simple)\n"
@@ -3709,7 +3710,7 @@ main(int argc, char **argv)
state = (CState *) pg_malloc(sizeof(CState));
memset(state, 0, sizeof(CState));
- while ((c = getopt_long(argc, argv, "ih:nvp:dqb:SNc:j:Crs:t:T:U:lf:D:F:M:P:R:L:", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "ih:nvp:dqb:SNc:j:Crs:t:T:U:l:f:D:F:M:P:R:L:", long_options, &optindex)) != -1)
{
char *script;
@@ -3830,6 +3831,7 @@ main(int argc, char **argv)
break;
case 'l':
benchmarking_option_set = true;
+ logfilename = pg_strdup(optarg);
use_log = true;
break;
case 'q':
@@ -4390,9 +4392,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, main_pid);
else
- snprintf(logpath, sizeof(logpath), "pgbench_log.%d.%d", main_pid, thread->tid);
+ snprintf(logpath, sizeof(logpath), "%s.%d.%d", logfile, main_pid, thread->tid);
+
thread->logfile = fopen(logpath, "w");
if (thread->logfile == NULL)
The log file generated by pgbench -l option is fixed file name
'pgbench_log.<pid>.<thread id>'. And it's a little complicated for the
script that runs pgbench repeatedly to identify the log file name.
Attached patch make it possible to specify the log file name. I think
it's useful for the use who want to run pgbench repeatedly in script
and collects and analyze the result.The one thing I concern is that this patch changes -l option so that
it requires argument.
But changing its behavior would be good rather than adding new option.Please give me feedback.
Patch applies but does not compile, because "logfilename" is not declared.
I guess "logfile" was meant instead.
I understand and agree that in some case having only a predefined file
prefix in the current directory as the only option can be a hindrance for
scripts which use pgbench and rely on the log.
I'm not at ease either with changing the behavior of such an option, as
some people may be happy with it and some script may be using it. I would
suggest not to do so.
Moreover, what is provided is not a file name, but a prefix used to build
file names.
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
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Nov 2, 2016 at 1:41 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
The log file generated by pgbench -l option is fixed file name
'pgbench_log.<pid>.<thread id>'. And it's a little complicated for the
script that runs pgbench repeatedly to identify the log file name.
Attached patch make it possible to specify the log file name. I think
it's useful for the use who want to run pgbench repeatedly in script
and collects and analyze the result.The one thing I concern is that this patch changes -l option so that
it requires argument.
But changing its behavior would be good rather than adding new option.Please give me feedback.
Patch applies but does not compile, because "logfilename" is not declared.
I guess "logfile" was meant instead.I understand and agree that in some case having only a predefined file
prefix in the current directory as the only option can be a hindrance for
scripts which use pgbench and rely on the log.I'm not at ease either with changing the behavior of such an option, as some
people may be happy with it and some script may be using it. I would suggest
not to do so.Moreover, what is provided is not a file name, but a prefix used to build
file names.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
Thank you for reviewing this patch!
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.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
pgbench_log_file_name_v2.patchapplication/x-patch; name=pgbench_log_file_name_v2.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 285608d..100cdb0 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -614,6 +614,16 @@ pgbench <optional> <replaceable>options</> </optional> <replaceable>dbname</>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--log-prefix=<replaceable>prefix</></option></term>
+ <listitem>
+ <para>
+ Prefix of transaction log file. If this option is not given,
+ <replaceable>pgbench_log</replaceable> is used.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
@@ -1121,15 +1131,17 @@ 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
+ <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
<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>,
- where <replaceable>mmm</> is a sequential number for each worker starting
- with 1.
+ <filename><replaceable>pgbench_log</replaceable>.<replaceable>nnn</>.<replaceable>mmm</></filename>,
+ where <replaceable>mmm</> is a sequential number
+ for each worker starting with 1.
</para>
<para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index d44cfda..32ffdff 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;
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,9 @@ main(int argc, char **argv)
progress_timestamp = true;
benchmarking_option_set = true;
break;
+ case 7:
+ logfile_prefix = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -4087,6 +4093,12 @@ main(int argc, char **argv)
exit(1);
}
+ if (!use_log && logfile_prefix)
+ {
+ fprintf(stderr, "log file prefix (--log-prefix) is allowed only when actually logging transactions\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 +4400,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)
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 prefixI 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.
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.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hello,
On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho@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 prefixI 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 whereIt 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)
Thanks,
Beena Emerson
Have a Great Day!
On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemerson@gmail.com> wrote:
Hello,
On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho@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 prefixI 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 whereIt 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
transactionspgbench --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
Attachments:
pgbench_log_file_name_v3.patchtext/x-diff; charset=US-ASCII; name=pgbench_log_file_name_v3.patchDownload
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)
Hello Sawada-san,
On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemerson@gmail.com>
wrote:Hello,
On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho@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 prefixI 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 whereIt 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
transactionspgbench --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)
Kindly fix this.
Thank you,
--
Beena Emerson
Have a Great Day!
On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <memissemerson@gmail.com> wrote:
Hello Sawada-san,
On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada.mshk@gmail.com>
wrote:On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemerson@gmail.com>
wrote:Hello,
On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho@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 prefixI 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 whereIt 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
transactionspgbench --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
Attachments:
pgbench_log_file_name_v4.patchtext/x-diff; charset=US-ASCII; name=pgbench_log_file_name_v4.patchDownload
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)
Hello,
On Mon, Nov 7, 2016 at 8:42 PM, Masahiko Sawada <sawada.mshk@gmail.com>
wrote:
On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <memissemerson@gmail.com>
wrote:Hello Sawada-san,
On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada.mshk@gmail.com>
wrote:On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemerson@gmail.com>
wrote:Hello,
On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho@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 prefixI 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 whenthe
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 whereIt 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
transactionspgbench --sampling-rate=1 -t 20
log sampling (--sampling-rate) is allowed only when loggingtransactions
(-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 erroreven
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.
Thank you for the update.
I checked. It works as expected. I have no more comments.
If its okay with Fabien, we can mark it ready for committer.
Thanks,
Beena Emerson
Have a Great Day!
[ ... v4 ]
I checked. It works as expected. I have no more comments.
If its okay with Fabien, we can mark it ready for committer.
Patch applies, compiles (including the documentation), make check ok and
features works for me. Code could be a little simpler, but it is okay.
I've switched the status to "ready for committers".
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Nov 8, 2016 at 4:04 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
[ ... v4 ]
I checked. It works as expected. I have no more comments.
If its okay with Fabien, we can mark it ready for committer.Patch applies, compiles (including the documentation), make check ok and
features works for me. Code could be a little simpler, but it is okay. I've
switched the status to "ready for committers".
Committed with some wordsmithing and cosmetic tweaks.
--
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