From: | Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, PostgreSQL Developers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: extend pgbench expressions with functions |
Date: | 2016-03-08 16:48:11 |
Message-ID: | alpine.DEB.2.10.1603081733330.23770@sto |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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.
From | Date | Subject | |
---|---|---|---|
Next Message | Pavel Stehule | 2016-03-08 17:01:36 | Re: raw output from copy |
Previous Message | Andreas Joseph Krogh | 2016-03-08 16:45:14 | Re: Exclude pg_largeobject form pg_dump |