]> granicus.if.org Git - postgresql/commitdiff
Fix mvdistinct and dependencies size calculations
authorTomas Vondra <tomas.vondra@postgresql.org>
Sun, 21 Apr 2019 17:54:15 +0000 (19:54 +0200)
committerTomas Vondra <tomas.vondra@postgresql.org>
Sun, 21 Apr 2019 18:23:34 +0000 (20:23 +0200)
The formulas used to calculate size while (de)serializing mvndistinct
and functional dependencies were based on offset() of the structs. But
that is incorrect, because the structures are not copied directly, we
we copy the individual fields directly.

At the moment this works fine, because there is no alignment padding
on any platform we support. But it might break if we ever added some
fields into any of the structs, for example. It's also confusing.

Fixed by reworking the macros to directly sum sizes of serialized
fields. The macros are now useful only for serialiation, so there is
no point in keeping them in the public header file. So make them
private by moving them to the .c files.

Also adds a couple more asserts to check the serialization, and fixes
an incorrect allocation of MVDependency instead of (MVDependency *).

Reported-By: Tom Lane
Discussion: https://postgr.es/m/29785.1555365602@sss.pgh.pa.us

src/backend/statistics/dependencies.c
src/backend/statistics/mvdistinct.c
src/include/statistics/statistics.h

index f11cced2936e27c68227c0f5e402ef27bdfba29b..0b26e4166d90c4b673258c2c7ef1a5e254fd8127 100644 (file)
 #include "utils/syscache.h"
 #include "utils/typcache.h"
 
+/* size of the struct header fields (magic, type, ndeps) */
+#define SizeOfHeader           (3 * sizeof(uint32))
+
+/* size of a serialized dependency (degree, natts, atts) */
+#define SizeOfItem(natts) \
+       (sizeof(double) + sizeof(AttrNumber) * (1 + (natts)))
+
+/* minimal size of a dependency (with two attributes) */
+#define MinSizeOfItem  SizeOfItem(2)
+
+/* minimal size of dependencies, when all deps are minimal */
+#define MinSizeOfItems(ndeps) \
+       (SizeOfHeader + (ndeps) * MinSizeOfItem)
+
 /*
  * Internal state for DependencyGenerator of dependencies. Dependencies are similar to
  * k-permutations of n elements, except that the order does not matter for the
@@ -408,7 +422,7 @@ statext_dependencies_build(int numrows, HeapTuple *rows, Bitmapset *attrs,
                        dependencies->ndeps++;
                        dependencies = (MVDependencies *) repalloc(dependencies,
                                                                                                           offsetof(MVDependencies, deps)
-                                                                                                          + dependencies->ndeps * sizeof(MVDependency));
+                                                                                                          + dependencies->ndeps * sizeof(MVDependency *));
 
                        dependencies->deps[dependencies->ndeps - 1] = d;
                }
@@ -436,12 +450,11 @@ statext_dependencies_serialize(MVDependencies *dependencies)
        Size            len;
 
        /* we need to store ndeps, with a number of attributes for each one */
-       len = VARHDRSZ + SizeOfDependencies
-               + dependencies->ndeps * SizeOfDependency;
+       len = VARHDRSZ + SizeOfHeader;
 
        /* and also include space for the actual attribute numbers and degrees */
        for (i = 0; i < dependencies->ndeps; i++)
-               len += (sizeof(AttrNumber) * dependencies->deps[i]->nattributes);
+               len += SizeOfItem(dependencies->deps[i]->nattributes);
 
        output = (bytea *) palloc0(len);
        SET_VARSIZE(output, len);
@@ -461,15 +474,22 @@ statext_dependencies_serialize(MVDependencies *dependencies)
        {
                MVDependency *d = dependencies->deps[i];
 
-               memcpy(tmp, d, SizeOfDependency);
-               tmp += SizeOfDependency;
+               memcpy(tmp, &d->degree, sizeof(double));
+               tmp += sizeof(double);
+
+               memcpy(tmp, &d->nattributes, sizeof(AttrNumber));
+               tmp += sizeof(AttrNumber);
 
                memcpy(tmp, d->attributes, sizeof(AttrNumber) * d->nattributes);
                tmp += sizeof(AttrNumber) * d->nattributes;
 
+               /* protect against overflow */
                Assert(tmp <= ((char *) output + len));
        }
 
