On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <[email protected]> wrote:
> Hello Sawada-san,
>
> On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <[email protected]>
> wrote:
>>
>> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <[email protected]>
>> wrote:
>> > Hello,
>> >
>> > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <[email protected]>
>> > 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
