Hello, On Mon, Nov 7, 2016 at 8:42 PM, Masahiko Sawada <sawada.m...@gmail.com> wrote:
> On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <memissemer...@gmail.com> > wrote: > > 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) > > > > Thanks. > Attached latest patch. > Please review it. > Thank you for the update. I checked. It works as expected. I have no more comments. If its okay with Fabien, we can mark it ready for committer. Thanks, Beena Emerson Have a Great Day!
