Hello Robert,
Having a look at 33-b, this looks pretty good now, but:
// comments are not allowed. I'd just remove the two you have.
Oops, C89 did not make it everywhere yet!
It make no sense to exit(1) and then return 0, so don't do that. [...]
This would get rid of the internal-error case here altogether in favor
of testing it via an assertion.
Ok. Fine with me.
I think that coerceToInt() should not exit(1) when an overflow occurs;
instead, I think the function should be declared as bool
coerceToInt(PgBenchValue *pval, int64 *result), and the error return
should be propagated back to the caller, which can then return false
as we do for other error cases.
This is the pain (ugly repeated code) that I wish to avoid, because then
we cannot write a simple addition for doing an addition, but have to do
several ugly tests instead. Ok, elegance is probably not a sufficient
argument against doing that.
Moreover, I do not see any condition in which doing what you suggest makes
much sense from the user perspective: if there is an internal error in the
bench code it seems much more logical to ask for the user for a sensible
bench script, because I would not know how to interpret tps on a bench
with internal failures in the client code anyway.
For me, exiting immediatly on internal script errors is the right action.
If this is a blocker I'll do them, but I'm convince it is not what should
be done.
I think that some of the places you've used coerceToInt() are not
appropriate. Like, for example:
+ if
(coerceToInt(rval) == 0)
+
fprintf(stderr, "division by zero\n");
+ return false;
+ }
Now, if rval is out of range of an integer, that is going to overflow
while trying to see whether it should divide by zero. Please work a
little harder here and in similar cases.
Ok.
Maybe add a helper function
checkIntegerEquality(PgBenchValue *, int64).
Maybe.
--
Fabien.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
