Improve histogram-filling loop in new compute_array_stats() code.

Do "frac" arithmetic in int64 to prevent overflow with large statistics
targets, and improve the comments so people have some chance of
understanding how it works.

Alexander Korotkov and Tom Lane

diff --git a/src/backend/utils/adt/array_typanalyze.c b/src/backend/utils/adt/array_typanalyze.c
index 941e2adb0384795d2af2d1070c17198bb462fc0a..ba9873905e2ecf4da57d8258f23699b5b1284ccb 100644 (file)
--- a/src/backend/utils/adt/array_typanalyze.c
+++ b/src/backend/utils/adt/array_typanalyze.c
@@ -579,9 +579,9 @@ compute_array_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc,
        {
            int         num_hist = stats->attr->attstattarget;
            DECountItem **sorted_count_items;
-           int         count_item_index;
+           int         j;
            int         delta;
-           int         frac;
+           int64       frac;
            float4     *hist;
 
            /* num_hist must be at least 2 for the loop below to work */
@@ -594,45 +594,70 @@ compute_array_stats(VacAttrStats *stats, AnalyzeAttrFetchFunc fetchfunc,
            sorted_count_items = (DECountItem **)
                palloc(sizeof(DECountItem *) * count_items_count);
            hash_seq_init(&scan_status, count_tab);
-           count_item_index = 0;
+           j = 0;
            while ((count_item = (DECountItem *) hash_seq_search(&scan_status)) != NULL)
            {
-               sorted_count_items[count_item_index++] = count_item;
+               sorted_count_items[j++] = count_item;
            }
            qsort(sorted_count_items, count_items_count,
                  sizeof(DECountItem *), countitem_compare_count);
 
            /*
-            * Fill stanumbers with the histogram, followed by the average
-            * count.  This array must be stored in anl_context.
+            * Prepare to fill stanumbers with the histogram, followed by the
+            * average count.  This array must be stored in anl_context.
             */
            hist = (float4 *)
                MemoryContextAlloc(stats->anl_context,
                                   sizeof(float4) * (num_hist + 1));
            hist[num_hist] = (double) element_no / (double) nonnull_cnt;
 
-           /*
-            * Construct the histogram.
+           /*----------
+            * Construct the histogram of distinct-element counts (DECs).
+            *
+            * The object of this loop is to copy the min and max DECs to
+            * hist[0] and hist[num_hist - 1], along with evenly-spaced DECs
+            * in between (where "evenly-spaced" is with reference to the
+            * whole input population of arrays).  If we had a complete sorted
+            * array of DECs, one per analyzed row, the i'th hist value would
+            * come from DECs[i * (analyzed_rows - 1) / (num_hist - 1)]
+            * (compare the histogram-making loop in compute_scalar_stats()).
+            * But instead of that we have the sorted_count_items[] array,
+            * which holds unique DEC values with their frequencies (that is,
+            * a run-length-compressed version of the full array).  So we
+            * control advancing through sorted_count_items[] with the
+            * variable "frac", which is defined as (x - y) * (num_hist - 1),
+            * where x is the index in the notional DECs array corresponding
+            * to the start of the next sorted_count_items[] element's run,
+            * and y is the index in DECs from which we should take the next
+            * histogram value.  We have to advance whenever x <= y, that is
+            * frac <= 0.  The x component is the sum of the frequencies seen
+            * so far (up through the current sorted_count_items[] element),
+            * and of course y * (num_hist - 1) = i * (analyzed_rows - 1),
+            * per the subscript calculation above.  (The subscript calculation
+            * implies dropping any fractional part of y; in this formulation
+            * that's handled by not advancing until frac reaches 1.)
             *
-            * XXX this needs work: frac could overflow, and it's not clear
-            * how or why the code works.  Even if it does work, it needs
-            * documented.
+            * Even though frac has a bounded range, it could overflow int32
+            * when working with very large statistics targets, so we do that
+            * math in int64.
+            *----------
             */
            delta = analyzed_rows - 1;
-           count_item_index = 0;
-           frac = sorted_count_items[0]->frequency * (num_hist - 1);
+           j = 0;              /* current index in sorted_count_items */
+           /* Initialize frac for sorted_count_items[0]; y is initially 0 */
+           frac = (int64) sorted_count_items[0]->frequency * (num_hist - 1);
            for (i = 0; i < num_hist; i++)
            {
                while (frac <= 0)
                {
-                   count_item_index++;
-                   Assert(count_item_index < count_items_count);
-                   frac += sorted_count_items[count_item_index]->frequency * (num_hist - 1);
+                   /* Advance, and update x component of frac */
+                   j++;
+                   frac += (int64) sorted_count_items[j]->frequency * (num_hist - 1);
                }
-               hist[i] = sorted_count_items[count_item_index]->count;
-               frac -= delta;
+               hist[i] = sorted_count_items[j]->count;
+               frac -= delta;      /* update y for upcoming i increment */
            }
-           Assert(count_item_index == count_items_count - 1);
+           Assert(j == count_items_count - 1);
 
            stats->stakind[slot_idx] = STATISTIC_KIND_DECHIST;
            stats->staop[slot_idx] = extra_data->eq_opr;