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); 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); 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" +