This is an automated email from the ASF dual-hosted git repository. yjhjstz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/cloudberry.git
commit 96d06cafad8f6ef573a705d21941370618685fba Author: Huansong Fu <[email protected]> AuthorDate: Thu Sep 8 15:01:36 2022 -0700 Resolve a FIXME in merge_leaf_stats() A FIXME was left noting whether to use AccessShareLock or NoLock when opening child tables to merge their stats. Since we have acquired ShareUpdateExclusiveLock on the parent table when we are at merge_leaf_stats(), we don't need extra lock to guard against concurrent DROP of either the parent or the child (which requries AccessExclusiveLock on the parent). Concurrent UPDATE is possible but because we are not updating the table ourselves, NoLock is sufficient here. --- src/backend/commands/analyze.c | 14 +++-- src/test/isolation2/expected/lockmodes.out | 61 ++++++++++++++++++++++ .../isolation2/expected/lockmodes_optimizer.out | 61 ++++++++++++++++++++++ src/test/isolation2/sql/lockmodes.sql | 28 ++++++++++ 4 files changed, 160 insertions(+), 4 deletions(-) diff --git a/src/backend/commands/analyze.c b/src/backend/commands/analyze.c index bdf8740c28..a8ac8c75b3 100644 --- a/src/backend/commands/analyze.c +++ b/src/backend/commands/analyze.c @@ -4414,11 +4414,17 @@ merge_leaf_stats(VacAttrStatsP stats, (errmsg("Merging leaf partition stats to calculate root partition stats : column %s", get_attname(stats->attr->attrelid, stats->attr->attnum, false)))); - /* GPDB_12_MERGE_FIXME: what's the appropriate lock level? AccessShareLock - * is enough to scan the table, but are we updating them, too? If not, - * NoLock might be enough? + /* + * Since we have acquired ShareUpdateExclusiveLock on the parent table when + * ANALYZE'ing it, we don't need extra lock to guard against concurrent DROP + * of either the parent or the child (which requries AccessExclusiveLock on + * the parent). + * Concurrent UPDATE is possible but because we are not updating the table + * ourselves, NoLock is sufficient here. */ - all_children_list = find_all_inheritors(stats->attr->attrelid, AccessShareLock, NULL); + all_children_list = find_all_inheritors(stats->attr->attrelid, NoLock, NULL); + SIMPLE_FAULT_INJECTOR("merge_leaf_stats_after_find_children"); + oid_list = NIL; foreach (lc, all_children_list) { diff --git a/src/test/isolation2/expected/lockmodes.out b/src/test/isolation2/expected/lockmodes.out index 5318955776..df566ff9cd 100644 --- a/src/test/isolation2/expected/lockmodes.out +++ b/src/test/isolation2/expected/lockmodes.out @@ -2200,3 +2200,64 @@ ALTER 1: drop table lockmodes_datadir; DROP 1q: ... <quitting> +2q: ... <quitting> + +-- Check that concurrent DROP on leaf partition won't impact analyze on the +-- parent since analyze will hold a ShareUpdateExclusiveLock and DROP will +-- require an AccessExclusiveLock. +-- Case 1. The analyze result is expected when there's concurrent drop on child. +1:create table analyzedrop(a int) partition by range(a); +CREATE +1:create table analyzedrop_1 partition of analyzedrop for values from (0) to (10); +CREATE +1:create table analyzedrop_2 partition of analyzedrop for values from (10) to (20); +CREATE +1:insert into analyzedrop select * from generate_series(0,19); +INSERT 20 +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1&: analyze analyzedrop; <waiting ...> +2&: drop table analyzedrop_1; <waiting ...> +3:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1<: <... completed> +ANALYZE +2<: <... completed> +DROP +3:select * from pg_stats where tablename like 'analyzedrop%'; + schemaname | tablename | attname | inherited | null_frac | avg_width | n_distinct | most_common_vals | most_common_freqs | histogram_bounds | correlation | most_common_elems | most_common_elem_freqs | elem_count_histogram +------------+---------------+---------+-----------+-----------+-----------+------------+------------------+-------------------+--------------------------------------------------+-------------+-------------------+------------------------+---------------------- + public | analyzedrop | a | t | 0 | 4 | -1 | | | {0,1,2,3,4,5,6,7,8,9,11,12,13,14,15,16,17,18,19} | | | | + public | analyzedrop_2 | a | f | 0 | 4 | -1 | | | {10,11,12,13,14,15,16,17,18,19} | 1 | | | +(2 rows) +-- Case 2. No failure should happen when there's concurrent drop on parent as well. +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1&: analyze analyzedrop; <waiting ...> +2&: drop table analyzedrop_2; <waiting ...> +3&: drop table analyzedrop; <waiting ...> +4:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1<: <... completed> +ANALYZE +2<: <... completed> +DROP +3<: <... completed> +DROP +--empty as table is dropped +4:select * from pg_stats where tablename like 'analyzedrop%'; + schemaname | tablename | attname | inherited | null_frac | avg_width | n_distinct | most_common_vals | most_common_freqs | histogram_bounds | correlation | most_common_elems | most_common_elem_freqs | elem_count_histogram +------------+-----------+---------+-----------+-----------+-----------+------------+------------------+-------------------+------------------+-------------+-------------------+------------------------+---------------------- +(0 rows) diff --git a/src/test/isolation2/expected/lockmodes_optimizer.out b/src/test/isolation2/expected/lockmodes_optimizer.out index f98192946d..ed247a8a1b 100644 --- a/src/test/isolation2/expected/lockmodes_optimizer.out +++ b/src/test/isolation2/expected/lockmodes_optimizer.out @@ -2207,3 +2207,64 @@ ALTER 1: drop table lockmodes_datadir; DROP 1q: ... <quitting> +2q: ... <quitting> + +-- Check that concurrent DROP on leaf partition won't impact analyze on the +-- parent since analyze will hold a ShareUpdateExclusiveLock and DROP will +-- require an AccessExclusiveLock. +-- Case 1. The analyze result is expected when there's concurrent drop on child. +1:create table analyzedrop(a int) partition by range(a); +CREATE +1:create table analyzedrop_1 partition of analyzedrop for values from (0) to (10); +CREATE +1:create table analyzedrop_2 partition of analyzedrop for values from (10) to (20); +CREATE +1:insert into analyzedrop select * from generate_series(0,19); +INSERT 20 +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1&: analyze analyzedrop; <waiting ...> +2&: drop table analyzedrop_1; <waiting ...> +3:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1<: <... completed> +ANALYZE +2<: <... completed> +DROP +3:select * from pg_stats where tablename like 'analyzedrop%'; + schemaname | tablename | attname | inherited | null_frac | avg_width | n_distinct | most_common_vals | most_common_freqs | histogram_bounds | correlation | most_common_elems | most_common_elem_freqs | elem_count_histogram +------------+---------------+---------+-----------+-----------+-----------+------------+------------------+-------------------+--------------------------------------------------+-------------+-------------------+------------------------+---------------------- + public | analyzedrop | a | t | 0 | 4 | -1 | | | {0,1,2,3,4,5,6,7,8,9,11,12,13,14,15,16,17,18,19} | | | | + public | analyzedrop_2 | a | f | 0 | 4 | -1 | | | {10,11,12,13,14,15,16,17,18,19} | 1 | | | +(2 rows) +-- Case 2. No failure should happen when there's concurrent drop on parent as well. +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1&: analyze analyzedrop; <waiting ...> +2&: drop table analyzedrop_2; <waiting ...> +3&: drop table analyzedrop; <waiting ...> +4:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; + gp_inject_fault_infinite +-------------------------- + Success: +(1 row) +1<: <... completed> +ANALYZE +2<: <... completed> +DROP +3<: <... completed> +DROP +--empty as table is dropped +4:select * from pg_stats where tablename like 'analyzedrop%'; + schemaname | tablename | attname | inherited | null_frac | avg_width | n_distinct | most_common_vals | most_common_freqs | histogram_bounds | correlation | most_common_elems | most_common_elem_freqs | elem_count_histogram +------------+-----------+---------+-----------+-----------+-----------+------------+------------------+-------------------+------------------+-------------+-------------------+------------------------+---------------------- +(0 rows) diff --git a/src/test/isolation2/sql/lockmodes.sql b/src/test/isolation2/sql/lockmodes.sql index 28541f8ba8..a42f9c1171 100644 --- a/src/test/isolation2/sql/lockmodes.sql +++ b/src/test/isolation2/sql/lockmodes.sql @@ -712,3 +712,31 @@ ALTER SYSTEM SET gp_enable_global_deadlock_detector TO on; 1: drop table lockmodes_datadir; 1q: +2q: + +-- Check that concurrent DROP on leaf partition won't impact analyze on the +-- parent since analyze will hold a ShareUpdateExclusiveLock and DROP will +-- require an AccessExclusiveLock. +-- Case 1. The analyze result is expected when there's concurrent drop on child. +1:create table analyzedrop(a int) partition by range(a); +1:create table analyzedrop_1 partition of analyzedrop for values from (0) to (10); +1:create table analyzedrop_2 partition of analyzedrop for values from (10) to (20); +1:insert into analyzedrop select * from generate_series(0,19); +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; +1&: analyze analyzedrop; +2&: drop table analyzedrop_1; +3:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; +1<: +2<: +3:select * from pg_stats where tablename like 'analyzedrop%'; +-- Case 2. No failure should happen when there's concurrent drop on parent as well. +1:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'suspend', dbid) from gp_segment_configuration where content = -1 and role = 'p'; +1&: analyze analyzedrop; +2&: drop table analyzedrop_2; +3&: drop table analyzedrop; +4:select gp_inject_fault_infinite('merge_leaf_stats_after_find_children', 'reset', dbid) from gp_segment_configuration where content = -1 and role = 'p'; +1<: +2<: +3<: +--empty as table is dropped +4:select * from pg_stats where tablename like 'analyzedrop%'; --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
