Fix locking in master_drop_all_shards / master_apply_delete_command

src/backend/distributed/master/master_delete_protocol.c

@@ -51,6 +51,7 @@
 #include "optimizer/predtest.h"
 #include "optimizer/restrictinfo.h"
 #include "storage/lock.h"
+#include "storage/lmgr.h"
 #include "tcop/tcopprot.h"
 #include "utils/array.h"
 #include "utils/builtins.h"
@@ -105,9 +106,7 @@ master_apply_delete_command(PG_FUNCTION_ARGS)
 	Node *queryTreeNode = NULL;
 	DeleteStmt *deleteStatement = NULL;
 	int droppedShardCount = 0;
-	LOCKTAG lockTag;
+	LOCKMODE lockMode = 0;
-	bool sessionLock = false;
-	bool dontWait = false;
 	char partitionMethod = 0;
 	bool failOK = false;
 
@@ -124,7 +123,15 @@ master_apply_delete_command(PG_FUNCTION_ARGS)
 
 	schemaName = deleteStatement->relation->schemaname;
 	relationName = deleteStatement->relation->relname;
-	relationId = RangeVarGetRelid(deleteStatement->relation, NoLock, failOK);
+
+	/*
+	 * We take an exclusive lock while dropping shards to prevent concurrent
+	 * writes. We don't want to block SELECTs, which means queries might fail
+	 * if they access a shard that has just been dropped.
+	 */
+	lockMode = ExclusiveLock;
+
+	relationId = RangeVarGetRelid(deleteStatement->relation, lockMode, failOK);
 
 	/* schema-prefix if it is not specified already */
 	if (schemaName == NULL)
@@ -166,10 +173,6 @@ master_apply_delete_command(PG_FUNCTION_ARGS)
 	CheckDeleteCriteria(deleteCriteria);
 	CheckPartitionColumn(relationId, deleteCriteria);
 
-	/* acquire lock */
-	SET_LOCKTAG_ADVISORY(lockTag, MyDatabaseId, relationId, 0, 0);
-	LockAcquire(&lockTag, ExclusiveLock, sessionLock, dontWait);
-
 	shardIntervalList = LoadShardIntervalList(relationId);
 
 	/* drop all shards if where clause is not present */
@@ -214,6 +217,14 @@ master_drop_all_shards(PG_FUNCTION_ARGS)
 
 	CheckTableSchemaNameForDrop(relationId, &schemaName, &relationName);
 
+	/*
+	 * master_drop_all_shards is typically called from the DROP TABLE trigger,
+	 * but could be called by a user directly. Make sure we have an
+	 * AccessExlusiveLock to prevent any other commands from running on this table
+	 * concurrently.
+	 */
+	LockRelationOid(relationId, AccessExclusiveLock);
+
 	shardIntervalList = LoadShardIntervalList(relationId);
 	droppedShardCount = DropShards(relationId, schemaName, relationName,
 								   shardIntervalList);

src/test/regress/expected/isolation_drop_shards.out

@@ -0,0 +1,162 @@
+Parsed test spec with 2 sessions
+
+starting permutation: s1-begin s1-drop-all-shards s2-truncate s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+
+master_drop_all_shards
+
+16             
+step s2-truncate: 
+	TRUNCATE append_table;
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-truncate: <... completed>
+
+starting permutation: s1-begin s1-drop-all-shards s2-apply-delete-command s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+
+master_drop_all_shards
+
+16             
+step s2-apply-delete-command: 
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-apply-delete-command: <... completed>
+master_apply_delete_command
+
+0              
+
+starting permutation: s1-begin s1-drop-all-shards s2-drop-all-shards s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+
+master_drop_all_shards
+
+16             
+step s2-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-drop-all-shards: <... completed>
+master_drop_all_shards
+
+0              
+
+starting permutation: s1-begin s1-drop-all-shards s2-select s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+
+master_drop_all_shards
+
+16             
+step s2-select: 
+	SELECT * FROM append_table;
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-select: <... completed>
+test_id        data           
+
+
+starting permutation: s1-begin s1-apply-delete-command s2-truncate s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-apply-delete-command: 
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+
+master_apply_delete_command
+
+16             
+step s2-truncate: 
+	TRUNCATE append_table;
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-truncate: <... completed>
+
+starting permutation: s1-begin s1-apply-delete-command s2-apply-delete-command s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-apply-delete-command: 
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+
+master_apply_delete_command
+
+16             
+step s2-apply-delete-command: 
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-apply-delete-command: <... completed>
+master_apply_delete_command
+
+0              
+
+starting permutation: s1-begin s1-apply-delete-command s2-drop-all-shards s1-commit
+?column?       
+
+1              
+step s1-begin: 
+	BEGIN;
+
+step s1-apply-delete-command: 
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+
+master_apply_delete_command
+
+16             
+step s2-drop-all-shards: 
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+ <waiting ...>
+step s1-commit: 
+	COMMIT;
+
+step s2-drop-all-shards: <... completed>
+master_drop_all_shards
+
+0              

src/test/regress/isolation_schedule

@@ -1,3 +1,4 @@
 test: isolation_cluster_management
 test: isolation_dml_vs_repair
 test: isolation_concurrent_dml
+test: isolation_drop_shards

src/test/regress/specs/isolation_drop_shards.spec

@@ -0,0 +1,71 @@
+setup
+{
+	CREATE TABLE append_table (test_id integer NOT NULL, data text);
+	SELECT create_distributed_table('append_table', 'test_id', 'append');
+
+	SELECT 1 FROM (
+		SELECT min(master_create_empty_shard('append_table')) FROM generate_series(1,16)
+	) a;
+}
+
+teardown
+{
+	DROP TABLE append_table;
+}
+
+session "s1"
+
+step "s1-begin"
+{
+	BEGIN;
+}
+
+step "s1-apply-delete-command"
+{
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+}
+
+step "s1-drop-all-shards"
+{
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+}
+
+step "s1-commit"
+{
+	COMMIT;
+}
+
+session "s2"
+
+step "s2-truncate"
+{
+	TRUNCATE append_table;
+}
+
+step "s2-apply-delete-command"
+{
+	SELECT master_apply_delete_command('DELETE FROM append_table');
+}
+
+step "s2-drop-all-shards"
+{
+	SELECT master_drop_all_shards('append_table', 'public', 'append_table');
+}
+
+step "s2-select"
+{
+	SELECT * FROM append_table;
+}
+
+permutation "s1-begin" "s1-drop-all-shards" "s2-truncate" "s1-commit"
+permutation "s1-begin" "s1-drop-all-shards" "s2-apply-delete-command" "s1-commit"
+permutation "s1-begin" "s1-drop-all-shards" "s2-drop-all-shards" "s1-commit"
+permutation "s1-begin" "s1-drop-all-shards" "s2-select" "s1-commit"
+
+permutation "s1-begin" "s1-apply-delete-command" "s2-truncate" "s1-commit"
+permutation "s1-begin" "s1-apply-delete-command" "s2-apply-delete-command" "s1-commit"
+permutation "s1-begin" "s1-apply-delete-command" "s2-drop-all-shards" "s1-commit"
+
+# We can't verify master_apply_delete_command + SELECT since it blocks on the
+# the workers, but this is not visible on the master, meaning the isolation
+# test cannot proceed.