Fix mvdistinct and dependencies size calculations

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

Author: tvondra

src/backend/statistics/dependencies.c

@@ -32,6 +32,20 @@
 #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)",

src/backend/statistics/mvdistinct.c

@@ -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;

src/include/statistics/statistics.h

@@ -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 */