Skip to content

Commit

Permalink
Remove useless table lock
Browse files Browse the repository at this point in the history
In ae21ee9 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.
  • Loading branch information
fabriziomello committed Nov 15, 2023
1 parent 49121ae commit 0254abd
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 8 deletions.
3 changes: 0 additions & 3 deletions sql/size_utils.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions src/utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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);

Expand Down
29 changes: 28 additions & 1 deletion test/isolation/expected/concurrent_query_and_drop_chunks.out
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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'); <waiting ...>
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)

24 changes: 20 additions & 4 deletions test/isolation/specs/concurrent_query_and_drop_chunks.spec
Original file line number Diff line number Diff line change
Expand Up @@ -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'); }
Expand All @@ -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"

0 comments on commit 0254abd

Please sign in to comment.