Date: Thu, 8 Aug 2024 10:13:51 -0700
From: Noah Misch <noah@leadboat.com>
To: Alexander Korotkov <aekorotkov@gmail.com>
Cc: Dmitry Koval <d.koval@postgrespro.ru>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Message-ID: <20240808171351.a9.nmisch@google.com>
References: <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
 <171085858750.2046434.12246066729700037354.pgcf@coridan.postgresql.org>
 <909e16b4-9eda-435c-b033-8294ae4adc05@postgrespro.ru>
 <CAPpHfdsVQwnZPxaQPwcu1g_01F0d-o=ECRcf14X3CmavJB6vSQ@mail.gmail.com>
 <a83d5a56-ff33-47e8-b2d8-7e9a5d59ee1e@postgrespro.ru>
 <CAPpHfdsCCV8DOv0Ovu4dTSnuWXiEz_AjuW-J6TPj++tcfEGvRw@mail.gmail.com>
 <3a4b9c1c-3edf-4e29-9629-b4113aefb0f3@postgrespro.ru>
 <CAPpHfduNYkMVuUSxZjD6u4tZG6aKdhgQcmhr-K9OQfeg56MNww@mail.gmail.com>
 <c3730d78-6081-4c41-9715-d1d192734576@postgrespro.ru>
 <CAPpHfdtj7YsPaASoVPN+N3H4_Ct+kQw8QY1d_9u7FPnbghkicw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="NQwcF9XvrtwJ+baa"
Content-Disposition: inline
In-Reply-To: <CAPpHfdtj7YsPaASoVPN+N3H4_Ct+kQw8QY1d_9u7FPnbghkicw@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: <https://www.postgresql.org/message-id/20240808171351.a9.nmisch%40google.com>
Precedence: bulk


--NQwcF9XvrtwJ+baa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Apr 07, 2024 at 01:22:51AM +0300, Alexander Korotkov wrote:
> I've pushed 0001 and 0002

The partition MERGE (1adf16b8f) and SPLIT (87c21bb94) v17 patches introduced
createPartitionTable() with this code:

	createStmt->relation = newPartName;
...
	wrapper->utilityStmt = (Node *) createStmt;
