Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands

From: Alexander Korotkov <[email-masked]>
Date: Sun, 7 Apr 2024 01:22:51 +0300

Hi, Dmitry!

On Fri, Apr 5, 2024 at 4:00 PM Dmitry Koval <[email-masked]> wrote:
> > I've revised the patchset.
>
> Thanks for the corrections (especially ddl.sgml).
> Could you also look at a small optimization for the MERGE PARTITIONS
> command (in a separate file
> v31-0003-Additional-patch-for-ALTER-TABLE-.-MERGE-PARTITI.patch, I wrote
> about it in an email 2024-03-31 00:56:50)?
>
> Files v31-0001-*.patch, v31-0002-*.patch are the same as
> v30-0001-*.patch, v30-0002-*.patch (after rebasing because patch stopped
> applying due to changes in upstream).

I've pushed 0001 and 0002.  I didn't push 0003 for the following reasons.
1) This doesn't keep functionality equivalent to 0001.  With 0003, the
merged partition will inherit indexes, constraints, and so on from the
one of merging partitions.
2) This is not necessarily an optimization. Without 0003 indexes on
the merged partition are created after moving the rows in
attachPartitionTable(). With 0003 we merge data into the existing
partition which saves its indexes.  That might cause a significant
performance loss because mass inserts into indexes may be much slower
than building indexes from scratch.
I think both aspects need to be carefully considered.  Even if we
accept them, this needs to be documented.  I think now it's too late
for both of these.  So, this should wait for v18.

------
Regards,
Alexander Korotkov

---

From: Dmitry Koval <[email-masked]>
Date: Sun, 7 Apr 2024 01:38:56 +0300

Hi, Alexander!

> I didn't push 0003 for the following reasons. ....

Thanks for clarifying. You are right, these are serious reasons.

-- 
With best regards,
Dmitry Koval

Postgres Professional: http://postgrespro.com

---

From: Alexander Lakhin <[email-masked]>
Date: Sun, 7 Apr 2024 22:00:00 +0300

Hi Alexander and Dmitry,

> I've pushed 0001 and 0002.  I didn't push 0003 for the following reasons.

Please try the following (erroneous) query:
CREATE TABLE t1(i int, t text) PARTITION BY LIST (t);
CREATE TABLE t1pa PARTITION OF t1 FOR VALUES IN ('A');

CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
ALTER TABLE t2 SPLIT PARTITION t1pa INTO
   (PARTITION t2a FOR VALUES FROM ('A') TO ('B'),
    PARTITION t2b FOR VALUES FROM ('B') TO ('C'));

that triggers an assertion failure:
TRAP: failed Assert("datums != NIL"), File: "partbounds.c", Line: 3434, PID: 1841459

or a segfault (in a non-assert build):
Program terminated with signal SIGSEGV, Segmentation fault.

#0  pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
1866            if (VARATT_IS_COMPRESSED(datum) || VARATT_IS_EXTERNAL(datum))
(gdb) bt
#0  pg_detoast_datum_packed (datum=0x0) at fmgr.c:1866
#1  0x000055f38c5d5e3f in bttextcmp (...) at varlena.c:1834
#2  0x000055f38c6030dd in FunctionCall2Coll (...) at fmgr.c:1161
#3  0x000055f38c417c83 in partition_rbound_cmp (...) at partbounds.c:3525
#4  check_partition_bounds_for_split_range (...) at partbounds.c:5221
#5  check_partitions_for_split (...) at partbounds.c:5688
#6  0x000055f38c256c49 in transformPartitionCmdForSplit (...) at parse_utilcmd.c:3451
#7  transformAlterTableStmt (...) at parse_utilcmd.c:3810
#8  0x000055f38c2bdf9c in ATParseTransformCmd (...) at tablecmds.c:5650
...

Best regards,
Alexander

---

From: Tender Wang <[email-masked]>
Date: Mon, 8 Apr 2024 18:43:51 +0800

