Fix race conditions during chunk (de)compression

This patch introduces a further check to compress_chunk_impl and
decompress_chunk_impl. After all locks are acquired, a check is made
to see if the chunk is still (un-)compressed. If the chunk was
(de-)compressed while waiting for the locks, the (de-)compression
operation is stopped.

In addition, the chunk locks in decompress_chunk_impl
are upgraded to AccessExclusiveLock to ensure the chunk is not deleted
while other transactions are using it.

Fixes: #4480

CHANGELOG.md

@@ -11,6 +11,9 @@ accidentally triggering the load of a previous DB version.**
 * #4393 Support intervals with day component when constifying now()
 * #4397 Support intervals with month component when constifying now()
 
+**Bugfixes**
+* #4482 Ensure (de-)compression is not performed twice on the same chunk
+
 **Thanks**
 @nikugogoi for reporting a bug with CTEs and upserts on distributed hypertables
 

src/chunk.c

@@ -4326,6 +4326,7 @@ bool
 ts_chunk_validate_chunk_status_for_operation(Oid chunk_relid, int32 chunk_status,
 											 ChunkOperation cmd, bool throw_error)
 {
+	/* Handle frozen chunks */
 	if (ts_flags_are_set_32(chunk_status, CHUNK_STATUS_FROZEN))
 	{
 		/* Data modification is not permitted on a frozen chunk */
@@ -4350,6 +4351,41 @@ ts_chunk_validate_chunk_status_for_operation(Oid chunk_relid, int32 chunk_status
 				break; /*supported operations */
 		}
 	}
+	/* Handle unfrozen chunks */
+	else
+	{
+		switch (cmd)
+		{
+			/* supported operations */
+			case CHUNK_INSERT:
+			case CHUNK_DELETE:
+			case CHUNK_UPDATE:
+				break;
+			/* Only uncompressed chunks can be compressed */
+			case CHUNK_COMPRESS:
+			{
+				if (ts_flags_are_set_32(chunk_status, CHUNK_STATUS_COMPRESSED))
+					ereport((throw_error ? ERROR : NOTICE),
+							(errcode(ERRCODE_DUPLICATE_OBJECT),
+							 errmsg("chunk \"%s\" is already compressed",
+									get_rel_name(chunk_relid))));
+				return false;
+			}
+			/* Only compressed chunks can be decompressed */
+			case CHUNK_DECOMPRESS:
+			{
+				if (!ts_flags_are_set_32(chunk_status, CHUNK_STATUS_COMPRESSED))
+					ereport((throw_error ? ERROR : NOTICE),
+							(errcode(ERRCODE_DUPLICATE_OBJECT),
+							 errmsg("chunk \"%s\" is already decompressed",
+									get_rel_name(chunk_relid))));
+				return false;
+			}
+			default:
+				break;
+		}
+	}
+
 	return true;
 }
 

test/runner_isolation.sh

@@ -0,0 +1,16 @@
+#!/usr/bin/env bash
+#
+# Wrapper for the PostgreSQL isolationtest runner. It replaces
+# the chunks IDs in the output of the tests by _X_X_. So, even
+# if the IDs change, the tests will not fail.
+##############################################################
+
+set -e
+set -u
+
+ISOLATIONTEST=$1
+shift
+
+$ISOLATIONTEST "$@" | \
+   sed -e 's!_[0-9]\{1,\}_[0-9]\{1,\}_chunk!_X_X_chunk!g'
+ 

test/test-defs.cmake

@@ -78,7 +78,8 @@ set(PG_REGRESS_SHARED_OPTS_EXTRA
 
 set(PG_ISOLATION_REGRESS_OPTS_EXTRA
     --create-role=${TEST_ROLE_SUPERUSER},${TEST_ROLE_DEFAULT_PERM_USER},${TEST_ROLE_DEFAULT_PERM_USER_2},${TEST_ROLE_CLUSTER_SUPERUSER},${TEST_ROLE_1},${TEST_ROLE_2},${TEST_ROLE_3},${TEST_ROLE_READ_ONLY}
-    --dbname=${TEST_DBNAME})
+    --dbname=${TEST_DBNAME}
+    --launcher=${PRIMARY_TEST_DIR}/runner_isolation.sh)
 
 set(PG_REGRESS_OPTS_INOUT --inputdir=${TEST_INPUT_DIR}
                           --outputdir=${TEST_OUTPUT_DIR})

