From: David Rowley <drowley@postgresql.org>
Date: Thu, 18 Jan 2024 21:44:36 +0000 (+1300)
Subject: Fix broken Bitmapset optimization in DiscreteKnapsack()
X-Git-Tag: REL_17_BETA1~1074
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4b310636439da432c473e0e85de661b3342bb745;p=thirdparty%2Fpostgresql.git

Fix broken Bitmapset optimization in DiscreteKnapsack()

Some code in DiscreteKnapsack() attempted to zero all words in a
Bitmapset by performing bms_del_members() to delete all the members from
itself before replacing those members with members from another set.
When that code was written, this was a valid way to manipulate the set
in such a way to save from palloc having to be called to allocate a new
Bitmapset.  However, 00b41463c modified Bitmapsets so that an empty set is
*always* represented as NULL and this breaks the optimization as the
Bitmapset code will always pfree the memory when the set becomes empty.

Since DiscreteKnapsack() has been coded to avoid as many unneeded
allocations as possible, it seems risky to not fix this.  Here we add
bms_replace_members() to effectively perform an in-place copy of another
set, reusing the memory of the existing set, when possible.

This got broken in v16, but no backpatch for now as there've been no
complaints.

Reviewed-by: Richard Guo
Discussion: https://postgr.es/m/CAApHDvoTCBkBU2PJghNOFUiO0q=QP4WAWHi5sJP6_4=b2WodrA@mail.gmail.com
---

diff --git a/src/backend/lib/knapsack.c b/src/backend/lib/knapsack.c
index 13d800718f0..439da1ad70d 100644
--- a/src/backend/lib/knapsack.c
+++ b/src/backend/lib/knapsack.c
@@ -89,10 +89,7 @@ DiscreteKnapsack(int max_weight, int num_items,
 			{
 				/* copy sets[ow] to sets[j] without realloc */
 				if (j != ow)
-				{
-					sets[j] = bms_del_members(sets[j], sets[j]);
-					sets[j] = bms_add_members(sets[j], sets[ow]);
-				}
+					sets[j] = bms_replace_members(sets[j], sets[ow]);
 
 				sets[j] = bms_add_member(sets[j], i);
 
diff --git a/src/backend/nodes/bitmapset.c b/src/backend/nodes/bitmapset.c
index b0f908f978f..65805d45277 100644
--- a/src/backend/nodes/bitmapset.c
+++ b/src/backend/nodes/bitmapset.c
@@ -976,6 +976,50 @@ bms_add_members(Bitmapset *a, const Bitmapset *b)
 	return result;
 }
 
+/*
+ * bms_replace_members
+ *		Remove all existing members from 'a' and repopulate the set with members
+ *		from 'b', recycling 'a', when possible.
+ */
+Bitmapset *
+bms_replace_members(Bitmapset *a, const Bitmapset *b)
+{
+	int			i;
+
+	Assert(bms_is_valid_set(a));
+	Assert(bms_is_valid_set(b));
+
+	if (a == NULL)
+		return bms_copy(b);
+	if (b == NULL)
+	{
+		pfree(a);
+		return NULL;
+	}
+
+	if (a->nwords < b->nwords)
+		a = (Bitmapset *) repalloc(a, BITMAPSET_SIZE(b->nwords));
+
+	i = 0;
+	do
+	{
+		a->words[i] = b->words[i];
+	} while (++i < b->nwords);
+
+	a->nwords = b->nwords;
+
+#ifdef REALLOCATE_BITMAPSETS
+
+	/*
+	 * There's no guarantee that the repalloc returned a new pointer, so copy
+	 * and free unconditionally here.
+	 */
+	a = bms_copy_and_free(a);
+#endif
+
+	return a;
+}
+
 /*
  * bms_add_range
  *		Add members in the range of 'lower' to 'upper' to the set.
diff --git a/src/include/nodes/bitmapset.h b/src/include/nodes/bitmapset.h
index efc8890ce61..906e8dcc157 100644
--- a/src/include/nodes/bitmapset.h
+++ b/src/include/nodes/bitmapset.h
@@ -109,6 +109,7 @@ extern BMS_Membership bms_membership(const Bitmapset *a);
 extern Bitmapset *bms_add_member(Bitmapset *a, int x);
 extern Bitmapset *bms_del_member(Bitmapset *a, int x);
 extern Bitmapset *bms_add_members(Bitmapset *a, const Bitmapset *b);
+extern Bitmapset *bms_replace_members(Bitmapset *a, const Bitmapset *b);
 extern Bitmapset *bms_add_range(Bitmapset *a, int lower, int upper);
 extern Bitmapset *bms_int_members(Bitmapset *a, const Bitmapset *b);
 extern Bitmapset *bms_del_members(Bitmapset *a, const Bitmapset *b);