Hi all,
  I went through the MERGE/SPLIT partition codes today, thanks for the
works.  I found some grammar errors:
 i. in error messages(Users can see this grammar errors, not friendly).
ii. in codes comments

-- 
Tender Wang
OpenPie:  https://en.openpie.com/

---

From: Alexander Lakhin <[email-masked]>
Date: Mon, 8 Apr 2024 15:00:00 +0300

Hi Tender Wang,

08.04.2024 13:43, Tender Wang wrote:
> Hi all,
>   I went through the MERGE/SPLIT partition codes today, thanks for the works.  I found some grammar errors:

On a quick glance, I saw also:
NULL-value
partitionde
splited
temparary

And a trailing whitespace at:
      the quarter partition back to monthly partitions:
warning: 1 line adds whitespace errors.

I'm also confused by "administrators" here:
https://www.postgresql.org/docs/devel/ddl-partitioning.html

(We can find on the same page, for instance:
... whereas table inheritance allows data to be divided in a manner of
the user's choosing.
It seems to me, that "users" should work for merging partitions as well.)

Though the documentation addition requires more than just a quick glance,
of course.

Best regards,
Alexander

---

From: Dmitry Koval <[email-masked]>
Date: Mon, 8 Apr 2024 23:43:21 +0300

Hi!

Attached fix for the problems found by Alexander Lakhin.

About grammar errors.
Unfortunately, I don't know English well.
Therefore, I plan (in the coming days) to show the text to specialists 
who perform technical translation of documentation.

-- 
With best regards,
Dmitry Koval

From 578b3fae50baffa3626570447d55ce4177fc6e7d Mon Sep 17 00:00:00 2001
From: Koval Dmitry <[email-masked]>
Date: Mon, 8 Apr 2024 23:10:52 +0300
Subject: [PATCH v1] Fixes for ALTER TABLE ... SPLIT/MERGE PARTITIONS ...
 commands

---
 doc/src/sgml/ddl.sgml                         |  2 +-
 src/backend/commands/tablecmds.c              | 12 ------
 src/backend/parser/parse_utilcmd.c            | 38 +++++++++++++++++++
 src/test/regress/expected/partition_merge.out | 29 +++++++++++---
 src/test/regress/expected/partition_split.out | 13 +++++++
 src/test/regress/sql/partition_merge.sql      | 24 ++++++++++--
 src/test/regress/sql/partition_split.sql      | 15 ++++++++
 7 files changed, 111 insertions(+), 22 deletions(-)

diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 8ff9a520ca..cd8304ef75 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -4410,7 +4410,7 @@ ALTER TABLE measurement
      this operation is not supported for hash-partitioned tables and acquires
      an <literal>ACCESS EXCLUSIVE</literal> lock, which could impact high-load
      systems due to the lock's restrictive nature.  For example, we can split
