From: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
---|---|
To: | Beena Emerson <memissemerson(at)gmail(dot)com> |
Cc: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Specifying the log file name of pgbench -l option |
Date: | 2016-11-07 11:17:29 |
Message-ID: | CAD21AoD5Ang=xk02LGpSoDcUT6hP3jYt8DxymgEVEu-p-7qZoA@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
pgbench_log_file_name_v3.patch | text/x-diff | 4.4 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Daniel Gustafsson | 2016-11-07 11:32:55 | Re: Radix tree for character conversion |
Previous Message | Albe Laurenz | 2016-11-07 10:02:59 | Re: Add PGDLLEXPORT to PG_FUNCTION_INFO_V1 |