From 8194dc4c624ea3d8c6ef7bb65e3051aeb2c0aba8 Mon Sep 17 00:00:00 2001 From: aykutbozkurt Date: Tue, 14 Jun 2022 21:54:25 +0300 Subject: [PATCH] * Added isolation tests for vacuum, * Added more regression tests for more vacuum options, * Fixed deadlock for unqualified vacuum when there is only 1 worker, * Supported lock_skipped for vacuum. --- src/backend/distributed/commands/vacuum.c | 43 ++++- .../expected/isolation_vacuum_skip_locked.out | 171 ++++++++++++++++++ src/test/regress/expected/multi_utilities.out | 97 +++++++++- src/test/regress/isolation_schedule | 2 +- .../spec/isolation_vacuum_skip_locked.spec | 61 +++++++ src/test/regress/sql/multi_utilities.sql | 64 ++++++- 6 files changed, 430 insertions(+), 8 deletions(-) create mode 100644 src/test/regress/expected/isolation_vacuum_skip_locked.out create mode 100644 src/test/regress/spec/isolation_vacuum_skip_locked.spec diff --git a/src/backend/distributed/commands/vacuum.c b/src/backend/distributed/commands/vacuum.c index 31300f353..972a21a5f 100644 --- a/src/backend/distributed/commands/vacuum.c +++ b/src/backend/distributed/commands/vacuum.c @@ -127,6 +127,8 @@ VacuumRelationIdList(VacuumStmt *vacuumStmt, CitusVacuumParams vacuumParams) LOCKMODE lockMode = (vacuumParams.options & VACOPT_FULL) ? AccessExclusiveLock : ShareUpdateExclusiveLock; + bool skipLocked = (vacuumParams.options & VACOPT_SKIP_LOCKED); + List *vacuumRelationList = ExtractVacuumTargetRels(vacuumStmt); List *relationIdList = NIL; @@ -134,8 +136,19 @@ VacuumRelationIdList(VacuumStmt *vacuumStmt, CitusVacuumParams vacuumParams) RangeVar *vacuumRelation = NULL; foreach_ptr(vacuumRelation, vacuumRelationList) { - Oid relationId = RangeVarGetRelid(vacuumRelation, lockMode, false); - relationIdList = lappend_oid(relationIdList, relationId); + /* + * If skip_locked option is enabled, we are skipping that relation + * if the lock for it is currently not available; else, we get the lock. + */ + Oid relationId = RangeVarGetRelidExtended(vacuumRelation, + lockMode, + skipLocked ? RVR_SKIP_LOCKED : 0, NULL, + NULL); + + if (OidIsValid(relationId)) + { + relationIdList = lappend_oid(relationIdList, relationId); + } } return relationIdList; @@ -198,6 +211,9 @@ ExecuteVacuumOnDistributedTables(VacuumStmt *vacuumStmt, List *relationIdList, static List * VacuumTaskList(Oid relationId, CitusVacuumParams vacuumParams, List *vacuumColumnList) { + LOCKMODE lockMode = (vacuumParams.options & VACOPT_FULL) ? AccessExclusiveLock : + ShareUpdateExclusiveLock; + /* resulting task list */ List *taskList = NIL; @@ -216,8 +232,20 @@ VacuumTaskList(Oid relationId, CitusVacuumParams vacuumParams, List *vacuumColum * 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. + * Also note that if skip locked option is enabled, we try to acquire the lock + * in nonblocking way. If lock is not available, vacuum just skip that relation. */ - LockRelationOid(relationId, ShareUpdateExclusiveLock); + if (!(vacuumParams.options & VACOPT_SKIP_LOCKED)) + { + LockRelationOid(relationId, lockMode); + } + else + { + if (!ConditionalLockRelationOid(relationId, lockMode)) + { + return NIL; + } + } List *shardIntervalList = LoadShardIntervalList(relationId); @@ -601,6 +629,7 @@ ExecuteUnqualifiedVacuumTasks(VacuumStmt *vacuumStmt, CitusVacuumParams vacuumPa task->cannotBeExecutedInTransction = ((vacuumParams.options) & VACOPT_VACUUM); + bool hasPeerWorker = false; int32 localNodeGroupId = GetLocalGroupId(); WorkerNode *workerNode = NULL; @@ -615,9 +644,13 @@ ExecuteUnqualifiedVacuumTasks(VacuumStmt *vacuumStmt, CitusVacuumParams vacuumPa task->taskPlacementList = lappend(task->taskPlacementList, targetPlacement); + hasPeerWorker = true; } } - bool localExecution = false; - ExecuteUtilityTaskList(list_make1(task), localExecution); + if (hasPeerWorker) + { + bool localExecution = false; + ExecuteUtilityTaskList(list_make1(task), localExecution); + } } diff --git a/src/test/regress/expected/isolation_vacuum_skip_locked.out b/src/test/regress/expected/isolation_vacuum_skip_locked.out new file mode 100644 index 000000000..5e27966ee --- /dev/null +++ b/src/test/regress/expected/isolation_vacuum_skip_locked.out @@ -0,0 +1,171 @@ +Parsed test spec with 2 sessions + +starting permutation: lock_share vac_specified commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_specified: VACUUM (SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_share vac_all_parts commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +step vac_all_parts: VACUUM (SKIP_LOCKED) parted; +step commit: + COMMIT; + + +starting permutation: lock_share analyze_specified commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +s2: WARNING: skipping analyze of "part1" --- lock not available +step analyze_specified: ANALYZE (SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_share analyze_all_parts commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +step analyze_all_parts: ANALYZE (SKIP_LOCKED) parted; +step commit: + COMMIT; + + +starting permutation: lock_share vac_analyze_specified commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_analyze_specified: VACUUM (ANALYZE, SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_share vac_analyze_all_parts commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +step vac_analyze_all_parts: VACUUM (ANALYZE, SKIP_LOCKED) parted; +step commit: + COMMIT; + + +starting permutation: lock_share vac_full_specified commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_full_specified: VACUUM (SKIP_LOCKED, FULL) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_share vac_full_all_parts commit +step lock_share: + BEGIN; + LOCK part1 IN SHARE MODE; + +step vac_full_all_parts: VACUUM (SKIP_LOCKED, FULL) parted; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive vac_specified commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_specified: VACUUM (SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive vac_all_parts commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +step vac_all_parts: VACUUM (SKIP_LOCKED) parted; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive analyze_specified commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +s2: WARNING: skipping analyze of "part1" --- lock not available +step analyze_specified: ANALYZE (SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive analyze_all_parts commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +step analyze_all_parts: ANALYZE (SKIP_LOCKED) parted; +step commit: + COMMIT; + +step analyze_all_parts: <... completed> + +starting permutation: lock_access_exclusive vac_analyze_specified commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_analyze_specified: VACUUM (ANALYZE, SKIP_LOCKED) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive vac_analyze_all_parts commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +step vac_analyze_all_parts: VACUUM (ANALYZE, SKIP_LOCKED) parted; +step commit: + COMMIT; + +step vac_analyze_all_parts: <... completed> + +starting permutation: lock_access_exclusive vac_full_specified commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +s2: WARNING: skipping vacuum of "part1" --- lock not available +step vac_full_specified: VACUUM (SKIP_LOCKED, FULL) part1, part2; +step commit: + COMMIT; + + +starting permutation: lock_access_exclusive vac_full_all_parts commit +step lock_access_exclusive: + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; + +step vac_full_all_parts: VACUUM (SKIP_LOCKED, FULL) parted; +step commit: + COMMIT; + diff --git a/src/test/regress/expected/multi_utilities.out b/src/test/regress/expected/multi_utilities.out index ce03e3c18..0c3d86602 100644 --- a/src/test/regress/expected/multi_utilities.out +++ b/src/test/regress/expected/multi_utilities.out @@ -310,7 +310,7 @@ SET citus.shard_count TO 1; SET citus.shard_replication_factor TO 1; SET citus.next_shard_id TO 970000; SET citus.log_remote_commands TO OFF; -CREATE TABLE local_vacuum_table(id int); +CREATE TABLE local_vacuum_table(id int primary key, b text); CREATE TABLE reference_vacuum_table(id int); SELECT create_reference_table('reference_vacuum_table'); create_reference_table @@ -341,7 +341,23 @@ DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx NOTICE: issuing SET citus.enable_ddl_propagation TO 'on' DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx -- should not propagate because no distributed table is specified +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; VACUUM local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + pg_size_pretty +--------------------------------------------------------------------- + 21 MB +(1 row) + +-- vacuum full deallocates pages of dead tuples whereas normal vacuum only marks dead tuples on visibility map +VACUUM FULL local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + pg_size_pretty +--------------------------------------------------------------------- + 16 kB +(1 row) + -- should propagate to all workers because table is reference table VACUUM reference_vacuum_table; NOTICE: issuing VACUUM public.reference_vacuum_table_970000 @@ -366,6 +382,85 @@ NOTICE: issuing VACUUM public.reference_vacuum_table_970000 DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx NOTICE: issuing VACUUM public.reference_vacuum_table_970000 DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx +-- vacuum (disable_page_skipping) aggressively process pages of the relation, it does not respect visibility map +VACUUM (DISABLE_PAGE_SKIPPING true) local_vacuum_table; +VACUUM (DISABLE_PAGE_SKIPPING false) local_vacuum_table; +-- vacuum (index_cleanup on, parallel 1) should execute index vacuuming and index cleanup phases in parallel +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +VACUUM (INDEX_CLEANUP OFF, PARALLEL 1) local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + pg_size_pretty +--------------------------------------------------------------------- + 56 MB +(1 row) + +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +VACUUM (INDEX_CLEANUP ON, PARALLEL 1) local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + pg_size_pretty +--------------------------------------------------------------------- + 21 MB +(1 row) + +----------------- PROCESS_TOAST is only available for pg14 +-- vacuum (process_toast false) should not be vacuuming toast tables (default is true) +--select reltoastrelid from pg_class where relname='local_vacuum_table' +--\gset +--SELECT relfrozenxid AS frozenxid FROM pg_class WHERE oid=:reltoastrelid::regclass +--\gset +--VACUUM (FREEZE, PROCESS_TOAST true) local_vacuum_table; +--SELECT relfrozenxid::text::integer > :frozenxid AS frozen_performed FROM pg_class +--WHERE oid=:reltoastrelid::regclass; +--SELECT relfrozenxid AS frozenxid FROM pg_class WHERE oid=:reltoastrelid::regclass +--\gset +--VACUUM (FREEZE, PROCESS_TOAST false) local_vacuum_table; +--SELECT relfrozenxid::text::integer = :frozenxid AS frozen_not_performed FROM pg_class +--WHERE oid=:reltoastrelid::regclass; +--------------------------------------------------------------------- +-- vacuum (truncate false) should not attempt to truncate off any empty pages at the end of the table (default is true) +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +vacuum (TRUNCATE false) local_vacuum_table; +SELECT pg_total_relation_size('local_vacuum_table') as size1 \gset +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +vacuum (TRUNCATE true) local_vacuum_table; +SELECT pg_total_relation_size('local_vacuum_table') as size2 \gset +SELECT :size1 > :size2 as truncate_less_size; + truncate_less_size +--------------------------------------------------------------------- + t +(1 row) + +-- vacuum (analyze) should be analyzing the table to generate statistics after vacuuming +select analyze_count from pg_stat_all_tables where relname = 'local_vacuum_table' or relname = 'reference_vacuum_table'; + analyze_count +--------------------------------------------------------------------- + 0 + 0 +(2 rows) + +vacuum (analyze) local_vacuum_table, reference_vacuum_table; +NOTICE: issuing VACUUM (ANALYZE) public.reference_vacuum_table_970000 +DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx +NOTICE: issuing VACUUM (ANALYZE) public.reference_vacuum_table_970000 +DETAIL: on server postgres@localhost:xxxxx connectionId: xxxxxxx +-- give enough time for stats to be updated.(updated per 500ms by default) +select pg_sleep(1); + pg_sleep +--------------------------------------------------------------------- + +(1 row) + +select analyze_count from pg_stat_all_tables where relname = 'local_vacuum_table' or relname = 'reference_vacuum_table'; + analyze_count +--------------------------------------------------------------------- + 1 + 1 +(2 rows) + -- should not propagate because ddl propagation is disabled SET citus.enable_ddl_propagation TO OFF; VACUUM distributed_vacuum_table; diff --git a/src/test/regress/isolation_schedule b/src/test/regress/isolation_schedule index ac60c7c49..da5549d2f 100644 --- a/src/test/regress/isolation_schedule +++ b/src/test/regress/isolation_schedule @@ -27,7 +27,7 @@ test: isolation_dml_vs_repair isolation_copy_placement_vs_copy_placement test: isolation_concurrent_dml isolation_data_migration test: isolation_drop_shards isolation_copy_placement_vs_modification -test: isolation_insert_vs_vacuum isolation_transaction_recovery +test: isolation_insert_vs_vacuum isolation_transaction_recovery isolation_vacuum_skip_locked test: isolation_progress_monitoring test: isolation_dump_local_wait_edges diff --git a/src/test/regress/spec/isolation_vacuum_skip_locked.spec b/src/test/regress/spec/isolation_vacuum_skip_locked.spec new file mode 100644 index 000000000..4f1c2d3e2 --- /dev/null +++ b/src/test/regress/spec/isolation_vacuum_skip_locked.spec @@ -0,0 +1,61 @@ +// Test for SKIP_LOCKED option of VACUUM and ANALYZE commands. +# +// This also verifies that log messages are not emitted for skipped relations +// that were not specified in the VACUUM or ANALYZE command. + +setup +{ + CREATE TABLE parted (a INT) PARTITION BY LIST (a); + CREATE TABLE part1 PARTITION OF parted FOR VALUES IN (1); + ALTER TABLE part1 SET (autovacuum_enabled = false); + CREATE TABLE part2 PARTITION OF parted FOR VALUES IN (2); + ALTER TABLE part2 SET (autovacuum_enabled = false); +} + +teardown +{ + DROP TABLE IF EXISTS parted; +} + +session s1 +step lock_share +{ + BEGIN; + LOCK part1 IN SHARE MODE; +} +step lock_access_exclusive +{ + BEGIN; + LOCK part1 IN ACCESS EXCLUSIVE MODE; +} +step commit +{ + COMMIT; +} + +session s2 +step vac_specified { VACUUM (SKIP_LOCKED) part1, part2; } +step vac_all_parts { VACUUM (SKIP_LOCKED) parted; } +step analyze_specified { ANALYZE (SKIP_LOCKED) part1, part2; } +step analyze_all_parts { ANALYZE (SKIP_LOCKED) parted; } +step vac_analyze_specified { VACUUM (ANALYZE, SKIP_LOCKED) part1, part2; } +step vac_analyze_all_parts { VACUUM (ANALYZE, SKIP_LOCKED) parted; } +step vac_full_specified { VACUUM (SKIP_LOCKED, FULL) part1, part2; } +step vac_full_all_parts { VACUUM (SKIP_LOCKED, FULL) parted; } + +permutation lock_share vac_specified commit +permutation lock_share vac_all_parts commit +permutation lock_share analyze_specified commit +permutation lock_share analyze_all_parts commit +permutation lock_share vac_analyze_specified commit +permutation lock_share vac_analyze_all_parts commit +permutation lock_share vac_full_specified commit +permutation lock_share vac_full_all_parts commit +permutation lock_access_exclusive vac_specified commit +permutation lock_access_exclusive vac_all_parts commit +permutation lock_access_exclusive analyze_specified commit +permutation lock_access_exclusive analyze_all_parts commit +permutation lock_access_exclusive vac_analyze_specified commit +permutation lock_access_exclusive vac_analyze_all_parts commit +permutation lock_access_exclusive vac_full_specified commit +permutation lock_access_exclusive vac_full_all_parts commit diff --git a/src/test/regress/sql/multi_utilities.sql b/src/test/regress/sql/multi_utilities.sql index c84796181..849a2c898 100644 --- a/src/test/regress/sql/multi_utilities.sql +++ b/src/test/regress/sql/multi_utilities.sql @@ -202,7 +202,7 @@ SET citus.next_shard_id TO 970000; SET citus.log_remote_commands TO OFF; -CREATE TABLE local_vacuum_table(id int); +CREATE TABLE local_vacuum_table(id int primary key, b text); CREATE TABLE reference_vacuum_table(id int); SELECT create_reference_table('reference_vacuum_table'); @@ -216,7 +216,14 @@ SET citus.log_remote_commands TO ON; VACUUM; -- should not propagate because no distributed table is specified +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; VACUUM local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + +-- vacuum full deallocates pages of dead tuples whereas normal vacuum only marks dead tuples on visibility map +VACUUM FULL local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); -- should propagate to all workers because table is reference table VACUUM reference_vacuum_table; @@ -230,6 +237,61 @@ VACUUM distributed_vacuum_table, local_vacuum_table, reference_vacuum_table; -- only reference_vacuum_table should propagate VACUUM local_vacuum_table, reference_vacuum_table; +-- vacuum (disable_page_skipping) aggressively process pages of the relation, it does not respect visibility map +VACUUM (DISABLE_PAGE_SKIPPING true) local_vacuum_table; +VACUUM (DISABLE_PAGE_SKIPPING false) local_vacuum_table; + +-- vacuum (index_cleanup on, parallel 1) should execute index vacuuming and index cleanup phases in parallel +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +VACUUM (INDEX_CLEANUP OFF, PARALLEL 1) local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +VACUUM (INDEX_CLEANUP ON, PARALLEL 1) local_vacuum_table; +SELECT pg_size_pretty( pg_total_relation_size('local_vacuum_table') ); + +----------------- PROCESS_TOAST is only available for pg14 +-- vacuum (process_toast false) should not be vacuuming toast tables (default is true) +--select reltoastrelid from pg_class where relname='local_vacuum_table' +--\gset + +--SELECT relfrozenxid AS frozenxid FROM pg_class WHERE oid=:reltoastrelid::regclass +--\gset +--VACUUM (FREEZE, PROCESS_TOAST true) local_vacuum_table; +--SELECT relfrozenxid::text::integer > :frozenxid AS frozen_performed FROM pg_class +--WHERE oid=:reltoastrelid::regclass; + +--SELECT relfrozenxid AS frozenxid FROM pg_class WHERE oid=:reltoastrelid::regclass +--\gset +--VACUUM (FREEZE, PROCESS_TOAST false) local_vacuum_table; +--SELECT relfrozenxid::text::integer = :frozenxid AS frozen_not_performed FROM pg_class +--WHERE oid=:reltoastrelid::regclass; +--------------- + +-- vacuum (truncate false) should not attempt to truncate off any empty pages at the end of the table (default is true) +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +vacuum (TRUNCATE false) local_vacuum_table; +SELECT pg_total_relation_size('local_vacuum_table') as size1 \gset + +insert into local_vacuum_table select i from generate_series(1,1000000) i; +delete from local_vacuum_table; +vacuum (TRUNCATE true) local_vacuum_table; +SELECT pg_total_relation_size('local_vacuum_table') as size2 \gset + +SELECT :size1 > :size2 as truncate_less_size; + +-- vacuum (analyze) should be analyzing the table to generate statistics after vacuuming +select analyze_count from pg_stat_all_tables where relname = 'local_vacuum_table' or relname = 'reference_vacuum_table'; +vacuum (analyze) local_vacuum_table, reference_vacuum_table; + +-- give enough time for stats to be updated.(updated per 500ms by default) +select pg_sleep(1); + +select analyze_count from pg_stat_all_tables where relname = 'local_vacuum_table' or relname = 'reference_vacuum_table'; + -- should not propagate because ddl propagation is disabled SET citus.enable_ddl_propagation TO OFF; VACUUM distributed_vacuum_table;