public inbox for [email protected]
help / color / mirror / Atom feedFrom: Noah Misch <[email protected]>
To: Alexander Korotkov <[email protected]>
Cc: Dmitry Koval <[email protected]>
Cc: [email protected]
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
Date: Thu, 8 Aug 2024 10:13:51 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <CAPpHfdtj7YsPaASoVPN+N3H4_Ct+kQw8QY1d_9u7FPnbghkicw@mail.gmail.com>
References: <171085360143.2046436.7217841141682511557.pgcf@coridan.postgresql.org>
<171085858750.2046434.12246066729700037354.pgcf@coridan.postgresql.org>
<[email protected]>
<CAPpHfdsVQwnZPxaQPwcu1g_01F0d-o=ECRcf14X3CmavJB6vSQ@mail.gmail.com>
<[email protected]>
<CAPpHfdsCCV8DOv0Ovu4dTSnuWXiEz_AjuW-J6TPj++tcfEGvRw@mail.gmail.com>
<[email protected]>
<CAPpHfduNYkMVuUSxZjD6u4tZG6aKdhgQcmhr-K9OQfeg56MNww@mail.gmail.com>
<[email protected]>
<CAPpHfdtj7YsPaASoVPN+N3H4_Ct+kQw8QY1d_9u7FPnbghkicw@mail.gmail.com>
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
Attachments:
[text/plain] repro-merge-partition-race-v0.patch (6.0K, ../[email protected]/2-repro-merge-partition-race-v0.patch)
download | inline diff:
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
view thread (20+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add SPLIT PARTITION/MERGE PARTITIONS commands
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox