Re: BUG #18247: Integer overflow leads to negative width - Mailing list pgsql-bugs

Junwang Zhao <zhjwpku@gmail.com> writes:
> I guess using double for the sum is in case of overflow of int64?
> pg_class.relnatts
> is of type int16, I guess it's not possible to overflow int64.

Not sure what I think about that.  The main advantage of double is
that you definitely don't need to worry about overflow.  It's
probably about the same speed either way on modern hardware.
(On 32-bit hardware it's quite likely that double is faster,
since even 32-bit CPUs tend to have hardware floating point.)
I left it as double for the moment, but we could discuss that
some more.

Some review comments:

* clamp_width_est doesn't need to check isinf, since the
comparison will work just fine.  It does need to check isnan,
unless we switch to int64.

* We can fold the Assert about width >= 0 into clamp_width_est
too; it fits naturally there and saves code in the callers.

* I did not like changing the output type of get_rel_data_width;
it seems better to me to have it do clamp_width_est internally.
For one reason, several of the call sites are relying on
doing integer division, which the v2 patch might break.

The attached v3 fixes those things and makes some cosmetic
changes (mostly comments).

            regards, tom lane

diff --git a/src/backend/optimizer/path/costsize.c b/src/backend/optimizer/path/costsize.c
index 5227346aeb..630b445546 100644
--- a/src/backend/optimizer/path/costsize.c
+++ b/src/backend/optimizer/path/costsize.c
@@ -218,6 +218,29 @@ clamp_row_est(double nrows)
     return nrows;
 }

+/*
+ * clamp_width_est
+ *        Force a tuple-width estimate to a sane value.
+ */
+int32
+clamp_width_est(double tuple_width)
+{
+    /*
+     * Anything more than MaxAllocSize is clearly bogus, since we could not
+     * create a tuple that large.  Treat NaN like infinity.
+     */
+    if (tuple_width > MaxAllocSize || isnan(tuple_width))
+        return (int32) MaxAllocSize;
+
+    /*
+     * Unlike clamp_row_est, we just Assert that the value isn't negative,
+     * rather than masking such errors.
+     */
+    Assert(tuple_width >= 0);
+
+    return (int32) tuple_width;
+}
+
 /*
  * clamp_cardinality_to_long
  *        Cast a Cardinality value to a sane long value.
@@ -6101,7 +6124,7 @@ static void
 set_rel_width(PlannerInfo *root, RelOptInfo *rel)
 {
     Oid            reloid = planner_rt_fetch(rel->relid, root)->relid;
-    int32        tuple_width = 0;
+    double        tuple_width = 0;
     bool        have_wholerow_var = false;
     ListCell   *lc;

@@ -6213,7 +6236,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
      */
     if (have_wholerow_var)
     {
-        int32        wholerow_width = MAXALIGN(SizeofHeapTupleHeader);
+        double        wholerow_width = MAXALIGN(SizeofHeapTupleHeader);

         if (reloid != InvalidOid)
         {
@@ -6230,7 +6253,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
                 wholerow_width += rel->attr_widths[i - rel->min_attr];
         }

-        rel->attr_widths[0 - rel->min_attr] = wholerow_width;
+        rel->attr_widths[0 - rel->min_attr] = clamp_width_est(wholerow_width);

         /*
          * Include the whole-row Var as part of the output tuple.  Yes, that
@@ -6239,8 +6262,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
         tuple_width += wholerow_width;
     }

-    Assert(tuple_width >= 0);
-    rel->reltarget->width = tuple_width;
+    rel->reltarget->width = clamp_width_est(tuple_width);
 }

 /*
@@ -6258,7 +6280,7 @@ set_rel_width(PlannerInfo *root, RelOptInfo *rel)
 PathTarget *
 set_pathtarget_cost_width(PlannerInfo *root, PathTarget *target)
 {
-    int32        tuple_width = 0;
+    double        tuple_width = 0;
     ListCell   *lc;

     /* Vars are assumed to have cost zero, but other exprs do not */
@@ -6282,8 +6304,7 @@ set_pathtarget_cost_width(PlannerInfo *root, PathTarget *target)
         }
     }

-    Assert(tuple_width >= 0);
-    target->width = tuple_width;
+    target->width = clamp_width_est(tuple_width);

     return target;
 }
diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c
index a8cea5efe1..80818776e8 100644
--- a/src/backend/optimizer/plan/planner.c
+++ b/src/backend/optimizer/plan/planner.c
@@ -4610,6 +4610,7 @@ create_one_window_path(PlannerInfo *root,
              * Note: a WindowFunc adds nothing to the target's eval costs; but
              * we do need to account for the increase in tlist width.
              */
+            double        tuple_width = window_target->width;
             ListCell   *lc2;

             window_target = copy_pathtarget(window_target);
@@ -4618,8 +4619,9 @@ create_one_window_path(PlannerInfo *root,
                 WindowFunc *wfunc = lfirst_node(WindowFunc, lc2);

                 add_column_to_pathtarget(window_target, (Expr *) wfunc, 0);
-                window_target->width += get_typavgwidth(wfunc->wintype, -1);
+                tuple_width += get_typavgwidth(wfunc->wintype, -1);
             }
+            window_target->width = clamp_width_est(tuple_width);
         }
         else
         {
diff --git a/src/backend/optimizer/util/placeholder.c b/src/backend/optimizer/util/placeholder.c
index b1723578e6..5d7db4e94f 100644
--- a/src/backend/optimizer/util/placeholder.c
+++ b/src/backend/optimizer/util/placeholder.c
@@ -375,6 +375,7 @@ add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
                             SpecialJoinInfo *sjinfo)
 {
     Relids        relids = joinrel->relids;
+    double        tuple_width = joinrel->reltarget->width;
     ListCell   *lc;

     foreach(lc, root->placeholder_list)
@@ -419,7 +420,7 @@ add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
                     cost_qual_eval_node(&cost, (Node *) phv->phexpr, root);
                     joinrel->reltarget->cost.startup += cost.startup;
                     joinrel->reltarget->cost.per_tuple += cost.per_tuple;
-                    joinrel->reltarget->width += phinfo->ph_width;
+                    tuple_width += phinfo->ph_width;
                 }
             }

@@ -443,6 +444,8 @@ add_placeholders_to_joinrel(PlannerInfo *root, RelOptInfo *joinrel,
                                 phinfo->ph_lateral);
         }
     }
+
+    joinrel->reltarget->width = clamp_width_est(tuple_width);
 }

 /*
diff --git a/src/backend/optimizer/util/plancat.c b/src/backend/optimizer/util/plancat.c
index 7159c775fb..77478738ec 100644
--- a/src/backend/optimizer/util/plancat.c
+++ b/src/backend/optimizer/util/plancat.c
@@ -1137,7 +1137,7 @@ estimate_rel_size(Relation rel, int32 *attr_widths,
 int32
 get_rel_data_width(Relation rel, int32 *attr_widths)
 {
-    int32        tuple_width = 0;
+    double        tuple_width = 0;
     int            i;

     for (i = 1; i <= RelationGetNumberOfAttributes(rel); i++)
@@ -1167,7 +1167,7 @@ get_rel_data_width(Relation rel, int32 *attr_widths)
         tuple_width += item_width;
     }

-    return tuple_width;
+    return clamp_width_est(tuple_width);
 }

 /*
diff --git a/src/backend/optimizer/util/relnode.c b/src/backend/optimizer/util/relnode.c
index 5d83f60eb9..1b35b08c75 100644
--- a/src/backend/optimizer/util/relnode.c
+++ b/src/backend/optimizer/util/relnode.c
@@ -22,6 +22,7 @@
 #include "optimizer/clauses.h"
 #include "optimizer/cost.h"
 #include "optimizer/inherit.h"
+#include "optimizer/optimizer.h"
 #include "optimizer/pathnode.h"
 #include "optimizer/paths.h"
 #include "optimizer/placeholder.h"
@@ -1092,6 +1093,7 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
                     bool can_null)
 {
     Relids        relids = joinrel->relids;
+    double        tuple_width = joinrel->reltarget->width;
     ListCell   *vars;
     ListCell   *lc;

@@ -1144,7 +1146,7 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
                 joinrel->reltarget->exprs = lappend(joinrel->reltarget->exprs,
                                                     phv);
                 /* Bubbling up the precomputed result has cost zero */
-                joinrel->reltarget->width += phinfo->ph_width;
+                tuple_width += phinfo->ph_width;
             }
             continue;
         }
@@ -1165,7 +1167,7 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
                 list_nth(root->row_identity_vars, var->varattno - 1);

             /* Update reltarget width estimate from RowIdentityVarInfo */
-            joinrel->reltarget->width += ridinfo->rowidwidth;
+            tuple_width += ridinfo->rowidwidth;
         }
         else
         {
@@ -1181,7 +1183,7 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,
                 continue;        /* nope, skip it */

             /* Update reltarget width estimate from baserel's attr_widths */
-            joinrel->reltarget->width += baserel->attr_widths[ndx];
+            tuple_width += baserel->attr_widths[ndx];
         }

         /*
@@ -1221,6 +1223,8 @@ build_joinrel_tlist(PlannerInfo *root, RelOptInfo *joinrel,

         /* Vars have cost zero, so no need to adjust reltarget->cost */
     }
+
+    joinrel->reltarget->width = clamp_width_est(tuple_width);
 }

 /*
diff --git a/src/include/optimizer/optimizer.h b/src/include/optimizer/optimizer.h
index 6e8b81c51d..68c1443183 100644
--- a/src/include/optimizer/optimizer.h
+++ b/src/include/optimizer/optimizer.h
@@ -90,6 +90,7 @@ extern PGDLLIMPORT double recursive_worktable_factor;
 extern PGDLLIMPORT int effective_cache_size;

 extern double clamp_row_est(double nrows);
+extern int32 clamp_width_est(double tuple_width);
 extern long clamp_cardinality_to_long(Cardinality x);

 /* in path/indxpath.c: */