Re: Specifying the log file name of pgbench -l option

Hello,

On Mon, Nov 7, 2016 at 8:42 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Mon, Nov 7, 2016 at 10:52 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
> wrote:
> > Hello Sawada-san,
> >
> > On Mon, Nov 7, 2016 at 4:47 PM, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> > wrote:
> >>
> >> On Wed, Nov 2, 2016 at 3:57 PM, Beena Emerson <memissemerson(at)gmail(dot)com>
> >> wrote:
> >> > Hello,
> >> >
> >> > On Wed, Nov 2, 2016 at 4:16 AM, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)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!