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

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.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center