On Tue, Feb 9, 2016 at 4:22 AM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
>> + /* compute total_weight */
>> + for (i = 0; i < num_scripts; i++)
>> + {
>> + total_weight += sql_script[i].weight;
>> +
>> + /* detect overflow... */
>> If let as int64, you may want to remove this overflow check, or keep
>> it as int32.
>
>
> I'd rather keep int64, and remove the check.
OK, and you did so. Thanks.
>>> [JSON/YAML]
>>> If you want something else, it would help to provide a sample of what you
>>> expect.
>>
>> You could do that with an additional option here as well:
>> --output-format=normal|yamljson. The tastes of each user is different.
>
> I think that json/yaml-ifying pgbench output is beyond the object of this
> patch, so should be kept out?
Yeah, that's just a free idea that this set of patches does not need
to address. If someone thinks that's worth it, feel free to submit a
patch, perhaps we could add a TODO item on the wiki. Regarding the
output generated by your patch, I think that's fine. Perhaps Alvaro
has other thoughts on the matter. I don't know this part.
>>> Find attached a 18-d which addresses these concerns, and a actualized
>>> 18-e
>>> for the prefix.
>>
>>
>> (I could live without that personally)
>
> Hmmm, I type them and I'm not so good with a keyboard, so "se" is better
> than:
>
> "selct-only<back-back-back-back-back-back-back-back>ect-only".
I can understand that feeling.
>> -/* return builtin script "name", or fail if not found */
>> +/* return commands for selected builtin script, if unambiguous */
>> static script_t *
>> findBuiltin(const char *name)
>> This comment needs a refresh. This does not return a set of commands,
>> but the script itself.
>
> Indeed.
>
> Attached 19-d and 19-e.
+/* return builtin script "name", or fail if not found */
builtin does not sound like correct English to me, but built-in is.
--
Michael