-     the quarter partition back to monthly partitions: 
+     the quarter partition back to monthly partitions:
 <programlisting>
 ALTER TABLE measurement SPLIT PARTITION measurement_y2006q1 INTO
    (PARTITION measurement_y2006m01 FOR VALUES FROM ('2006-01-01') TO ('2006-02-01'),
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 865c6331c1..8a98a0af48 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -21223,12 +21223,6 @@ ATExecSplitPartition(List **wqueue, AlteredTableInfo *tab, Relation rel,
 	 */
 	splitRel = table_openrv(cmd->name, AccessExclusiveLock);
 
-	if (splitRel->rd_rel->relkind != RELKIND_RELATION)
-		ereport(ERROR,
-				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-				 errmsg("cannot split non-table partition \"%s\"",
-						RelationGetRelationName(splitRel))));
-
 	splitRelOid = RelationGetRelid(splitRel);
 
 	/* Check descriptions of new partitions. */
@@ -21463,12 +21457,6 @@ ATExecMergePartitions(List **wqueue, AlteredTableInfo *tab, Relation rel,
 		 */
 		mergingPartition = table_openrv(name, AccessExclusiveLock);
 
-		if (mergingPartition->rd_rel->relkind != RELKIND_RELATION)
-			ereport(ERROR,
-					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-					 errmsg("cannot merge non-table partition \"%s\"",
-							RelationGetRelationName(mergingPartition))));
-
 		/*
 		 * Checking that two partitions have the same name was before, in
 		 * function transformPartitionCmdForMerge().
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 9e3e14087f..0d5ed0079c 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -32,6 +32,7 @@
 #include "catalog/heap.h"
 #include "catalog/index.h"
 #include "catalog/namespace.h"
+#include "catalog/partition.h"
 #include "catalog/pg_am.h"
 #include "catalog/pg_collation.h"
 #include "catalog/pg_constraint.h"
@@ -3415,6 +3416,38 @@ transformRuleStmt(RuleStmt *stmt, const char *queryString,
 }

+/*
+ * checkPartition: check that partRelOid is partition of rel
+ */
+static void
+checkPartition(Relation rel, Oid partRelOid)
+{
+	Relation	partRel;
+
+	partRel = relation_open(partRelOid, AccessShareLock);
+
+	if (partRel->rd_rel->relkind != RELKIND_RELATION)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a table",
+						RelationGetRelationName(partRel))));
+
+	if (!partRel->rd_rel->relispartition)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("\"%s\" is not a partition",
+						RelationGetRelationName(partRel))));
+
+	if (get_partition_parent(partRelOid, false) != RelationGetRelid(rel))
+		ereport(ERROR,
+				(errcode(ERRCODE_UNDEFINED_TABLE),
+				 errmsg("relation \"%s\" is not a partition of relation \"%s\"",
+						RelationGetRelationName(partRel),
+						RelationGetRelationName(rel))));
+
+	relation_close(partRel, AccessShareLock);
+}
+
 /*
  * transformPartitionCmdForSplit
  *		Analyze the ALTER TABLLE ... SPLIT PARTITION command
@@ -3447,6 +3480,8 @@ transformPartitionCmdForSplit(CreateStmtContext *cxt, PartitionCmd *partcmd)
 
 	splitPartOid = RangeVarGetRelid(partcmd->name, NoLock, false);
 
+	checkPartition(parent, splitPartOid);
+
 	/* Then we should check partitions with transformed bounds. */
 	check_partitions_for_split(parent, splitPartOid, partcmd->name, partcmd->partlist, cxt->pstate);
 }
@@ -3509,6 +3544,9 @@ transformPartitionCmdForMerge(CreateStmtContext *cxt, PartitionCmd *partcmd)
 		partOid = RangeVarGetRelid(name, NoLock, false);
 		if (partOid == defaultPartOid)
 			isDefaultPart = true;
+
+		checkPartition(parent, partOid);
+
 		partOids = lappend_oid(partOids, partOid);
 	}
 
diff --git a/src/test/regress/expected/partition_merge.out b/src/test/regress/expected/partition_merge.out
index 2ba0ec47d9..60eacf6bf3 100644
--- a/src/test/regress/expected/partition_merge.out
+++ b/src/test/regress/expected/partition_merge.out
@@ -25,9 +25,9 @@ ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_fe
 ERROR:  partition with name "sales_feb2022" already used
 LINE 1: ...e MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_feb2...
                                                              ^
--- ERROR:  cannot merge non-table partition "sales_apr2022"
+-- ERROR:  "sales_apr2022" is not a table
 ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_apr2022) INTO sales_feb_mar_apr2022;
-ERROR:  cannot merge non-table partition "sales_apr2022"
+ERROR:  "sales_apr2022" is not a table
 -- ERROR:  invalid partitions order, partition "sales_mar2022" can not be merged
 -- (space between sections sales_jan2022 and sales_mar2022)
 ALTER TABLE sales_range MERGE PARTITIONS (sales_jan2022, sales_mar2022) INTO sales_jan_mar2022;