+       /* make sure we've produced exactly the right amount of data */
+       Assert(tmp == ((char *) output + len));
+
        return output;
 }
 
@@ -487,9 +507,9 @@ statext_dependencies_deserialize(bytea *data)
        if (data == NULL)
                return NULL;
 
-       if (VARSIZE_ANY_EXHDR(data) < SizeOfDependencies)
+       if (VARSIZE_ANY_EXHDR(data) < SizeOfHeader)
                elog(ERROR, "invalid MVDependencies size %zd (expected at least %zd)",
-                        VARSIZE_ANY_EXHDR(data), SizeOfDependencies);
+                        VARSIZE_ANY_EXHDR(data), SizeOfHeader);
 
        /* read the MVDependencies header */
        dependencies = (MVDependencies *) palloc0(sizeof(MVDependencies));
@@ -519,9 +539,7 @@ statext_dependencies_deserialize(bytea *data)
                                 errmsg("invalid zero-length item array in MVDependencies")));
 
        /* what minimum bytea size do we expect for those parameters */
-       min_expected_size = SizeOfDependencies +
-               dependencies->ndeps * (SizeOfDependency +
-                                                          sizeof(AttrNumber) * 2);
+       min_expected_size = SizeOfItem(dependencies->ndeps);
 
        if (VARSIZE_ANY_EXHDR(data) < min_expected_size)
                elog(ERROR, "invalid dependencies size %zd (expected at least %zd)",
index b477c18c46af415e0fee5275f8e3a80f8f6cedb4..133503cb9b3ab54dfc22ed2b25ba1827af5c0880 100644 (file)
@@ -43,6 +43,20 @@ static double estimate_ndistinct(double totalrows, int numrows, int d, int f1);
 static int     n_choose_k(int n, int k);
 static int     num_combinations(int n);
 
+/* size of the struct header fields (magic, type, nitems) */
+#define SizeOfHeader           (3 * sizeof(uint32))
+
+/* size of a serialized ndistinct item (coefficient, natts, atts) */
+#define SizeOfItem(natts) \
+       (sizeof(double) + sizeof(int) + (natts) * sizeof(AttrNumber))
+
+/* minimal size of a ndistinct item (with two attributes) */
+#define MinSizeOfItem  SizeOfItem(2)
+
+/* minimal size of mvndistinct, when all items are minimal */
+#define MinSizeOfItems(nitems) \
+       (SizeOfHeader + (nitems) * MinSizeOfItem)
+
 /* Combination generator API */
 
 /* internal state for generator of k-combinations of n elements */
@@ -168,8 +182,7 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
         * Base size is size of scalar fields in the struct, plus one base struct
         * for each item, including number of items for each.
         */
-       len = VARHDRSZ + SizeOfMVNDistinct +
-               ndistinct->nitems * (offsetof(MVNDistinctItem, attrs) + sizeof(int));
+       len = VARHDRSZ + SizeOfHeader;
 
        /* and also include space for the actual attribute numbers */
        for (i = 0; i < ndistinct->nitems; i++)
@@ -178,7 +191,8 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
 
                nmembers = bms_num_members(ndistinct->items[i].attrs);
                Assert(nmembers >= 2);
-               len += sizeof(AttrNumber) * nmembers;
+
+               len += SizeOfItem(nmembers);
        }
 
        output = (bytea *) palloc(len);
@@ -195,8 +209,7 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
        tmp += sizeof(uint32);
 
        /*
-        * store number of attributes and attribute numbers for each ndistinct
-        * entry
+        * store number of attributes and attribute numbers for each entry
         */
        for (i = 0; i < ndistinct->nitems; i++)
        {
@@ -218,9 +231,13 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct)
                        tmp += sizeof(AttrNumber);
                }
 
+               /* protect against overflows */
                Assert(tmp <= ((char *) output + len));
        }
 
+       /* check we used exactly the expected space */
+       Assert(tmp == ((char *) output + len));
+
        return output;
 }
 
@@ -241,9 +258,9 @@ statext_ndistinct_deserialize(bytea *data)
                return NULL;
 
        /* we expect at least the basic fields of MVNDistinct struct */