...
	ProcessUtility(wrapper,
...
	newRel = table_openrv(newPartName, NoLock);

This breaks from the CVE-2014-0062 (commit 5f17304) principle of not repeating
name lookups.  The attached demo uses this defect to make one partition have
two parents.

--NQwcF9XvrtwJ+baa
Content-Type: text/plain; charset=us-ascii
Content-Disposition: attachment;
	filename="repro-merge-partition-race-v0.patch"

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index ae2efdc..654b502 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -3495,7 +3495,11 @@ StorePartitionBound(Relation rel, Relation parent, PartitionBoundSpec *bound)
 		elog(ERROR, "cache lookup failed for relation %u",
 			 RelationGetRelid(rel));
 
-#ifdef USE_ASSERT_CHECKING
+	/*
+	 * Assertion fails during partition getting multiple parents.  Disable the
+	 * assertion, to see what non-assert builds experience.
+	 */
+#if 0
 	{
 		Form_pg_class classForm;
 		bool		isnull;
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8fcb188..48207f9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -95,6 +95,7 @@
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
+#include "utils/injection_point.h"
 #include "utils/inval.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -15825,7 +15826,11 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispart
 			 */
 			if (parent_rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
 			{
-				Assert(child_att->attinhcount == 1);
+				/*
+				 * Assertion fails during partition getting multiple parents.
+				 * Disable, to see what non-assert builds experience.
+				 */
+				/* Assert(child_att->attinhcount == 1); */
 				child_att->attislocal = false;
 			}
 
@@ -20388,7 +20393,14 @@ createPartitionTable(RangeVar *newPartName, Relation modelRel,
 	 * Open the new partition with no lock, because we already have
 	 * AccessExclusiveLock placed there after creation.
 	 */
-	newRel = table_openrv(newPartName, NoLock);
+	INJECTION_POINT("merge-after-create");
+	/*
+	 * For testing, switch to taking a lock.  This solves two problems.
+	 * First, it gets an AcceptInvalidationMessages(), so we actually
+	 * invalidate the search path.  Second, it avoids an assertion failure
+	 * from our lack of lock, so we see what non-assert builds experience.
+	 */
+	newRel = table_openrv(newPartName, AccessExclusiveLock);
 
 	/*
 	 * We intended to create the partition with the same persistence as the
diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile
index 2ffd2f7..9af45bf 100644
--- a/src/test/modules/injection_points/Makefile
+++ b/src/test/modules/injection_points/Makefile
@@ -9,7 +9,7 @@ PGFILEDESC = "injection_points - facility for injection points"
 REGRESS = injection_points
 REGRESS_OPTS = --dlpath=$(top_builddir)/src/test/regress
 
-ISOLATION = inplace
+ISOLATION = inplace merge
 
 # The injection points are cluster-wide, so disable installcheck
 NO_INSTALLCHECK = 1
diff --git a/src/test/modules/injection_points/expected/merge.out b/src/test/modules/injection_points/expected/merge.out
new file mode 100644
index 0000000..04920e7
--- /dev/null
+++ b/src/test/modules/injection_points/expected/merge.out
@@ -0,0 +1,60 @@
+Parsed test spec with 3 sessions
+
+starting permutation: merge1 ct2 detach3
+injection_points_attach
+-----------------------
+                       
+(1 row)
+
+step merge1: 
+	SET search_path = target;
+	ALTER TABLE parted MERGE PARTITIONS (part1, part3) INTO part_all;
+	-- only one of *parted should have rows, but both do
+	SELECT a AS new_target_parted FROM target.parted ORDER BY 1;
+	SELECT a AS old_target_parted FROM old_target.parted ORDER BY 1;
+	SELECT a AS new_target_part_all FROM target.part_all ORDER BY 1;
+	SELECT a AS old_target_part_all FROM old_target.part_all ORDER BY 1;
+ <waiting ...>
+step ct2: 
+	ALTER SCHEMA target RENAME TO old_target;
+	CREATE SCHEMA target
+		CREATE TABLE parted (a int) partition by list (a)
+		CREATE TABLE part_all PARTITION OF parted FOR VALUES IN (1, 2, 3, 4)
+
+step detach3: 
+	SELECT injection_points_detach('merge-after-create');
+	SELECT injection_points_wakeup('merge-after-create');
+
+injection_points_detach
+-----------------------
+                       
+(1 row)
+
+injection_points_wakeup
+-----------------------
+                       
+(1 row)
+
+step merge1: <... completed>
+new_target_parted
+-----------------
+                1
+                3
+(2 rows)
+
+old_target_parted
+-----------------
+                1
+                3
+(2 rows)
+
+new_target_part_all
+-------------------
+                  1
+                  3
+(2 rows)
+
+old_target_part_all
+-------------------
+(0 rows)
+
diff --git a/src/test/modules/injection_points/specs/merge.spec b/src/test/modules/injection_points/specs/merge.spec
new file mode 100644
index 0000000..63bcd2b
--- /dev/null
+++ b/src/test/modules/injection_points/specs/merge.spec
@@ -0,0 +1,51 @@
+setup
+{
+	CREATE EXTENSION injection_points;
+	CREATE SCHEMA target
+		CREATE TABLE parted (a int) partition by list (a)
+		CREATE TABLE part1 PARTITION OF parted FOR VALUES IN (1, 2)
+		CREATE TABLE part3 PARTITION OF parted FOR VALUES IN (3, 4);
+	INSERT INTO target.parted VALUES (1),(3);
+}
+teardown
+{
+	DROP SCHEMA target, old_target CASCADE;
+	DROP EXTENSION injection_points;
+}
+
+# MERGE PARTITIONS
+session s1
+setup	{
+	SELECT injection_points_set_local();
+	SELECT injection_points_attach('merge-after-create', 'wait');
+}
+step merge1	{
+	SET search_path = target;
+	ALTER TABLE parted MERGE PARTITIONS (part1, part3) INTO part_all;
+	-- only one of *parted should have rows, but both do
+	SELECT a AS new_target_parted FROM target.parted ORDER BY 1;
+	SELECT a AS old_target_parted FROM old_target.parted ORDER BY 1;
+	SELECT a AS new_target_part_all FROM target.part_all ORDER BY 1;
+	SELECT a AS old_target_part_all FROM old_target.part_all ORDER BY 1;
+}
+
+
+# inject another table via ALTER SCHEMA RENAME
+session s2
+step ct2	{
+	ALTER SCHEMA target RENAME TO old_target;
+	CREATE SCHEMA target
+		CREATE TABLE parted (a int) partition by list (a)
+		CREATE TABLE part_all PARTITION OF parted FOR VALUES IN (1, 2, 3, 4)
+}
+
+
+# injection release
+session s3
+step detach3	{
+	SELECT injection_points_detach('merge-after-create');
+	SELECT injection_points_wakeup('merge-after-create');
+}
+
+
+permutation merge1(detach3) ct2 detach3

--NQwcF9XvrtwJ+baa--