From 0254abd9c2ff87f396c0ccc240ea09469114a96b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabr=C3=ADzio=20de=20Royes=20Mello?= Date: Tue, 14 Nov 2023 17:55:02 -0300 Subject: [PATCH] Remove useless table lock In ae21ee96 we fixed a race condition when running a query to get the hypertable sizes and one or more chunks was dropped in a concurrent session leading to exception because the chunks does not exist. In fact the table lock introduced is useless because we also added proper joins with Postgres catalog tables to ensure that the relation exists in the database when calculating the sizes. And even worse with this table lock now dropping chunks wait for the functions that calculate the hypertable sizes. Fixed it by removing the useless table lock and also added isolation tests to make sure we'll not end up with race conditions again. --- sql/size_utils.sql | 3 -- src/utils.c | 2 ++ .../concurrent_query_and_drop_chunks.out | 29 ++++++++++++++++++- .../concurrent_query_and_drop_chunks.spec | 24 ++++++++++++--- 4 files changed, 50 insertions(+), 8 deletions(-) diff --git a/sql/size_utils.sql b/sql/size_utils.sql index d2a3c22f6ad..19645534094 100644 --- a/sql/size_utils.sql +++ b/sql/size_utils.sql @@ -212,9 +212,6 @@ BEGIN END IF; END IF; - /* Lock hypertable to avoid risk of concurrent process dropping chunks */ - EXECUTE format('LOCK TABLE %I.%I IN ACCESS SHARE MODE', schema_name, table_name); - CASE WHEN is_distributed THEN RETURN QUERY SELECT *, NULL::name diff --git a/src/utils.c b/src/utils.c index a005c086d4d..def5f25030a 100644 --- a/src/utils.c +++ b/src/utils.c @@ -37,6 +37,7 @@ #include "compat/compat.h" #include "chunk.h" +#include "debug_point.h" #include "guc.h" #include "hypertable_cache.h" #include "utils.h" @@ -995,6 +996,7 @@ ts_relation_size_impl(Oid relid) Datum reloid = ObjectIdGetDatum(relid); Relation rel; + DEBUG_WAITPOINT("relation_size_before_lock"); /* Open relation earlier to keep a lock during all function calls */ rel = try_relation_open(relid, AccessShareLock); diff --git a/test/isolation/expected/concurrent_query_and_drop_chunks.out b/test/isolation/expected/concurrent_query_and_drop_chunks.out index aa0d48539df..318105c0c41 100644 --- a/test/isolation/expected/concurrent_query_and_drop_chunks.out +++ b/test/isolation/expected/concurrent_query_and_drop_chunks.out @@ -1,4 +1,4 @@ -Parsed test spec with 2 sessions +Parsed test spec with 4 sessions starting permutation: s2_query s1_wp_enable s2_query s1_drop_chunks s1_wp_release s2_show_num_chunks step s2_query: SELECT * FROM measurements ORDER BY 1; @@ -39,3 +39,30 @@ count 1 (1 row) + +starting permutation: s3_wp_enable s4_hypertable_size s3_drop_chunks s3_wp_release +step s3_wp_enable: SELECT debug_waitpoint_enable('relation_size_before_lock'); +debug_waitpoint_enable +---------------------- + +(1 row) + +step s4_hypertable_size: SELECT count(*) FROM hypertable_size('measurements'); +step s3_drop_chunks: SELECT count(*) FROM drop_chunks('measurements', TIMESTAMPTZ '2020-03-01'); +count +----- + 1 +(1 row) + +step s3_wp_release: SELECT debug_waitpoint_release('relation_size_before_lock'); +debug_waitpoint_release +----------------------- + +(1 row) + +step s4_hypertable_size: <... completed> +count +----- + 1 +(1 row) + diff --git a/test/isolation/specs/concurrent_query_and_drop_chunks.spec b/test/isolation/specs/concurrent_query_and_drop_chunks.spec index 96769feae5e..558ee807f10 100644 --- a/test/isolation/specs/concurrent_query_and_drop_chunks.spec +++ b/test/isolation/specs/concurrent_query_and_drop_chunks.spec @@ -13,10 +13,9 @@ teardown { DROP TABLE measurements; } -# Test concurrent querying and drop chunks. The wait point happens -# after chunks have been found for table expansion, but before the -# chunks are locked. Because one chunk will dropped before the lock is -# acqurired, the chunk should also be ignored. +# +# Test concurrent querying and drop chunks. +# session "s1" step "s1_wp_enable" { SELECT debug_waitpoint_enable('hypertable_expansion_before_lock_chunk'); } @@ -27,4 +26,21 @@ session "s2" step "s2_show_num_chunks" { SELECT count(*) FROM show_chunks('measurements') ORDER BY 1; } step "s2_query" { SELECT * FROM measurements ORDER BY 1; } +session "s3" +step "s3_wp_enable" { SELECT debug_waitpoint_enable('relation_size_before_lock'); } +step "s3_wp_release" { SELECT debug_waitpoint_release('relation_size_before_lock'); } +step "s3_drop_chunks" { SELECT count(*) FROM drop_chunks('measurements', TIMESTAMPTZ '2020-03-01'); } + +session "s4" +step "s4_hypertable_size" { SELECT count(*) FROM hypertable_size('measurements'); } + +# The wait point happens after chunks have been found for table +# expansion, but before the chunks are locked. Because one chunk +# will dropped before the lock is acqurired, the chunk should +# also be ignored. permutation "s2_query" "s1_wp_enable" "s2_query" "s1_drop_chunks" "s1_wp_release" "s2_show_num_chunks" + +# The wait point happens before the relation_size get the lock +# for the relation and one chunk will be dropped in another session +# don't leading to race conditions +permutation "s3_wp_enable" "s4_hypertable_size" "s3_drop_chunks" "s3_wp_release"