tsl/src/compression/api.c

@@ -28,6 +28,7 @@
 #include "compat/compat.h"
 #include "cache.h"
 #include "chunk.h"
+#include "debug_point.h"
 #include "errors.h"
 #include "error_utils.h"
 #include "hypertable.h"
@@ -236,11 +237,27 @@ compress_chunk_impl(Oid hypertable_relid, Oid chunk_relid)
 	/* Perform an analyze on the chunk to get up-to-date stats before compressing */
 	preserve_uncompressed_chunk_stats(chunk_relid);
 
-	/* aquire locks on catalog tables to keep till end of txn */
+	/* acquire locks on catalog tables to keep till end of txn */
 	LockRelationOid(catalog_get_table_id(ts_catalog_get(), HYPERTABLE_COMPRESSION),
 					AccessShareLock);
 	LockRelationOid(catalog_get_table_id(ts_catalog_get(), CHUNK), RowExclusiveLock);
 
+	DEBUG_WAITPOINT("compress_chunk_impl_start");
+
+	/*
+	 * Re-read the state of the chunk after all locks have been acquired and ensure
+	 * it is still uncompressed. Another process running in parallel might have
+	 * already performed the compression while we were waiting for the locks to be
+	 * acquired.
+	 */
+	Chunk *chunk_state_after_lock = ts_chunk_get_by_relid(chunk_relid, true);
+
+	/* Throw error if chunk has invalid status for operation */
+	ts_chunk_validate_chunk_status_for_operation(chunk_state_after_lock->table_id,
+												 chunk_state_after_lock->fd.status,
+												 CHUNK_COMPRESS,
+												 true);
+
 	/* get compression properties for hypertable */
 	htcols_list = ts_hypertable_compression_get(cxt.srcht->fd.id);
 	htcols_listlen = list_length(htcols_list);
