Re: CPU costs of random_zipfian in pgbench - Mailing list pgsql-hackers

Hello Tom,

> I started to look through this, and the more I looked the more unhappy
> I got that we're having this discussion at all.  The zipfian support
> in pgbench is seriously over-engineered and under-documented.  As an
> example, I was flabbergasted to find out that the end-of-run summary
> statistics now include this:
>
>    /* Report zipfian cache overflow */
>    for (i = 0; i < nthreads; i++)
>    {
>        totalCacheOverflows += threads[i].zipf_cache.overflowCount;
>    }
>    if (totalCacheOverflows > 0)
>    {
>        printf("zipfian cache array overflowed %d time(s)\n", totalCacheOverflows);
>    }
>
> What is the point of that, and if there is a point, why is it nowhere
> mentioned in pgbench.sgml?

Indeed, there should.

> What would a user do with this information, and how would they know what 
> to do?

Sure, but it was unclear what to do. Extending the cache to avoid that 
would look like over-engineering.

> I remain of the opinion that we ought to simply rip out support for
> zipfian with s < 1.

Some people really want zipfian because it reflects their data access 
pattern, possibly with s < 1.

We cannot helpt it: real life seems zipfianly distributed:-)

> It's not useful for benchmarking purposes to have a random-number 
> function with such poor computational properties.

This is mostly a startup cost, the generation cost when a bench is running 
is reasonable. How to best implement the precomputation is an open 
question.

As a reviewer I was not thrilled by the cache stuff, but I had no better 
idea that would not fall under "over-over engineering" or the like.

Maybe it could error out and say "recompile me", but then someone
would have said "that is unacceptable".

Maybe it could auto extend the cache, but that is still more 
unnecessary over-engineering, IMHO.

Maybe a there could be some mandatory declarations or partial eval that 
could precompute the needed parameters out/before the bench is started, 
with a clear message "precomputing stuff...", but that would be over over 
over engineering again... and that would mean restricting random_zipfian 
parameters to near-constants, which would require some explaining, but 
maybe it is an option. I guess that in the paper original context, the 
parameters (s & n) are known before the bench is started, so that the 
needed value are computed offline once and for all.

> I think leaving it in there is just a foot-gun: we'd be a lot better off 
> throwing an error that tells people to use some other distribution.

When s < 1, the startup cost is indeed a pain. However, it is a pain 
prescribed by a Turing Award.

> Or if we do leave it in there, we for sure have to have documentation
> that *actually* explains how to use it, which this patch still doesn't.

I'm not sure what explaining there could be about how to use it: one calls 
the function to obtain pseudo-random integers with the desired 
distribution?

> There's nothing suggesting that you'd better not use a large number of
> different (n,s) combinations.

Indeed, there is no caveat about this point, as noted above.

Please find an updated patch for the documentation, pointing out the 
existence of the cache and an advice not to overdo it.

It does not solve the underlying problem which raised your complaint, but 
at least it is documented.

-- 
Fabien.