Fix error in show_chunks

If "created_after/before" is used with a "time" type partitioning column then show_chunks was not showing appropriate list due to a mismatch in the comparison of the "creation_time" metadata (which is stored as a timestamptz) with the internally converted epoch based input argument value. This is now fixed by not doing the unnecessary conversion into the internal format for cases where it's not needed. Fixes timescale#6611
fabriziomello · Mar 7, 2024 · 6b819de · 6b819de
1 parent a8666d0
commit 6b819de
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 21 deletions.
diff --git a/.unreleased/fix_6617 b/.unreleased/fix_6617
@@ -0,0 +1,2 @@
+Fixes: #6617 Fix error in show_chunks
+Thanks: @kevcenteno for reporting an issue with the show_chunks API showing incorrect output when 'created_before/created_after' was used with time-partitioned columns.
diff --git a/src/chunk.c b/src/chunk.c
@@ -2081,7 +2081,8 @@ ts_chunk_show_chunks(PG_FUNCTION_ARGS)
 
 		/*
 		 * We cannot have a mix of [older_than/newer_than] and [created_before/created_after].
-		 * So, check that first.
+		 * So, check that first. Note that created_before/created_after have a type of
+		 * TIMESTAMPTZOID regardless of the partitioning type.
 		 */
 
 		if (!PG_ARGISNULL(3))
@@ -2094,7 +2095,11 @@ ts_chunk_show_chunks(PG_FUNCTION_ARGS)
 								"or \"created_after\"")));
 
 			arg_type = get_fn_expr_argtype(fcinfo->flinfo, 3);
-			created_before = ts_time_value_from_arg(PG_GETARG_DATUM(3), arg_type, time_type, false);
+			/* We use the existing function for various type/conversion checks */
+			created_before =
+				ts_time_value_from_arg(PG_GETARG_DATUM(3), arg_type, TIMESTAMPTZOID, false);
+			/* convert into int64 format for comparisons */
+			created_before = ts_internal_to_time_int64(created_before, TIMESTAMPTZOID);
 			before_after = true;
 		}
 
@@ -2108,7 +2113,11 @@ ts_chunk_show_chunks(PG_FUNCTION_ARGS)
 								"or \"created_after\"")));
 
 			arg_type = get_fn_expr_argtype(fcinfo->flinfo, 4);
-			created_after = ts_time_value_from_arg(PG_GETARG_DATUM(4), arg_type, time_type, false);
+			/* We use the existing function for various type/conversion checks */
+			created_after =
+				ts_time_value_from_arg(PG_GETARG_DATUM(4), arg_type, TIMESTAMPTZOID, false);
+			/* convert into int64 format for comparisons */
+			created_after = ts_internal_to_time_int64(created_after, TIMESTAMPTZOID);
 			before_after = true;
 		}
 
@@ -4152,7 +4161,8 @@ ts_chunk_drop_chunks(PG_FUNCTION_ARGS)
 
 	/*
 	 * We cannot have a mix of [older_than/newer_than] and [created_before/created_after].
-	 * So, check that first.
+	 * So, check that first. Note that created_before/created_after have a type of
+	 * TIMESTAMPTZOID regardless of the partitioning type.
 	 */
 
 	if (!PG_ARGISNULL(4))
@@ -4170,7 +4180,11 @@ ts_chunk_drop_chunks(PG_FUNCTION_ARGS)
 							 " partitioning.")));
 
 		arg_type = get_fn_expr_argtype(fcinfo->flinfo, 4);
-		created_before = ts_time_value_from_arg(PG_GETARG_DATUM(4), arg_type, time_type, false);
+		/* We use the existing function for various type/conversion checks */
+		created_before =
+			ts_time_value_from_arg(PG_GETARG_DATUM(4), arg_type, TIMESTAMPTZOID, false);
+		/* convert into int64 format for comparisons */
+		created_before = ts_internal_to_time_int64(created_before, TIMESTAMPTZOID);
 		before_after = true;
 		older_than = created_before;
 	}
@@ -4189,7 +4203,10 @@ ts_chunk_drop_chunks(PG_FUNCTION_ARGS)
 							 "with \"integer\"-like"
 							 " partitioning.")));
 		arg_type = get_fn_expr_argtype(fcinfo->flinfo, 5);
-		created_after = ts_time_value_from_arg(PG_GETARG_DATUM(5), arg_type, time_type, false);
+		/* We use the existing function for various type/conversion checks */
+		created_after = ts_time_value_from_arg(PG_GETARG_DATUM(5), arg_type, TIMESTAMPTZOID, false);
+		/* convert into int64 format for comparisons */
+		created_after = ts_internal_to_time_int64(created_after, TIMESTAMPTZOID);
 		before_after = true;
 		newer_than = created_after;
 	}

diff --git a/test/expected/chunk_utils.out b/test/expected/chunk_utils.out
@@ -862,14 +862,6 @@ SELECT * FROM test.show_subtables('drop_chunk_test_tstz');
  _timescaledb_internal._hyper_5_22_chunk | 
 (2 rows)
 
--- "created_before/after" can be used with time partitioning in show chunks
-SELECT show_chunks('drop_chunk_test_tstz', created_before => now() + INTERVAL '1 hour');
-               show_chunks               
------------------------------------------
- _timescaledb_internal._hyper_5_21_chunk
- _timescaledb_internal._hyper_5_22_chunk
-(2 rows)
-
 BEGIN;
     SELECT show_chunks('drop_chunk_test_ts');
                show_chunks               
@@ -1528,11 +1520,39 @@ ORDER BY chunk_name ;
  _timescaledb_internal | _hyper_12_41_chunk
 (1 row)
 
