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 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)