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.
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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