--- "created_before/after" can be used with time partitioning in drop chunks
+-- "created_before/after" can be used with time partitioning in drop/show chunks
+SELECT show_chunks('drop_chunk_test_tstz', created_before => now() - INTERVAL '1 hour');
+ show_chunks 
+-------------
+(0 rows)
+
 SELECT drop_chunks('drop_chunk_test_tstz', created_before => now() + INTERVAL '1 hour');
                drop_chunks               
 -----------------------------------------
  _timescaledb_internal._hyper_5_21_chunk
  _timescaledb_internal._hyper_5_22_chunk
 (2 rows)
 
+SELECT show_chunks('drop_chunk_test_ts');
+               show_chunks               
+-----------------------------------------
+ _timescaledb_internal._hyper_4_19_chunk
+ _timescaledb_internal._hyper_4_20_chunk
+(2 rows)
+
+-- "created_before/after" accept timestamptz even though partitioning col is just
+-- timestamp
+SELECT show_chunks('drop_chunk_test_ts', created_after => now() - INTERVAL '1 hour', created_before => now());
+               show_chunks               
+-----------------------------------------
+ _timescaledb_internal._hyper_4_19_chunk
+ _timescaledb_internal._hyper_4_20_chunk
+(2 rows)
+
+SELECT drop_chunks('drop_chunk_test_ts', created_after => INTERVAL '1 hour', created_before => now());
+               drop_chunks               
+-----------------------------------------
+ _timescaledb_internal._hyper_4_19_chunk
+ _timescaledb_internal._hyper_4_20_chunk
+(2 rows)
+
diff --git a/test/sql/chunk_utils.sql b/test/sql/chunk_utils.sql
@@ -308,8 +308,6 @@ INSERT INTO PUBLIC.drop_chunk_test_tstz VALUES(now()+INTERVAL '5 minutes', 1.0,
 
 SELECT * FROM test.show_subtables('drop_chunk_test_ts');
 SELECT * FROM test.show_subtables('drop_chunk_test_tstz');
--- "created_before/after" can be used with time partitioning in show chunks
-SELECT show_chunks('drop_chunk_test_tstz', created_before => now() + INTERVAL '1 hour');
 
 BEGIN;
     SELECT show_chunks('drop_chunk_test_ts');
@@ -649,5 +647,11 @@ FROM timescaledb_information.chunks
 WHERE hypertable_name = 'hyper1' and hypertable_schema = 'test1'
 ORDER BY chunk_name ;
 
--- "created_before/after" can be used with time partitioning in drop chunks
+-- "created_before/after" can be used with time partitioning in drop/show chunks
+SELECT show_chunks('drop_chunk_test_tstz', created_before => now() - INTERVAL '1 hour');
 SELECT drop_chunks('drop_chunk_test_tstz', created_before => now() + INTERVAL '1 hour');
+SELECT show_chunks('drop_chunk_test_ts');
+-- "created_before/after" accept timestamptz even though partitioning col is just
+-- timestamp
+SELECT show_chunks('drop_chunk_test_ts', created_after => now() - INTERVAL '1 hour', created_before => now());
+SELECT drop_chunks('drop_chunk_test_ts', created_after => INTERVAL '1 hour', created_before => now());
diff --git a/tsl/test/expected/policy_generalization.out b/tsl/test/expected/policy_generalization.out
@@ -144,13 +144,28 @@ SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_s
 (1 row)
 
 -- Compression policy
-SELECT add_compression_policy('test', compress_created_before => INTERVAL '2 seconds') AS compress_chunks_job_id \gset
+SELECT add_compression_policy('test', compress_created_before => INTERVAL '1 hour') AS compress_chunks_job_id \gset
 SELECT pg_sleep(3);
  pg_sleep 
 ----------
 
 (1 row)
 
+CALL run_job(:compress_chunks_job_id);
+-- Chunk compression status
+SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_stats;
+ compression_status 
+--------------------
+ Uncompressed
+(1 row)
+
+SELECT remove_compression_policy('test');
+ remove_compression_policy 
+---------------------------
+ t
+(1 row)
+
+SELECT add_compression_policy('test', compress_created_before => INTERVAL '2 seconds') AS compress_chunks_job_id \gset
 CALL run_job(:compress_chunks_job_id);
 -- Chunk compression status
 SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_stats;

diff --git a/tsl/test/sql/policy_generalization.sql b/tsl/test/sql/policy_generalization.sql
@@ -62,12 +62,17 @@ INSERT INTO test SELECT i, i %10, 0.10 FROM generate_series(1, 100, 1) i;
 SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_stats;
 
 -- Compression policy
-SELECT add_compression_policy('test', compress_created_before => INTERVAL '2 seconds') AS compress_chunks_job_id \gset
+SELECT add_compression_policy('test', compress_created_before => INTERVAL '1 hour') AS compress_chunks_job_id \gset
 SELECT pg_sleep(3);
 CALL run_job(:compress_chunks_job_id);
-
 -- Chunk compression status
 SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_stats;
+SELECT remove_compression_policy('test');
+SELECT add_compression_policy('test', compress_created_before => INTERVAL '2 seconds') AS compress_chunks_job_id \gset
+CALL run_job(:compress_chunks_job_id);
+-- Chunk compression status
+SELECT DISTINCT compression_status FROM _timescaledb_internal.compressed_chunk_stats;
+
 -- check for WARNING/NOTICE if policy already exists
 SELECT add_compression_policy('test', compress_created_before => INTERVAL '2 seconds',
     if_not_exists => true);
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Fixes: #6617 Fix error in show_chunks
		Thanks: @kevcenteno for reporting an issue with the show_chunks API showing incorrect output when 'created_before/created_after' was used with time-partitioned columns.