Re: CPU costs of random_zipfian in pgbench

[Fabien COELHO]

Hello again,
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?

The attached patch simplifies the code by erroring on cache overflow, 
instead of the LRU replacement strategy and unhelpful final report. The 
above lines are removed.

--
Fabien.diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 4789ab92ee..bbec36e559 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -422,8 +422,6 @@ typedef struct
 	double		alpha;
 	double		beta;
 	double		eta;
-
-	uint64		last_used;		/* last used logical time */
 } ZipfCell;
 
 /*
@@ -431,10 +429,7 @@ typedef struct
  */
 typedef struct
 {
-	uint64		current;		/* counter for LRU cache replacement algorithm */
-
 	int			nb_cells;		/* number of filled cells */
-	int			overflowCount;	/* number of cache overflows */
 	ZipfCell	cells[ZIPF_CACHE_SIZE];
 } ZipfCache;
 
@@ -1018,8 +1013,7 @@ zipfSetCacheCell(ZipfCell *cell, int64 n, double s)
 static ZipfCell *
 zipfFindOrCreateCacheCell(ZipfCache *cache, int64 n, double s)
 {
-	int			i,
-				least_recently_used = 0;
+	int			i;
 	ZipfCell   *cell;
 
 	/* search cached cell for given parameters */
@@ -1028,24 +1022,22 @@ zipfFindOrCreateCacheCell(ZipfCache *cache, int64 n, double s)
 		cell = &cache->cells[i];
 		if (cell->n == n && cell->s == s)
 			return &cache->cells[i];
+	}
 
-		if (cell->last_used < cache->cells[least_recently_used].last_used)
-			least_recently_used = i;
+	/* cache overflow */
+	if (cache->nb_cells >= ZIPF_CACHE_SIZE)
+	{
+		fprintf(stderr,
+				"Zipf precomputed constants cache overflow: only %d slots are available for\n"
+				"(param, size) pairs when param is in (0, 1).  Recompile pgbench for more.\n",
+				ZIPF_CACHE_SIZE);
+		exit(1);
 	}
 
 	/* create new one if it does not exist */
-	if (cache->nb_cells < ZIPF_CACHE_SIZE)
-		i = cache->nb_cells++;
-	else
-	{
-		/* replace LRU cell if cache is full */
-		i = least_recently_used;
-		cache->overflowCount++;
-	}
-
+	i = cache->nb_cells++;
 	zipfSetCacheCell(&cache->cells[i], n, s);
 
-	cache->cells[i].last_used = cache->current++;
 	return &cache->cells[i];
 }
 
@@ -5012,8 +5004,7 @@ printResults(TState *threads, StatsData *total, instr_time total_time,
 				tps_include,
 				tps_exclude;
 	int64		ntx = total->cnt - total->skipped;
-	int			i,
-				totalCacheOverflows = 0;
+	int			i;
 
 	time_include = INSTR_TIME_GET_DOUBLE(total_time);
 
@@ -5041,15 +5032,6 @@ printResults(TState *threads, StatsData *total, instr_time total_time,
 		printf("number of transactions actually processed: " INT64_FORMAT "\n",
 			   ntx);
 	}
-	/* 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);
-	}
 
 	/* Remaining stats are nonsensical if we failed to execute any xacts */
 	if (total->cnt <= 0)
@@ -5938,8 +5920,6 @@ main(int argc, char **argv)
 		thread->logfile = NULL; /* filled in later */
 		thread->latency_late = 0;
 		thread->zipf_cache.nb_cells = 0;
-		thread->zipf_cache.current = 0;
-		thread->zipf_cache.overflowCount = 0;
 		initStats(&thread->stats, 0);
 
 		nclients_dealt += thread->nstate;
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 45888dc12e..59d221eb7f 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -817,9 +817,9 @@ for my $e (@errors)
 
 # zipfian cache array overflow
 pgbench(
-	'-t 1', 0,
-	[ qr{processed: 1/1}, qr{zipfian cache array overflowed 1 time\(s\)} ],
-	[qr{^}],
+	'-t 1', 1,
+	[ qr{^$} ],
+	[ qr{cache overflow} ],
 	'pgbench zipfian array overflow on random_zipfian',
 	{
 		'001_pgbench_random_zipfian' => q{