-       if (VARSIZE_ANY_EXHDR(data) < SizeOfMVNDistinct)
+       if (VARSIZE_ANY_EXHDR(data) < SizeOfHeader)
                elog(ERROR, "invalid MVNDistinct size %zd (expected at least %zd)",
-                        VARSIZE_ANY_EXHDR(data), SizeOfMVNDistinct);
+                        VARSIZE_ANY_EXHDR(data), SizeOfHeader);
 
        /* initialize pointer to the data part (skip the varlena header) */
        tmp = VARDATA_ANY(data);
@@ -272,9 +289,7 @@ statext_ndistinct_deserialize(bytea *data)
                                 errmsg("invalid zero-length item array in MVNDistinct")));
 
        /* what minimum bytea size do we expect for those parameters */
-       minimum_size = (SizeOfMVNDistinct +
-                                       ndist.nitems * (SizeOfMVNDistinctItem +
-                                                                       sizeof(AttrNumber) * 2));
+       minimum_size = MinSizeOfItems(ndist.nitems);
        if (VARSIZE_ANY_EXHDR(data) < minimum_size)
                ereport(ERROR,
                                (errcode(ERRCODE_DATA_CORRUPTED),
@@ -285,7 +300,7 @@ statext_ndistinct_deserialize(bytea *data)
         * Allocate space for the ndistinct items (no space for each item's
         * attnos: those live in bitmapsets allocated separately)
         */
-       ndistinct = palloc0(MAXALIGN(SizeOfMVNDistinct) +
+       ndistinct = palloc0(MAXALIGN(offsetof(MVNDistinct, items)) +
                                                (ndist.nitems * sizeof(MVNDistinctItem)));
        ndistinct->magic = ndist.magic;
        ndistinct->type = ndist.type;
index 6fafbbf11c046b15a8b0d35a3a1d617388652644..69724cc1070bee4cbc745809220b2665ab5bfcbf 100644 (file)
@@ -29,10 +29,6 @@ typedef struct MVNDistinctItem
        Bitmapset  *attrs;                      /* attr numbers of items */
 } MVNDistinctItem;
 
-/* size of the struct, excluding attribute list */
-#define SizeOfMVNDistinctItem \
-       (offsetof(MVNDistinctItem, ndistinct) + sizeof(double))
-
 /* A MVNDistinct object, comprising all possible combinations of columns */
 typedef struct MVNDistinct
 {
@@ -42,13 +38,7 @@ typedef struct MVNDistinct
        MVNDistinctItem items[FLEXIBLE_ARRAY_MEMBER];
 } MVNDistinct;
 
-/* size of the struct excluding the items array */
-#define SizeOfMVNDistinct      (offsetof(MVNDistinct, nitems) + sizeof(uint32))
-
-
-/* size of the struct excluding the items array */
-#define SizeOfMVNDistinct      (offsetof(MVNDistinct, nitems) + sizeof(uint32))
-
+/* Multivariate functional dependencies */
 #define STATS_DEPS_MAGIC               0xB4549A2C      /* marks serialized bytea */
 #define STATS_DEPS_TYPE_BASIC  1       /* basic dependencies type */
 
@@ -63,10 +53,6 @@ typedef struct MVDependency
        AttrNumber      attributes[FLEXIBLE_ARRAY_MEMBER];      /* attribute numbers */
 } MVDependency;
 
-/* size of the struct excluding the deps array */
-#define SizeOfDependency \
-       (offsetof(MVDependency, nattributes) + sizeof(AttrNumber))
-
 typedef struct MVDependencies
 {
        uint32          magic;                  /* magic constant marker */
@@ -75,9 +61,6 @@ typedef struct MVDependencies
        MVDependency *deps[FLEXIBLE_ARRAY_MEMBER];      /* dependencies */
 } MVDependencies;
 
-/* size of the struct excluding the deps array */
-#define SizeOfDependencies     (offsetof(MVDependencies, ndeps) + sizeof(uint32))
-
 /* used to flag stats serialized to bytea */
 #define STATS_MCV_MAGIC                        0xE1A651C2      /* marks serialized bytea */
 #define STATS_MCV_TYPE_BASIC   1       /* basic MCV list type */