@@ -590,12 +590,12 @@ CREATE TABLE sales_nord2 PARTITION OF sales_list2 FOR VALUES IN ('Oslo', 'St. Pe
 CREATE TABLE sales_others2 PARTITION OF sales_list2 DEFAULT;
 CREATE TABLE sales_external (LIKE sales_list);
 CREATE TABLE sales_external2 (vch VARCHAR(5));
--- ERROR:  partition bound for relation "sales_external" is null
+-- ERROR:  "sales_external" is not a partition
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_east, sales_external) INTO sales_all;
-ERROR:  partition bound for relation "sales_external" is null
--- ERROR:  partition bound for relation "sales_external2" is null
+ERROR:  "sales_external" is not a partition
+-- ERROR:  "sales_external2" is not a partition
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_east, sales_external2) INTO sales_all;
-ERROR:  partition bound for relation "sales_external2" is null
+ERROR:  "sales_external2" is not a partition
 -- ERROR:  relation "sales_nord2" is not a partition of relation "sales_list"
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_nord2, sales_east) INTO sales_all;
 ERROR:  relation "sales_nord2" is not a partition of relation "sales_list"
@@ -729,4 +729,21 @@ SELECT * FROM sales_list WHERE salesman_name = 'Ivanov';
 RESET enable_seqscan;
 DROP TABLE sales_list;
 --
+-- Try to MERGE partitions of another table.
+--
+CREATE TABLE t1 (i int, a int, b int, c int) PARTITION BY RANGE (a, b);
+CREATE TABLE t1p1 PARTITION OF t1 FOR VALUES FROM (1, 1) TO (1, 2);
+CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
+CREATE TABLE t2pa PARTITION OF t2 FOR VALUES FROM ('A') TO ('C');
+CREATE TABLE t3 (i int, t text);
+-- ERROR:  relation "t1p1" is not a partition of relation "t2"
+ALTER TABLE t2 MERGE PARTITIONS (t1p1, t2pa) INTO t2p;
+ERROR:  relation "t1p1" is not a partition of relation "t2"
+-- ERROR:  "t3" is not a partition
+ALTER TABLE t2 MERGE PARTITIONS (t2pa, t3) INTO t2p;
+ERROR:  "t3" is not a partition
+DROP TABLE t3;
+DROP TABLE t2;
+DROP TABLE t1;
+--
 DROP SCHEMA partitions_merge_schema;
diff --git a/src/test/regress/expected/partition_split.out b/src/test/regress/expected/partition_split.out
index 675a1453c3..26a0d09969 100644
--- a/src/test/regress/expected/partition_split.out
+++ b/src/test/regress/expected/partition_split.out
@@ -1414,4 +1414,17 @@ SELECT * FROM sales_others;
 
 DROP TABLE sales_range;
 --
+-- Try to SPLIT partition of another table.
+--
+CREATE TABLE t1(i int, t text) PARTITION BY LIST (t);
+CREATE TABLE t1pa PARTITION OF t1 FOR VALUES IN ('A');
+CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
+-- ERROR:  relation "t1pa" is not a partition of relation "t2"
+ALTER TABLE t2 SPLIT PARTITION t1pa INTO
+   (PARTITION t2a FOR VALUES FROM ('A') TO ('B'),
+    PARTITION t2b FOR VALUES FROM ('B') TO ('C'));
+ERROR:  relation "t1pa" is not a partition of relation "t2"
+DROP TABLE t2;
+DROP TABLE t1;
+--
 DROP SCHEMA partition_split_schema;
diff --git a/src/test/regress/sql/partition_merge.sql b/src/test/regress/sql/partition_merge.sql
index bb461e6623..9afed70365 100644
--- a/src/test/regress/sql/partition_merge.sql
+++ b/src/test/regress/sql/partition_merge.sql
@@ -28,7 +28,7 @@ CREATE TABLE sales_others PARTITION OF sales_range DEFAULT;
 
 -- ERROR:  partition with name "sales_feb2022" already used
 ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_feb2022) INTO sales_feb_mar_apr2022;