@@ -334,19 +351,59 @@ decompress_chunk_impl(Oid uncompressed_hypertable_relid, Oid uncompressed_chunk_
 	/* acquire locks on src and compress hypertable and src chunk */
 	LockRelationOid(uncompressed_hypertable->main_table_relid, AccessShareLock);
 	LockRelationOid(compressed_hypertable->main_table_relid, AccessShareLock);
-	LockRelationOid(uncompressed_chunk->table_id, AccessShareLock); /*upgrade when needed */
 
-	/* aquire locks on catalog tables to keep till end of txn */
+	/*
+	 * Acquire an ExclusiveLock on the uncompressed and the compressed
+	 * chunk (the chunks can still be accessed by reads).
+	 *
+	 * The lock on the compressed chunk is needed because it gets deleted
+	 * after decompression. The lock on the uncompressed chunk is needed
+	 * to avoid deadlocks (e.g., caused by later lock upgrades or parallel
+	 * started chunk compressions).
+	 *
+	 * Note: Also the function decompress_chunk() will request an
+	 *       ExclusiveLock on the compressed and on the uncompressed
+	 *       chunk. See the comments in function about the concurrency of
+	 *       operations.
+	 */
+	LockRelationOid(uncompressed_chunk->table_id, ExclusiveLock);
+	LockRelationOid(compressed_chunk->table_id, ExclusiveLock);
+
+	/* acquire locks on catalog tables to keep till end of txn */
 	LockRelationOid(catalog_get_table_id(ts_catalog_get(), HYPERTABLE_COMPRESSION),
 					AccessShareLock);
+
 	LockRelationOid(catalog_get_table_id(ts_catalog_get(), CHUNK), RowExclusiveLock);
 
+	DEBUG_WAITPOINT("decompress_chunk_impl_start");
+
+	/*
+	 * Re-read the state of the chunk after all locks have been acquired and ensure
+	 * it is still compressed. Another process running in parallel might have
+	 * already performed the decompression while we were waiting for the locks to be
+	 * acquired.
+	 */
+	Chunk *chunk_state_after_lock = ts_chunk_get_by_relid(uncompressed_chunk_relid, true);
+
+	/* Throw error if chunk has invalid status for operation */
+	ts_chunk_validate_chunk_status_for_operation(chunk_state_after_lock->table_id,
+												 chunk_state_after_lock->fd.status,
+												 CHUNK_DECOMPRESS,
+												 true);
+
 	decompress_chunk(compressed_chunk->table_id, uncompressed_chunk->table_id);
+
 	/* Recreate FK constraints, since they were dropped during compression. */
 	ts_chunk_create_fks(uncompressed_chunk);
+
+	/* Prevent readers from using the compressed chunk that is going to be deleted */
+	LockRelationOid(uncompressed_chunk->table_id, AccessExclusiveLock);
+
+	/* Delete the compressed chunk */
 	ts_compression_chunk_size_delete(uncompressed_chunk->fd.id);
 	ts_chunk_clear_compressed_chunk(uncompressed_chunk);
 	ts_chunk_drop(compressed_chunk, DROP_RESTRICT, -1);
+
 	/* reenable autovacuum if necessary */
 	restore_autovacuum_on_decompress(uncompressed_hypertable_relid, uncompressed_chunk_relid);
 

tsl/test/isolation/expected/compression_chunk_race.out

@@ -0,0 +1,131 @@
+Parsed test spec with 3 sessions
+
+starting permutation: s3_lock_compression s3_lock_decompression s1_compress s2_compress s3_unlock_compression s1_decompress s2_decompress s3_unlock_decompression
+step s3_lock_compression: 
+    SELECT debug_waitpoint_enable('compress_chunk_impl_start');
+
+debug_waitpoint_enable
+----------------------
+                      
+(1 row)
+
+step s3_lock_decompression: 
+    SELECT debug_waitpoint_enable('decompress_chunk_impl_start');
+
+debug_waitpoint_enable
+----------------------
+                      
+(1 row)
+
+step s1_compress: 
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM (SELECT compress_chunk(i, if_not_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+ <waiting ...>
+step s2_compress: 
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM (SELECT compress_chunk(i, if_not_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+   RESET client_min_messages;
+ <waiting ...>
+step s3_unlock_compression: 
+    -- Ensure that we are waiting on our debug waitpoint and one chunk
+    -- Note: The OIDs of the advisory locks are based on the hash value of the lock name (see debug_point_init())
+    --       compress_chunk_impl_start = 3379597659.
+    -- 'SELECT relation::regclass, ....' can not be used, because it returns a field with a variable length
+    SELECT locktype, mode, granted, objid FROM pg_locks WHERE not granted AND (locktype = 'advisory' or relation::regclass::text LIKE '%chunk') ORDER BY relation, locktype, mode, granted;
+    SELECT debug_waitpoint_release('compress_chunk_impl_start');
+
+locktype|mode     |granted|     objid
+--------+---------+-------+----------
+relation|ShareLock|f      |          
+advisory|ShareLock|f      |3379597659
+(2 rows)
+
+debug_waitpoint_release
+-----------------------
+                       
+(1 row)
+
+step s1_compress: <... completed>
+compression_status
+------------------
+Uncompressed      
+Uncompressed      
+(2 rows)
+
+count
+-----
+    2
+(1 row)
+
+compression_status
+------------------
+Compressed        
+Compressed        
+(2 rows)
+
+  count
+-------
+1008050
+(1 row)
+
+step s2_compress: <... completed>
+compression_status
+------------------
+Uncompressed      
+Uncompressed      
+(2 rows)
+
+ERROR:  chunk "_hyper_X_X_chunk" is already compressed
+step s1_decompress: 
+   SELECT count(*) FROM (SELECT decompress_chunk(i, if_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+ <waiting ...>
+step s2_decompress: 
+   SELECT count(*) FROM (SELECT decompress_chunk(i, if_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+   RESET client_min_messages;
+ <waiting ...>
+step s3_unlock_decompression: 
+    -- Ensure that we are waiting on our debug waitpoint and one chunk
+    -- Note: The OIDs of the advisory locks are based on the hash value of the lock name (see debug_point_init())
+    --       decompress_chunk_impl_start = 2415054640.
+    -- 'SELECT relation::regclass, ....' can not be used, because it returns a field with a variable length
+    SELECT locktype, mode, granted, objid FROM pg_locks WHERE not granted AND (locktype = 'advisory' or relation::regclass::text LIKE '%chunk') ORDER BY relation, locktype, mode, granted;
+    SELECT debug_waitpoint_release('decompress_chunk_impl_start');
+
+locktype|mode         |granted|     objid
+--------+-------------+-------+----------
+relation|ExclusiveLock|f      |          
+advisory|ShareLock    |f      |2415054640
+(2 rows)
+
+debug_waitpoint_release
+-----------------------
+                       
+(1 row)
+
+step s1_decompress: <... completed>
+count
+-----
+    2
+(1 row)
+
+compression_status
+------------------
+Uncompressed      
+Uncompressed      
+(2 rows)
+
+  count
+-------
+1008050
+(1 row)
+
+step s2_decompress: <... completed>
+ERROR:  chunk "_hyper_X_X_chunk" is already decompressed

tsl/test/isolation/expected/deadlock_recompress_chunk.out

@@ -1,9 +1,9 @@
 Parsed test spec with 3 sessions
 
 starting permutation: recompress_insert_rows lock_after_decompress recompress_chunks_start query_start unlock_after_decompress
-compress_chunk                            
-------------------------------------------
-_timescaledb_internal._hyper_141_300_chunk
+count
+-----
+    1
 (1 row)
 
 step recompress_insert_rows: 

tsl/test/isolation/specs/CMakeLists.txt

@@ -3,9 +3,13 @@ set(TEST_TEMPLATES_MODULE reorder_deadlock.spec.in
                           reorder_vs_insert_other_chunk.spec.in)
 
 set(TEST_TEMPLATES_MODULE_DEBUG
-    reorder_vs_insert.spec.in reorder_vs_select.spec.in
-    remote_create_chunk.spec.in dist_restore_point.spec.in
-    cagg_drop_chunks.spec.in telemetry.spec.in)
+    reorder_vs_insert.spec.in
+    reorder_vs_select.spec.in
+    remote_create_chunk.spec.in
+    dist_restore_point.spec.in
+    cagg_drop_chunks.spec.in
+    telemetry.spec.in
+    compression_chunk_race.spec.in)
 
 # These tests are using markers for the isolation tests (to avoid race
 # conditions causing differing output), which were added after 12.7, 13.3, and

tsl/test/isolation/specs/compression_chunk_race.spec.in

@@ -0,0 +1,112 @@
+# This file and its contents are licensed under the Timescale License.
+# Please see the included NOTICE for copyright information and
+# LICENSE-TIMESCALE for a copy of the license.
+
+###
+# Test the execution of two compression jobs in parallel
+###
+
+setup {
+   CREATE OR REPLACE FUNCTION debug_waitpoint_enable(TEXT) RETURNS VOID LANGUAGE C VOLATILE STRICT
+     AS '@TS_MODULE_PATHNAME@', 'ts_debug_point_enable';
+
+   CREATE OR REPLACE FUNCTION debug_waitpoint_release(TEXT) RETURNS VOID LANGUAGE C VOLATILE STRICT
+     AS '@TS_MODULE_PATHNAME@', 'ts_debug_point_release';
+
+   CREATE TABLE sensor_data (
+   time timestamptz not null,
+   sensor_id integer not null,
+   cpu double precision null,
+   temperature double precision null);
+
+   -- Create large chunks that take a long time to compress
+   SELECT FROM create_hypertable('sensor_data','time', chunk_time_interval => INTERVAL '14 days');
+
+   INSERT INTO sensor_data
+   SELECT
+   time + (INTERVAL '1 minute' * random()) AS time,
+   sensor_id,
+   random() AS cpu,
+   random()* 100 AS temperature
+   FROM
+   generate_series('2022-01-01', '2022-01-15', INTERVAL '1 minute') AS g1(time),
+   generate_series(1, 50, 1) AS g2(sensor_id)
+   ORDER BY time;
+
+   SELECT count(*) FROM sensor_data;
+   
+   ALTER TABLE sensor_data SET (timescaledb.compress, timescaledb.compress_segmentby = 'sensor_id, cpu');
+}
+
+teardown {
+   DROP TABLE sensor_data;
+}
+
+
+session "s1"
+setup {
+   SET client_min_messages=ERROR; -- Suppress chunk "_hyper_XXX_chunk" is not compressed messages 
+}
+
+step "s1_compress" {
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM (SELECT compress_chunk(i, if_not_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+}
+
+step "s1_decompress" {
+   SELECT count(*) FROM (SELECT decompress_chunk(i, if_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+}
+
+session "s2"
+setup {
+   SET client_min_messages=ERROR; -- Suppress chunk "_hyper_XXX_chunk" is not compressed messages 
+}
+
+step "s2_compress" {
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM (SELECT compress_chunk(i, if_not_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+   RESET client_min_messages;
+}
+
+step "s2_decompress" {
+   SELECT count(*) FROM (SELECT decompress_chunk(i, if_compressed => true) FROM show_chunks('sensor_data') i) i;
+   SELECT compression_status FROM chunk_compression_stats('sensor_data');
+   SELECT count(*) FROM sensor_data;
+   RESET client_min_messages;
+}
+
+session "s3"
+step "s3_lock_compression" {
+    SELECT debug_waitpoint_enable('compress_chunk_impl_start');
+}
+
+step "s3_lock_decompression" {
+    SELECT debug_waitpoint_enable('decompress_chunk_impl_start');
+}
+
+step "s3_unlock_compression" {
+    -- Ensure that we are waiting on our debug waitpoint and one chunk
+    -- Note: The OIDs of the advisory locks are based on the hash value of the lock name (see debug_point_init())
+    --       compress_chunk_impl_start = 3379597659.
+    -- 'SELECT relation::regclass, ....' can not be used, because it returns a field with a variable length
+    SELECT locktype, mode, granted, objid FROM pg_locks WHERE not granted AND (locktype = 'advisory' or relation::regclass::text LIKE '%chunk') ORDER BY relation, locktype, mode, granted;
+    SELECT debug_waitpoint_release('compress_chunk_impl_start');
+}
+
+step "s3_unlock_decompression" {
+    -- Ensure that we are waiting on our debug waitpoint and one chunk
+    -- Note: The OIDs of the advisory locks are based on the hash value of the lock name (see debug_point_init())
+    --       decompress_chunk_impl_start = 2415054640.
+    -- 'SELECT relation::regclass, ....' can not be used, because it returns a field with a variable length
+    SELECT locktype, mode, granted, objid FROM pg_locks WHERE not granted AND (locktype = 'advisory' or relation::regclass::text LIKE '%chunk') ORDER BY relation, locktype, mode, granted;
+    SELECT debug_waitpoint_release('decompress_chunk_impl_start');
+}
+
+permutation "s3_lock_compression" "s3_lock_decompression" "s1_compress" "s2_compress" (s1_compress) "s3_unlock_compression" "s1_decompress" "s2_decompress" (s1_decompress) "s3_unlock_decompression" 
+

tsl/test/isolation/specs/deadlock_recompress_chunk.spec.in

@@ -28,7 +28,7 @@ setup {
 
     ALTER TABLE hyper SET (timescaledb.compress = true);
 
-    SELECT compress_chunk(show_chunks('hyper'));
+    SELECT count(*) FROM (SELECT compress_chunk(show_chunks('hyper'))) i;
 }
 
 teardown {