From cb6070c72096432e511b970e7c9260c9a03fdfe2 Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 10 Jul 2017 13:55:34 +0300 Subject: [PATCH 1/3] Use ShareUpdateExclusiveLock instead ShareLock in VACUUM Before this change, we used ShareLock to acquire lock on distributed tables while running VACUUM. This makes VACUUM and INSERT block each other. With this change we changed lock mode from ShareLock to ShareUpdateExclusiveLock, which does not conflict with the locks INSERT acquire. --- src/backend/distributed/executor/multi_utility.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/backend/distributed/executor/multi_utility.c b/src/backend/distributed/executor/multi_utility.c index cdc4e7f38..7e69ea697 100644 --- a/src/backend/distributed/executor/multi_utility.c +++ b/src/backend/distributed/executor/multi_utility.c @@ -1331,8 +1331,13 @@ VacuumTaskList(Oid relationId, VacuumStmt *vacuumStmt) char *schemaName = get_namespace_name(schemaId); char *tableName = get_rel_name(relationId); - /* lock relation metadata before getting shard list */ - LockRelationDistributionMetadata(relationId, ShareLock); + /* + * We obtain ShareUpdateExclusiveLock here to not conflict with INSERT's + * RowExclusiveLock. However if VACUUM FULL is used, we already obtain + * AccessExclusiveLock before reaching to that point and INSERT's will be + * blocked anyway. This is inline with PostgreSQL's own behaviour. + */ + LockRelationOid(relationId, ShareUpdateExclusiveLock); shardIntervalList = LoadShardIntervalList(relationId); From c8b9e4011b51c184ac9e37e57df3bce10357c309 Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 10 Jul 2017 15:46:37 +0300 Subject: [PATCH 2/3] Remove LockRelationDistributionMetadata function --- .../distributed/master/master_create_shards.c | 12 ++++++------ src/backend/distributed/utils/resource_lock.c | 15 --------------- src/include/distributed/resource_lock.h | 1 - 3 files changed, 6 insertions(+), 22 deletions(-) diff --git a/src/backend/distributed/master/master_create_shards.c b/src/backend/distributed/master/master_create_shards.c index a56f283bf..9f752244c 100644 --- a/src/backend/distributed/master/master_create_shards.c +++ b/src/backend/distributed/master/master_create_shards.c @@ -113,8 +113,8 @@ CreateShardsWithRoundRobinPolicy(Oid distributedTableId, int32 shardCount, */ EnsureTableOwner(distributedTableId); - /* we plan to add shards: get an exclusive metadata lock */ - LockRelationDistributionMetadata(distributedTableId, ExclusiveLock); + /* we plan to add shards: get an exclusive lock on relation oid */ + LockRelationOid(distributedTableId, ExclusiveLock); relationOwner = TableOwner(distributedTableId); @@ -264,8 +264,8 @@ CreateColocatedShards(Oid targetRelationId, Oid sourceRelationId) */ EnsureTableOwner(targetRelationId); - /* we plan to add shards: get an exclusive metadata lock on the target relation */ - LockRelationDistributionMetadata(targetRelationId, ExclusiveLock); + /* we plan to add shards: get an exclusive lock on target relation oid */ + LockRelationOid(targetRelationId, ExclusiveLock); /* we don't want source table to get dropped before we colocate with it */ LockRelationOid(sourceRelationId, AccessShareLock); @@ -369,8 +369,8 @@ CreateReferenceTableShard(Oid distributedTableId) */ EnsureTableOwner(distributedTableId); - /* we plan to add shards: get an exclusive metadata lock */ - LockRelationDistributionMetadata(distributedTableId, ExclusiveLock); + /* we plan to add shards: get an exclusive lock on relation oid */ + LockRelationOid(distributedTableId, ExclusiveLock); relationOwner = TableOwner(distributedTableId); diff --git a/src/backend/distributed/utils/resource_lock.c b/src/backend/distributed/utils/resource_lock.c index 15daf3d53..296508e9d 100644 --- a/src/backend/distributed/utils/resource_lock.c +++ b/src/backend/distributed/utils/resource_lock.c @@ -184,21 +184,6 @@ TryLockShardDistributionMetadata(int64 shardId, LOCKMODE lockMode) } -/* - * LockRelationDistributionMetadata returns after getting a the lock used for a - * relation's distribution metadata, blocking if required. Only ExclusiveLock - * and ShareLock modes are supported. Any locks acquired using this method are - * released at transaction end. - */ -void -LockRelationDistributionMetadata(Oid relationId, LOCKMODE lockMode) -{ - Assert(lockMode == ExclusiveLock || lockMode == ShareLock); - - (void) LockRelationOid(relationId, lockMode); -} - - /* * LockShardResource acquires a lock needed to modify data on a remote shard. * This task may be assigned to multiple backends at the same time, so the lock diff --git a/src/include/distributed/resource_lock.h b/src/include/distributed/resource_lock.h index 67d02a037..31dfff855 100644 --- a/src/include/distributed/resource_lock.h +++ b/src/include/distributed/resource_lock.h @@ -66,7 +66,6 @@ typedef enum AdvisoryLocktagClass /* Lock shard/relation metadata for safe modifications */ extern void LockShardDistributionMetadata(int64 shardId, LOCKMODE lockMode); extern bool TryLockShardDistributionMetadata(int64 shardId, LOCKMODE lockMode); -extern void LockRelationDistributionMetadata(Oid relationId, LOCKMODE lockMode); /* Lock shard data, for DML commands or remote fetches */ extern void LockShardResource(uint64 shardId, LOCKMODE lockmode); From a15b3c6df257de9f69c52d059828beacb2d2f835 Mon Sep 17 00:00:00 2001 From: Burak Yucesoy Date: Mon, 10 Jul 2017 15:46:48 +0300 Subject: [PATCH 3/3] Add tests for concurrent INSERT and VACUUM behaviour --- .../expected/isolation_insert_vs_vacuum.out | 36 ++++++++++++++ src/test/regress/isolation_schedule | 2 + .../specs/isolation_insert_vs_vacuum.spec | 47 +++++++++++++++++++ 3 files changed, 85 insertions(+) create mode 100644 src/test/regress/expected/isolation_insert_vs_vacuum.out create mode 100644 src/test/regress/specs/isolation_insert_vs_vacuum.spec diff --git a/src/test/regress/expected/isolation_insert_vs_vacuum.out b/src/test/regress/expected/isolation_insert_vs_vacuum.out new file mode 100644 index 000000000..eb6c481b1 --- /dev/null +++ b/src/test/regress/expected/isolation_insert_vs_vacuum.out @@ -0,0 +1,36 @@ +Parsed test spec with 2 sessions + +starting permutation: s1-begin s1-insert s2-vacuum-analyze s1-commit +create_distributed_table + + +step s1-begin: + BEGIN; + +step s1-insert: + INSERT INTO test_insert_vacuum VALUES(1, 1); + +step s2-vacuum-analyze: + VACUUM ANALYZE test_insert_vacuum; + +step s1-commit: + COMMIT; + + +starting permutation: s1-begin s1-insert s2-vacuum-full s1-commit +create_distributed_table + + +step s1-begin: + BEGIN; + +step s1-insert: + INSERT INTO test_insert_vacuum VALUES(1, 1); + +step s2-vacuum-full: + VACUUM FULL test_insert_vacuum; + +step s1-commit: + COMMIT; + +step s2-vacuum-full: <... completed> diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index d4077e44e..143dc6e86 100644 --- a/src/test/regress/isolation_schedule +++ b/src/test/regress/isolation_schedule @@ -8,3 +8,5 @@ test: isolation_cluster_management test: isolation_dml_vs_repair isolation_copy_placement_vs_copy_placement isolation_cancellation test: isolation_concurrent_dml isolation_data_migration test: isolation_drop_shards isolation_copy_placement_vs_modification + +test: isolation_insert_vs_vacuum diff --git a/src/test/regress/specs/isolation_insert_vs_vacuum.spec b/src/test/regress/specs/isolation_insert_vs_vacuum.spec new file mode 100644 index 000000000..050e6dc82 --- /dev/null +++ b/src/test/regress/specs/isolation_insert_vs_vacuum.spec @@ -0,0 +1,47 @@ +setup +{ + SET citus.shard_replication_factor TO 1; + CREATE TABLE test_insert_vacuum(column1 int, column2 int); + SELECT create_distributed_table('test_insert_vacuum', 'column1'); +} + +teardown +{ + DROP TABLE test_insert_vacuum; +} + +session "s1" + +step "s1-begin" +{ + BEGIN; +} + +step "s1-insert" +{ + INSERT INTO test_insert_vacuum VALUES(1, 1); +} + +step "s1-commit" +{ + COMMIT; +} + +session "s2" + +step "s2-vacuum-analyze" +{ + VACUUM ANALYZE test_insert_vacuum; +} + +step "s2-vacuum-full" +{ + VACUUM FULL test_insert_vacuum; +} + +# INSERT and VACUUM ANALYZE should not block each other. +permutation "s1-begin" "s1-insert" "s2-vacuum-analyze" "s1-commit" + +# INSERT and VACUUM FULL should block each other. +permutation "s1-begin" "s1-insert" "s2-vacuum-full" "s1-commit" +