Hello Sawada-san, On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemer...@gmail.com> > wrote: > > 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) > > > > 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!