--- ERROR:  cannot merge non-table partition "sales_apr2022"
+-- ERROR:  "sales_apr2022" is not a table
 ALTER TABLE sales_range MERGE PARTITIONS (sales_feb2022, sales_mar2022, sales_apr2022) INTO sales_feb_mar_apr2022;
 -- ERROR:  invalid partitions order, partition "sales_mar2022" can not be merged
 -- (space between sections sales_jan2022 and sales_mar2022)
@@ -350,9 +350,9 @@ CREATE TABLE sales_others2 PARTITION OF sales_list2 DEFAULT;
 CREATE TABLE sales_external (LIKE sales_list);
 CREATE TABLE sales_external2 (vch VARCHAR(5));
 
--- ERROR:  partition bound for relation "sales_external" is null
+-- ERROR:  "sales_external" is not a partition
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_east, sales_external) INTO sales_all;
--- ERROR:  partition bound for relation "sales_external2" is null
+-- ERROR:  "sales_external2" is not a partition
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_east, sales_external2) INTO sales_all;
 -- ERROR:  relation "sales_nord2" is not a partition of relation "sales_list"
 ALTER TABLE sales_list MERGE PARTITIONS (sales_west, sales_nord2, sales_east) INTO sales_all;
@@ -426,5 +426,23 @@ RESET enable_seqscan;
 
 DROP TABLE sales_list;
 
+--
+-- Try to MERGE partitions of another table.
+--
+CREATE TABLE t1 (i int, a int, b int, c int) PARTITION BY RANGE (a, b);
+CREATE TABLE t1p1 PARTITION OF t1 FOR VALUES FROM (1, 1) TO (1, 2);
+CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
+CREATE TABLE t2pa PARTITION OF t2 FOR VALUES FROM ('A') TO ('C');
+CREATE TABLE t3 (i int, t text);
+
+-- ERROR:  relation "t1p1" is not a partition of relation "t2"
+ALTER TABLE t2 MERGE PARTITIONS (t1p1, t2pa) INTO t2p;
+-- ERROR:  "t3" is not a partition
+ALTER TABLE t2 MERGE PARTITIONS (t2pa, t3) INTO t2p;
+
+DROP TABLE t3;
+DROP TABLE t2;
+DROP TABLE t1;
+
 --
 DROP SCHEMA partitions_merge_schema;
diff --git a/src/test/regress/sql/partition_split.sql b/src/test/regress/sql/partition_split.sql
index 8864f6ddaa..625b01ddd1 100644
--- a/src/test/regress/sql/partition_split.sql
+++ b/src/test/regress/sql/partition_split.sql
@@ -829,5 +829,20 @@ SELECT * FROM sales_others;
 
 DROP TABLE sales_range;
 
+--
+-- Try to SPLIT partition of another table.
+--
+CREATE TABLE t1(i int, t text) PARTITION BY LIST (t);
+CREATE TABLE t1pa PARTITION OF t1 FOR VALUES IN ('A');
+CREATE TABLE t2 (i int, t text) PARTITION BY RANGE (t);
+
+-- ERROR:  relation "t1pa" is not a partition of relation "t2"
+ALTER TABLE t2 SPLIT PARTITION t1pa INTO
+   (PARTITION t2a FOR VALUES FROM ('A') TO ('B'),
+    PARTITION t2b FOR VALUES FROM ('B') TO ('C'));
+
+DROP TABLE t2;
+DROP TABLE t1;
+
 --
 DROP SCHEMA partition_split_schema;
-- 
2.40.1.windows.1

---

From: Noah Misch <[email-masked]>
Date: Thu, 8 Aug 2024 10:13:51 -0700

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.
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

---

From: Alexander Korotkov <[email-masked]>
Date: Fri, 9 Aug 2024 01:43:11 +0300

> name lookups.  The attached demo uses this defect to make one partition have

Thank you for a valuable report.  I will dig into and fix that.

