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) Thanks, Beena Emerson Have a Great Day!
