Re: [HACKERS] WIP Patch: Pgbench Serialization and deadlock errors

Hello Marina,

> v10-0002-Pgbench-errors-use-a-separate-function-to-report.patch
> - a patch for a separate error reporting function (this is used to report
> client failures that do not cause an aborts and this depends on the level of
> debugging).

Patch applies cleanly, compiles, global & local make check ok.

This patch improves/homogenizes logging & error reporting in pgbench, in
preparation for another patch which will manage transaction restarts in
some cases.

However ISTM that it is not as necessary as the previous one, i.e. we
could do without it to get the desired feature, so I see it more as a
refactoring done "in passing", and I'm wondering whether it is
really worth it because it adds some new complexity, so I'm not sure of
the net benefit.

Anyway, I still have quite a few comments/suggestions on this version.

* ErrorLevel

If ErrorLevel is used for things which are not errors, its name should not
include "Error"? Maybe "LogLevel"?

I'm at odds with the proposed levels. ISTM that pgbench internal errors
which warrant an immediate exit should be dubbed "FATAL", which would
leave the "ERROR" name for... errors, eg SQL errors. I'd suggest to use an
INFO level for the PGBENCH_DEBUG function, and to keep LOG for main
program messages, so that all use case are separate. Or, maybe the
distinction between LOG/INFO is unclear so info is not necessary.

I'm unsure about the "log_min_messages" variable name, I'd suggest
"log_level".

I do not see the asserts on LOG >= log_min_messages as useful, because the
level can only be LOG or DEBUG anyway.

This point also suggest that maybe "pgbench_error" is misnamed as well
(ok, I know I suggested it in place of ereport, but e stands for error
there), as it is called on errors, but is also on other things. Maybe
"pgbench_log"? Or just simply "log" or "report", as it is really an local
function, which does not need a prefix? That would mean that
"pgbench_simple_error", which is indeed called on errors, could keep its
initial name "pgbench_error", and be called on errors.

Alternatively, the debug/logging code could be let as it is (i.e. direct
print to stderr) and the function only called when there is some kind of
error, in which case it could be named with "error" in its name (or
elog/ereport...).

* PQExpBuffer

I still do not see a positive value from importing PQExpBuffer complexity
and cost into pgbench, as the resulting code is not very readable and it
adds malloc/free cycles, so I'd try to avoid using PQExpBuf as much as
possible. ISTM that all usages could be avoided in the patch, and most
should be avoided even if ExpBuffer is imported because it is really
useful somewhere.

- to call pgbench_error from pgbench_simple_error, you can do a
pgbench_log_va(level, format, va_list) version called both from
pgbench_error & pgbench_simple_error.

- for PGBENCH_DEBUG function, do separate calls per type, the
very small partial code duplication is worth avoiding ExpBuf IMO.

- for doCustom debug: I'd just let the printf as it is, with a comment, as
it is really very internal stuff for debug. Or I'd just snprintf a
something in a static buffer.

- for syntax_error: it should terminate, so it should call
pgbench_error(FATAL, ...). Idem, I'd either keep the printf then call
pgbench_error(FATAL, "syntax error found\n") for a final message,
or snprintf in a static buffer.

- for listAvailableScript: I'd simply call "pgbench_error(LOG" several
time, once per line.

I see building a string with a format (printfExpBuf..) and then calling
the pgbench_error function with just a "%s" format on the result as not
very elegant, because the second format is somehow hacked around.

* bool client

I'm unconvince by this added boolean just to switch the level on
encountered errors.

I'd suggest to let lookupCreateVariable, putVariable* as they are, call
pgbench_error with a level which does not stop the execution, and abort if
necessary from the callers with a "aborted because of putVariable/eval/...
error" message, as it was done before.

pgbench_error calls pgbench_error. Hmmm, why not.

--
Fabien.