Hello Robert,
> 1. It's really not appropriate to fold the documentation changes
> raised on the other thread into this patch. I'm not going to commit
> something where the commit message is a laundry list of unrelated
> changes. Please separate out the documentation changes as a separate
> patch. Let's do that first, and then make this patch apply on top of
> those changes. The related changes in getGaussianRand etc. should
> also be part of that patch, not this one.
Hmmm. Attached is a two-part v16.
> 2. Please reduce the churn in the pgbench output example. Most of the
> lines that you've changed don't actually need to be changed.
I did a real run to get consistant figures, especially as now more
informations are shown. I did some computations to try to generate
something consistent without changing too many lines, but just taking a
real output would make more sense.
> 3. I think we should not have ENODE_INTEGER_CONSTANT and
> ENODE_DOUBLE_CONSTANT. We should just have ENODE_CONSTANT, and it
> should store the same datatype we use to represent values in the
> evaluator.
Why not. This induces a two level tag structure, which I do not find that
great, but it simplifies the evaluator code to have one less case.
> 4. The way you've defined the value type is not great. int64_t isn't
> used anywhere in our source base. Don't start here.
Indeed. I really meant "int64" which is used elsewhere in pgbench.
> I think the is_none, is_int, is_double naming is significantly inferior
> to what I suggested, and unlike what we do through the rest of our code.
Hmmm. I think that it is rather a matter of taste, and PGBT_DOUBLE does
not strike me as intrinsically superior, although possibly more in style.
I changed value_t and value_type_t to PgBenchValue and ~Type to blend in,
even if I find it on the heavy side.
> Similarly, the coercion functions are not very descriptive named,
I do not understand. "INT(value)" and "DOUBLE(value) both look pretty
explicit to me...
The point a choosing short names is to avoid newlines to stay (or try to
stay) under 80 columns, especially as pg indentations rules tend to move
things quickly on the right in case constructs, which are used in the
evaluation function.
I use "explicitely named" functions, but used short macros in the
evaluator.
> and I don't see why we'd want those to be macros rather than static
> functions.
Can be functions, sure. Switched.
> 5. The declaration of PgBenchExprList has a cuddled opening brace,
> which is not PostgreSQL style.
Indeed. There were other instances.
--
Fabien.
