From a8f368fced098478f72c735378162bca4d0a2495 Mon Sep 17 00:00:00 2001 From: Marco Slot Date: Wed, 3 May 2017 21:41:12 +0200 Subject: [PATCH 1/2] Fix locking in master_drop_all_shards / master_apply_delete_command --- .../master/master_delete_protocol.c | 27 ++- .../expected/isolation_drop_shards.out | 162 ++++++++++++++++++ src/test/regress/isolation_schedule | 1 + .../regress/specs/isolation_drop_shards.spec | 71 ++++++++ 4 files changed, 253 insertions(+), 8 deletions(-) create mode 100644 src/test/regress/expected/isolation_drop_shards.out create mode 100644 src/test/regress/specs/isolation_drop_shards.spec diff --git a/src/backend/distributed/master/master_delete_protocol.c b/src/backend/distributed/master/master_delete_protocol.c index 0d3525d84..ed8418aa4 100644 --- a/src/backend/distributed/master/master_delete_protocol.c +++ b/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; - bool sessionLock = false; - bool dontWait = false; + LOCKMODE lockMode = 0; 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); diff --git a/src/test/regress/expected/isolation_drop_shards.out b/src/test/regress/expected/isolation_drop_shards.out new file mode 100644 index 000000000..ba571371a --- /dev/null +++ b/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; + +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'); + +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'); + +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; + +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; + +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'); + +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'); + +step s1-commit: + COMMIT; + +step s2-drop-all-shards: <... completed> +master_drop_all_shards + +0 diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index 51c9d4f97..0dcf6e45c 100644 --- a/src/test/regress/isolation_schedule +++ b/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 diff --git a/src/test/regress/specs/isolation_drop_shards.spec b/src/test/regress/specs/isolation_drop_shards.spec new file mode 100644 index 000000000..e57523718 --- /dev/null +++ b/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. From 37026dc3513b615c8e813955f479d394fc40af49 Mon Sep 17 00:00:00 2001 From: Metin Doslu Date: Mon, 8 May 2017 17:07:03 +0300 Subject: [PATCH 2/2] Add truncate first isolation tests --- .../expected/isolation_drop_shards.out | 80 +++++++++++++++++++ .../regress/specs/isolation_drop_shards.spec | 15 +++- 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/test/regress/expected/isolation_drop_shards.out b/src/test/regress/expected/isolation_drop_shards.out index ba571371a..d007f502c 100644 --- a/src/test/regress/expected/isolation_drop_shards.out +++ b/src/test/regress/expected/isolation_drop_shards.out @@ -160,3 +160,83 @@ step s2-drop-all-shards: <... completed> master_drop_all_shards 0 + +starting permutation: s1-begin s1-truncate s2-truncate s1-commit +?column? + +1 +step s1-begin: + BEGIN; + +step s1-truncate: + TRUNCATE append_table; + +step s2-truncate: + TRUNCATE append_table; + +step s1-commit: + COMMIT; + +step s2-truncate: <... completed> + +starting permutation: s1-begin s1-truncate s2-apply-delete-command s1-commit +?column? + +1 +step s1-begin: + BEGIN; + +step s1-truncate: + TRUNCATE append_table; + +step s2-apply-delete-command: + SELECT master_apply_delete_command('DELETE FROM append_table'); + +step s1-commit: + COMMIT; + +step s2-apply-delete-command: <... completed> +master_apply_delete_command + +0 + +starting permutation: s1-begin s1-truncate s2-drop-all-shards s1-commit +?column? + +1 +step s1-begin: + BEGIN; + +step s1-truncate: + TRUNCATE append_table; + +step s2-drop-all-shards: + SELECT master_drop_all_shards('append_table', 'public', 'append_table'); + +step s1-commit: + COMMIT; + +step s2-drop-all-shards: <... completed> +master_drop_all_shards + +0 + +starting permutation: s1-begin s1-truncate s2-select s1-commit +?column? + +1 +step s1-begin: + BEGIN; + +step s1-truncate: + TRUNCATE append_table; + +step s2-select: + SELECT * FROM append_table; + +step s1-commit: + COMMIT; + +step s2-select: <... completed> +test_id data + diff --git a/src/test/regress/specs/isolation_drop_shards.spec b/src/test/regress/specs/isolation_drop_shards.spec index e57523718..fd9fce2d7 100644 --- a/src/test/regress/specs/isolation_drop_shards.spec +++ b/src/test/regress/specs/isolation_drop_shards.spec @@ -20,6 +20,11 @@ step "s1-begin" BEGIN; } +step "s1-truncate" +{ + TRUNCATE append_table; +} + step "s1-apply-delete-command" { SELECT master_apply_delete_command('DELETE FROM append_table'); @@ -62,10 +67,14 @@ 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" +# 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. 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. +permutation "s1-begin" "s1-truncate" "s2-truncate" "s1-commit" +permutation "s1-begin" "s1-truncate" "s2-apply-delete-command" "s1-commit" +permutation "s1-begin" "s1-truncate" "s2-drop-all-shards" "s1-commit" +permutation "s1-begin" "s1-truncate" "s2-select" "s1-commit"