I think I just solved a longstanding bug in the rrd_update routine

... I am writing a tool for seeding new rrd files from an existing
one ... essentially enabeling arbitrary restructuring ...

while doing this I found the following:

pdp:     1 2 | 3 4 5 6 7 | 8 9

for a consolidation of 5 steps I get

AVG RRA  ..  |         5 | ..
MIN RRA  ..  |         2 | ..
MAX RRA  ..  |         7 | ..

while I would have expected to get

AVG RRA  ..  |         5 | ..
MIN RRA  ..  |         3 | ..
MAX RRA  ..  |         7 | ..

the behavior has been like this at least since 1.2

looking at the code I found that the behavior had been introduced
by a patch optimizing rrd_update for updates covering multiple pdps
in one go ... in that optimization, the normal case where an update
covers exactly ONE pdp had been sort of neglected ... so much for
tunel vision.

The problem was, that the cdp value for MIN and MAX got initialized
from the last pdp value BEFORE the consolidation interval ... for
normal data no one would notice (or did notice) but it is still
wrong ...

git-svn-id: svn://svn.oetiker.ch/rrdtool/trunk/program@2024 a5681a0c-68f1-0310-ab6d-d61299d08faa

diff --git a/src/rrd_update.c b/src/rrd_update.c
index ec1e5fd..83ef706 100644 (file)
--- a/src/rrd_update.c
+++ b/src/rrd_update.c
@@ -212,7 +212,6 @@ static void initialize_cdp_val(
     unival *scratch,
     int current_cf,
     rrd_value_t pdp_temp_val,
-    unsigned long elapsed_pdp_st,
     unsigned long start_pdp_offset,
     unsigned long pdp_cnt);
 
@@ -227,8 +226,9 @@ static void reset_cdp(
     int cdp_idx,
     enum cf_en current_cf);
 
-static rrd_value_t initialize_average_carry_over(
+static rrd_value_t initialize_carry_over(
     rrd_value_t pdp_temp_val,
+    int         current_cf,
     unsigned long elapsed_pdp_st,
     unsigned long start_pdp_offset,
     unsigned long pdp_cnt);
@@ -1593,18 +1593,15 @@ static void update_cdp(
 
         if (*cdp_unkn_pdp_cnt > pdp_cnt * xff) {
             *cdp_primary_val = DNAN;
-            if (current_cf == CF_AVERAGE) {
-                *cdp_val =
-                    initialize_average_carry_over(pdp_temp_val,
-                                                  elapsed_pdp_st,
-                                                  start_pdp_offset, pdp_cnt);
-            } else {
-                *cdp_val = pdp_temp_val;
-            }
         } else {
             initialize_cdp_val(scratch, current_cf, pdp_temp_val,
-                               elapsed_pdp_st, start_pdp_offset, pdp_cnt);
-        }               /* endif meets xff value requirement for a valid value */
+                               start_pdp_offset, pdp_cnt);
+        }
+        *cdp_val =
+            initialize_carry_over(pdp_temp_val,current_cf,
+                                  elapsed_pdp_st,
+                                  start_pdp_offset, pdp_cnt);
+               /* endif meets xff value requirement for a valid value */
         /* initialize carry over CDP_unkn_pdp_cnt, this must after CDP_primary_val
          * is set because CDP_unkn_pdp_cnt is required to compute that value. */
         if (isnan(pdp_temp_val))
@@ -1640,7 +1637,6 @@ static void initialize_cdp_val(
     unival *scratch,
     int current_cf,
     rrd_value_t pdp_temp_val,
-    unsigned long elapsed_pdp_st,
     unsigned long start_pdp_offset,
     unsigned long pdp_cnt)
 {
@@ -1653,13 +1649,11 @@ static void initialize_cdp_val(
         scratch[CDP_primary_val].u_val =
             (cum_val + cur_val * start_pdp_offset) /
             (pdp_cnt - scratch[CDP_unkn_pdp_cnt].u_cnt);
-        scratch[CDP_val].u_val =
-            initialize_average_carry_over(pdp_temp_val, elapsed_pdp_st,
-                                          start_pdp_offset, pdp_cnt);
         break;
-    case CF_MAXIMUM:
+    case CF_MAXIMUM: 
         cum_val = IFDNAN(scratch[CDP_val].u_val, -DINF);
         cur_val = IFDNAN(pdp_temp_val, -DINF);
+
 #if 0
 #ifdef DEBUG
         if (isnan(scratch[CDP_val].u_val) && isnan(pdp_temp)) {
@@ -1674,8 +1668,6 @@ static void initialize_cdp_val(
             scratch[CDP_primary_val].u_val = cur_val;
         else
             scratch[CDP_primary_val].u_val = cum_val;
-        /* initialize carry over value */
-        scratch[CDP_val].u_val = pdp_temp_val;
         break;
     case CF_MINIMUM:
         cum_val = IFDNAN(scratch[CDP_val].u_val, DINF);
@@ -1694,14 +1686,10 @@ static void initialize_cdp_val(
             scratch[CDP_primary_val].u_val = cur_val;
         else
             scratch[CDP_primary_val].u_val = cum_val;
-        /* initialize carry over value */
-        scratch[CDP_val].u_val = pdp_temp_val;
         break;
     case CF_LAST:
     default:
         scratch[CDP_primary_val].u_val = pdp_temp_val;
-        /* initialize carry over value */
-        scratch[CDP_val].u_val = pdp_temp_val;
         break;
     }
 }
@@ -1763,17 +1751,34 @@ static void reset_cdp(
     }
 }
 
-static rrd_value_t initialize_average_carry_over(
+static rrd_value_t initialize_carry_over(
     rrd_value_t pdp_temp_val,
+    int current_cf,
     unsigned long elapsed_pdp_st,
     unsigned long start_pdp_offset,
     unsigned long pdp_cnt)
 {
-    /* initialize carry over value */
-    if (isnan(pdp_temp_val)) {
-        return DNAN;
-    }
-    return pdp_temp_val * ((elapsed_pdp_st - start_pdp_offset) % pdp_cnt);
+    unsigned long pdp_into_cdp_cnt = ((elapsed_pdp_st - start_pdp_offset) % pdp_cnt);
+    if ( pdp_into_cdp_cnt == 0 || isnan(pdp_temp_val)){
+        switch (current_cf) {
+        case CF_MAXIMUM:
+            return -DINF;
+        case CF_MINIMUM:
+            return DINF;
+        case CF_AVERAGE:
+            return 0;
+        default:
+            return DNAN;
+        }        
+    } 
+    else {
+        switch (current_cf) {
+        case CF_AVERAGE:
+            return pdp_temp_val *  pdp_into_cdp_cnt ;
+        default:
+            return pdp_temp_val;
+        }        
+    }        
 }
 
 /*