Bonjour Michaël,
>> Without looking at the context I thought that argv[0] was the program name,
>> which is not the case here. I put it back everywhere, including the DEBUG
>> message.
>
> The variable names in Command are confusing IMO...
Somehow, yes. Note that there is a logic, it will indeed be the argv of
the called shell command… And I do not think it is the point of this patch
to solve this possible confusion.
> + pg_log_error("gaussian parameter must be at least "
> + "%f (not %f)", MIN_GAUSSIAN_PARAM, param);
> I would keep all the error message strings to be on the same line.
> That makes grepping for them easier on the same, and that's the usual
> convention even if these are larger than 72-80 characters.
Ok. I also did other similar cases accordingly.
> #ifdef DEBUG
> Worth removing this ifdef?
Yep, especially as it is the only instance. Done.
> + pg_log_fatal("unexpected copy in result");
> + pg_log_error("%s", PQerrorMessage(con));
> + pg_log_fatal("cannot count number of branches");
> + pg_log_error("%s", PQerrorMessage(con));
> These are inconsistent with the rest, why not combining both?
Ok, done.
> I think that I would just remove the "debug" variable defined in
> pgbench.c all together, and switch the messages for the duration and the
> one in executeMetaCommand to use info-level logging..
Ok, done.
Patch v4 attached addresses all these points.
--
Fabien.