------
Regards,
Alexander Korotkov
Supabase

---

From: Dmitry Koval <[email-masked]>
Date: Fri, 9 Aug 2024 10:18:29 +0300

> name lookups.  The attached demo uses this defect to make one partition have

Thank you very much for information (especially for the demo)!

I'm not sure that we can get the identifier of the newly created 
partition from the ProcessUtility() function...
Maybe it would be enough to check that the new partition is located in 
the namespace in which we created it (see attachment)?

-- 
With best regards,
Dmitry Koval

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 0b2a52463f..a1937d078b 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -20388,6 +20388,14 @@ createPartitionTable(RangeVar *newPartName, Relation modelRel,
 	 */
 	newRel = table_openrv(newPartName, NoLock);
 
+	/* Check for case namespace was renamed during partition creation. */
+	if (RelationGetNamespace(newRel) != RelationGetNamespace(modelRel))
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("cannot create partition because namespace \"%s\" was changed to \"%s\"",
+						get_namespace_name(RelationGetNamespace(newRel)),
+						get_namespace_name(RelationGetNamespace(modelRel)))));
+
 	/*
 	 * We intended to create the partition with the same persistence as the
 	 * parent table, but we still need to recheck because that might be

---

From: Alexander Korotkov <[email-masked]>
Date: Sat, 10 Aug 2024 18:43:59 +0300

> the namespace in which we created it (see attachment)?

The new partition doesn't necessarily get created in the same
namespace as parent partition.  I think it would be better to somehow
open partition by its oid.

It would be quite unfortunate to replicate significant part of
ProcessUtilitySlow().  So, the question is how to get the oid of newly
created relation from ProcessUtility().  I don't like to change the
signature of ProcessUtility() especially as a part of backpatch.  So,
I tried to fit this into existing parameters.  Probably
QueryCompletion struct fits this purpose best from the existing
parameters.  Attached draft patch implements returning oid of newly
created relation as part of QueryCompletion.  Thoughts?

------
Regards,
Alexander Korotkov
Supabase

---

From: Dmitry Koval <[email-masked]>
Date: Sat, 10 Aug 2024 18:57:48 +0300

> Probably

I agree, returning the oid of the newly created relation is the best way 
to solve the problem.
(Excuse me, I won't have access to a laptop for the next week - and 
won't be able to look at the source code).

-- 
With best regards,
Dmitry Koval

---

From: Alexander Korotkov <[email-masked]>
Date: Mon, 19 Aug 2024 01:24:02 +0300

> won't be able to look at the source code).

Thank you for your feedback.  Although, I decided QueryCompletion is
not a good place for this new field.  It looks more appropriate to
place it to TableLikeClause, which already contains one relation oid
inside.  The revised patch is attached.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 21 Aug 2024 14:48:45 +0400

Hi, Alexander!

> inside.  The revised patch is attached.

I've looked at the patch v2. Remembering the OID of a relation newly
created with LIKE in TableLikeClause seems good to me.
Check-world passes sucessfully.

Shouldn't we also modify the TableLikeClause node in gram.y accordingly?

For the comments:
Put the Oid  -> Store the OID
so caller might use it -> for the caller to use it.
(Maybe also caller -> table create function)

Regards,
Pavel Borisov
Supabase

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 21 Aug 2024 14:55:04 +0300

Hi, Pavel!

On Wed, Aug 21, 2024 at 1:48 PM Pavel Borisov <[email-masked]> wrote:
> On Mon, 19 Aug 2024 at 02:24, Alexander Korotkov <[email-masked]> wrote:
>>
> I've looked at the patch v2. Remembering the OID of a relation newly created with LIKE in TableLikeClause seems good to me.

Thank you.

> Shouldn't we also modify the TableLikeClause node in gram.y accordingly?

On the one hand, makeNode() uses palloc0() and initializes all fields
with zero anyway.  On the other hand, there is already assignment of
relationOid.  So, yes I'll add assignment of newRelationOid for the
sake of uniformity.

> so caller might use it -> for the caller to use it.

Accepted.

> (Maybe also caller -> table create function)

I'll prefer to leave it "caller" as more generic term, which could
also fit potential future usages.

The revised patch is attached.  I'm going to push it if no objections.

------
Regards,
Alexander Korotkov
Supabase

---

From: Pavel Borisov <[email-masked]>
Date: Wed, 21 Aug 2024 16:06:31 +0400

Hi, Alexander!

> The revised patch is attached.  I'm going to push it if no objections.
Looked at v3
All good except the patch has "Oid" and "OID" in two comments. I suppose
"OID" is preferred elsewhere in the PG comments.

Regards,
Pavel.

---

From: Alexander Korotkov <[email-masked]>
Date: Wed, 21 Aug 2024 15:40:44 +0300

On Wed, Aug 21, 2024 at 3:06 PM Pavel Borisov <[email-masked]> wrote:
> On Wed, 21 Aug 2024 at 15:55, Alexander Korotkov <[email-masked]> wrote:
>>
>> Hi, Pavel!
>>
> All good except the patch has "Oid" and "OID" in two comments. I suppose "OID" is preferred elsewhere in the PG comments.

Correct, the same file contains "OID" multiple times.  Revised version
is attached.

------
Regards,
Alexander Korotkov
Supabase

---

From: Robert Haas <[email-masked]>
Date: Thu, 22 Aug 2024 12:33:27 -0400

Hi,

In response to some concerns raised about this fix on the
pgsql-release list today, I spent some time investigating this patch.
Unfortunately, I think there are too many problems here to be
reasonably fixed before release, and I think all of SPLIT/MERGE
PARTITION needs to be reverted.

I focused my investigation on createPartitionTable(), which is a
helper for both SPLIT PARTITION and MERGE PARTITION, and it works by
consing up a CREATE TABLE AS statement and then feeding that back
through
ProcessUtility. I think it's bad design to use such a high-level
facility here; it is unlike what we do elsewhere in tablecmds.c and
opens us up to a variety of problems. The first thing that I
discovered is that this patch does not fix all of the repeated name
lookup problems. There is still this:

    tlc->relation =
makeRangeVar(get_namespace_name(RelationGetNamespace(modelRel)),
                                 RelationGetRelationName(modelRel), -1);

And also this:

    createStmt->tablespacename =
get_tablespace_name(modelRel->rd_rel->reltablespace);

In both cases, we do a reverse lookup on an OID to get a name which
the CREATE TABLE code will later turn back into an OID. If we don't
get the same value, that's at least a bug and probably a security
vulnerability, and there is no way to be certain that we will get the
same value. The only remedy is to not repeat the lookup in the first
place.

Then I got to looking at this:

    tlc->options = CREATE_TABLE_LIKE_ALL &
        ~(CREATE_TABLE_LIKE_INDEXES | CREATE_TABLE_LIKE_IDENTITY |
CREATE_TABLE_LIKE_STATISTICS);

It's not obvious at first glance that there is a critical problem
here, but there are reasons to be nervous. We're deploying a lot of
machinery here to copy a lot of stuff and, while that's efficient from
a coding perspective, it means that stuff you might not expect can
just kind of happen. For instance:

robert.haas=# \d+
                                            List of relations
 Schema | Name |       Type        |    Owner    | Persistence |
Access method |    Size    | Description
--------+------+-------------------+-------------+-------------+---------------+------------+-------------
 public | foo  | partitioned table | robert.haas | permanent   |
        | 0 bytes    |
 public | foo1 | table             | robert.haas | permanent   | heap
        | 8192 bytes |
 public | foo2 | table             | bob         | permanent   | heap
        | 8192 bytes |
(3 rows)
robert.haas=# alter table foo split partition foo2 into (partition
foo3 for values from (10) to (15), partition foo4 for values from (15)
to (20));
ALTER TABLE
robert.haas=# \d+
                                            List of relations
 Schema | Name |       Type        |    Owner    | Persistence |
Access method |    Size    | Description
--------+------+-------------------+-------------+-------------+---------------+------------+-------------
 public | foo  | partitioned table | robert.haas | permanent   |
        | 0 bytes    |
 public | foo1 | table             | robert.haas | permanent   | heap
        | 8192 bytes |
 public | foo3 | table             | robert.haas | permanent   | heap
        | 8192 bytes |
 public | foo4 | table             | robert.haas | permanent   | heap
        | 8192 bytes |
(4 rows)

I've split a partition owned by bob into two partitions owned by
robert.haas. That's rather surprising. It doesn't work to split a
partition that I don't own (and thus gain access to it) but if the
superuser splits a non-superuser's partition, the superuser ends
upowning the new partitions. I don't know if that's a vulnerability or
just unexpected. However, then I found this, which I'm pretty well
certain is a vulnerability:

robert.haas=# set role bob;
SET
robert.haas=> create table foo (a int, b text) partition by range (a);
CREATE TABLE
robert.haas=> create table foo1 partition of foo for values from (0) to (10);
CREATE TABLE
robert.haas=> create table foo2 partition of foo for values from (10) to (20);
CREATE TABLE
robert.haas=> insert into foo values (11, 'carrots'), (16, 'pineapple');
INSERT 0 2
robert.haas=> create or replace function run_me(integer) returns
integer as $$begin raise notice 'you are running me as %',
current_user; return $1; end$$ language plpgsql immutable;
CREATE FUNCTION
robert.haas=> create index on foo (run_me(a));
NOTICE:  you are running me as bob
NOTICE:  you are running me as bob
CREATE INDEX
robert.haas=> reset role;
RESET
robert.haas=# alter table foo split partition foo2 into (partition
foo3 for values from (10) to (15), partition foo4 for values from (15)
to (20));
NOTICE:  you are running me as robert.haas
NOTICE:  you are running me as robert.haas
ALTER TABLE

I think it is very unlikely that the problems mentioned above are the
only ones. They're just what I found in an hour or two of testing.
Even if they were, we're probably too close to release to be rushing
out last minute fixes to multiple unanticipated security problems. But
because of the design that was chosen here, I think there is probably
more stuff here that is not right, some of which is security relevant
and some of which is just a question of whether we're really getting
the behavior that we want. And I don't think we can fix all that
without either a very large number of grotty hacks similar to the one
installed by 04158e7fa37c2dda9c3421ca922d02807b86df19, or a complete
redesign of the feature. I believe the latter is probably a wiser
course of action.

...Robert

---

From: "Jonathan S. Katz" <[email-masked]>
Date: Thu, 22 Aug 2024 12:43:22 -0400

> redesign of the feature. I believe the latter is probably a wiser

I can't comment on the design as much, but from a release standpoint, 
but security concerns this close to the RC/GA period do concern me.

Applying the lessons from PG15 + SQL/JSON where we (and I'll own that I 
was the one who pushed hard to include it) let it stay too long when it 
should have been reverted, I think we should take more time to work on 
this feature, revert it for PG17, and target it for PG18.

I understand it's disappointing to do a late revert of a feature, but I 
think it's better to be safer, particularly if we believe there's a an 
elevated risk of releasing something with vulnerabilities. As we saw 
with SQL/JSON, this we'll give us more time to come up with design we 
agree with, further test, and then promote as part of PG18.

Thanks,

Jonathan

---

From: Alexander Korotkov <[email-masked]>
Date: Thu, 22 Aug 2024 19:43:35 +0300

Hi!

> Access method |    Size    | Description

Thank you for your feedback.  Yes, it seems that there is not enough
time to even carefully analyze all the issues in these features.  The
rule of thumb I can get from this experience is "think multiple times
before accessing something already opened by its name".  I'm going to
revert these features during next couple days.

------
Regards,
Alexander Korotkov
Supabase