On Thu, Apr 14, 2022 at 10:37:06PM +0200, Alvaro Herrera wrote: > Thanks for the patch -- I have pushed it now, with some wording changes > and renaming the role to regress_* to avoid buildfarm's ire.
Cool, thanks. > Michaël in addition proposes an isolation test. I'm not sure; is it > worth the additional test run time? It doesn't seem a critical issue. > But if anybody feels like contributing one, step right ahead. Well, I am a bit annoyed that we don't actually check that a CLUSTER command does not block when doing a CLUSTER on a partitioned table while a lock is held on one of its partitions. So, attached is a proposal of patch to improve the test coverage in this area. While on it, I have added a test with a normal table. You can see the difference once you remove the ACL check added recently in get_tables_to_cluster_partitioned(). What do you think? -- Michael
From fdf5347bf4853f19341a8d67a655cae9fece15f0 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Sat, 16 Apr 2022 20:52:03 +0900
Subject: [PATCH] Add isolation tests for CLUSTER with partitions
---
.../expected/cluster-conflict-partition.out | 35 ++++++++++++++++++
.../isolation/expected/cluster-conflict.out | 19 ++++++++++
src/test/isolation/isolation_schedule | 2 +
.../specs/cluster-conflict-partition.spec | 37 +++++++++++++++++++
.../isolation/specs/cluster-conflict.spec | 30 +++++++++++++++
5 files changed, 123 insertions(+)
create mode 100644 src/test/isolation/expected/cluster-conflict-partition.out
create mode 100644 src/test/isolation/expected/cluster-conflict.out
create mode 100644 src/test/isolation/specs/cluster-conflict-partition.spec
create mode 100644 src/test/isolation/specs/cluster-conflict.spec
diff --git a/src/test/isolation/expected/cluster-conflict-partition.out b/src/test/isolation/expected/cluster-conflict-partition.out
new file mode 100644
index 0000000000..7acb675c97
--- /dev/null
+++ b/src/test/isolation/expected/cluster-conflict-partition.out
@@ -0,0 +1,35 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_auth: SET ROLE regress_cluster_part;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
+step s1_commit: COMMIT;
+step s2_cluster: <... completed>
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_cluster_part;
+step s1_lock_parent: LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind; <waiting ...>
+step s1_commit: COMMIT;
+step s2_cluster: <... completed>
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_auth: SET ROLE regress_cluster_part;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_cluster_part;
+step s1_lock_child: LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_cluster: CLUSTER cluster_part_tab USING cluster_part_ind;
+step s1_commit: COMMIT;
+step s2_reset: RESET ROLE;
diff --git a/src/test/isolation/expected/cluster-conflict.out b/src/test/isolation/expected/cluster-conflict.out
new file mode 100644
index 0000000000..614d8f9d15
--- /dev/null
+++ b/src/test/isolation/expected/cluster-conflict.out
@@ -0,0 +1,19 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1_begin s1_lock s2_auth s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s1_lock: LOCK cluster_tab IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_auth: SET ROLE regress_cluster_conflict;
+step s2_cluster: CLUSTER cluster_tab USING cluster_ind; <waiting ...>
+step s1_commit: COMMIT;
+step s2_cluster: <... completed>
+step s2_reset: RESET ROLE;
+
+starting permutation: s1_begin s2_auth s1_lock s2_cluster s1_commit s2_reset
+step s1_begin: BEGIN;
+step s2_auth: SET ROLE regress_cluster_conflict;
+step s1_lock: LOCK cluster_tab IN SHARE UPDATE EXCLUSIVE MODE;
+step s2_cluster: CLUSTER cluster_tab USING cluster_ind; <waiting ...>
+step s1_commit: COMMIT;
+step s2_cluster: <... completed>
+step s2_reset: RESET ROLE;
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 607760386e..529a4cbd4d 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -102,6 +102,8 @@ test: partition-key-update-2
test: partition-key-update-3
test: partition-key-update-4
test: plpgsql-toast
+test: cluster-conflict
+test: cluster-conflict-partition
test: truncate-conflict
test: serializable-parallel
test: serializable-parallel-2
diff --git a/src/test/isolation/specs/cluster-conflict-partition.spec b/src/test/isolation/specs/cluster-conflict-partition.spec
new file mode 100644
index 0000000000..5091f684a9
--- /dev/null
+++ b/src/test/isolation/specs/cluster-conflict-partition.spec
@@ -0,0 +1,37 @@
+# Tests for locking conflicts with CLUSTER command and partitions.
+
+setup
+{
+ CREATE ROLE regress_cluster_part;
+ CREATE TABLE cluster_part_tab (a int) PARTITION BY LIST (a);
+ CREATE TABLE cluster_part_tab1 PARTITION OF cluster_part_tab FOR VALUES IN (1);
+ CREATE TABLE cluster_part_tab2 PARTITION OF cluster_part_tab FOR VALUES IN (2);
+ CREATE INDEX cluster_part_ind ON cluster_part_tab(a);
+ ALTER TABLE cluster_part_tab OWNER TO regress_cluster_part;
+}
+
+teardown
+{
+ DROP TABLE cluster_part_tab;
+ DROP ROLE regress_cluster_part;
+}
+
+session s1
+step s1_begin { BEGIN; }
+step s1_lock_parent { LOCK cluster_part_tab IN SHARE UPDATE EXCLUSIVE MODE; }
+step s1_lock_child { LOCK cluster_part_tab1 IN SHARE UPDATE EXCLUSIVE MODE; }
+step s1_commit { COMMIT; }
+
+session s2
+step s2_auth { SET ROLE regress_cluster_part; }
+step s2_cluster { CLUSTER cluster_part_tab USING cluster_part_ind; }
+step s2_reset { RESET ROLE; }
+
+# CLUSTER on the parent waits if locked, passes for all cases.
+permutation s1_begin s1_lock_parent s2_auth s2_cluster s1_commit s2_reset
+permutation s1_begin s2_auth s1_lock_parent s2_cluster s1_commit s2_reset
+
+# When taking a lock on a partition leaf, CLUSTER on the parent skips
+# the leaf, passes for all cases.
+permutation s1_begin s1_lock_child s2_auth s2_cluster s1_commit s2_reset
+permutation s1_begin s2_auth s1_lock_child s2_cluster s1_commit s2_reset
diff --git a/src/test/isolation/specs/cluster-conflict.spec b/src/test/isolation/specs/cluster-conflict.spec
new file mode 100644
index 0000000000..2e1d547f01
--- /dev/null
+++ b/src/test/isolation/specs/cluster-conflict.spec
@@ -0,0 +1,30 @@
+# Tests for locking conflicts with CLUSTER command.
+
+setup
+{
+ CREATE ROLE regress_cluster_conflict;
+ CREATE TABLE cluster_tab (a int);
+ CREATE INDEX cluster_ind ON cluster_tab(a);
+ ALTER TABLE cluster_tab OWNER TO regress_cluster_conflict;
+}
+
+teardown
+{
+ DROP TABLE cluster_tab;
+ DROP ROLE regress_cluster_conflict;
+}
+
+session s1
+step s1_begin { BEGIN; }
+step s1_lock { LOCK cluster_tab IN SHARE UPDATE EXCLUSIVE MODE; }
+step s1_commit { COMMIT; }
+
+session s2
+step s2_auth { SET ROLE regress_cluster_conflict; }
+step s2_cluster { CLUSTER cluster_tab USING cluster_ind; }
+step s2_reset { RESET ROLE; }
+
+# The role has privileges to cluster the table, CLUSTER will block if
+# another session holds a lock on the table and succeed in all cases.
+permutation s1_begin s1_lock s2_auth s2_cluster s1_commit s2_reset
+permutation s1_begin s2_auth s1_lock s2_cluster s1_commit s2_reset
--
2.35.2
signature.asc
Description: PGP signature
