Allow only integer intervals for custom time types

Fix a check for a compatible chunk time interval type when creating a
hypertable with a custom time type.

Previously, the check allowed `Interval` type intervals for any
dimension type that is not an integer type, including custom time
types. The check is now changed so that it only accepts an `Interval`
for timestamp and date type dimensions.

A number of related error messages are also cleaned up so that they
are more consistent and conform to the error style guide.

src/dimension.c

@@ -1001,24 +1001,28 @@ dimension_interval_to_internal(const char *colname, Oid dimtype, Oid valuetype,
 			interval = get_validated_integer_interval(dimtype, DatumGetInt64(value));
 			break;
 		case INTERVALOID:
-			if (IS_INTEGER_TYPE(dimtype))
+			if (!IS_TIMESTAMP_TYPE(dimtype))
 				ereport(ERROR,
 						(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-						 errmsg(
+						 errmsg("invalid interval type for %s dimension", format_type_be(dimtype)),
-							 "invalid interval: must be an integer type for integer dimensions")));
+						 errhint("Use an interval of type integer.")));
 
 			interval = interval_to_usec(DatumGetIntervalP(value));
 			break;
 		default:
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-					 errmsg("invalid interval: must be an interval or integer type")));
+					 errmsg("invalid interval type for %s dimension", format_type_be(dimtype)),
+					 IS_TIMESTAMP_TYPE(dimtype) ?
+						 errhint("Use an interval of type integer or interval.") :
+						 errhint("Use an interval of type integer.")));
 	}
 
 	if (dimtype == DATEOID && (interval <= 0 || interval % USECS_PER_DAY != 0))
 		ereport(ERROR,
 				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-				 errmsg("invalid interval: must be multiples of one day")));
+				 errmsg("invalid interval for %s dimension", format_type_be(dimtype)),
+				 errhint("Use an interval that is a multiple of one day.")));
 
 	return interval;
 }

src/dimension.h

@@ -14,7 +14,7 @@
 
 #include "catalog.h"
 #include "export.h"
-#include "utils.h"
+#include "time_utils.h"
 
 typedef struct PartitioningInfo PartitioningInfo;
 typedef struct DimensionSlice DimensionSlice;
@@ -37,13 +37,7 @@ typedef struct Dimension
 } Dimension;
 
 #define IS_OPEN_DIMENSION(d) ((d)->type == DIMENSION_TYPE_OPEN)
-
 #define IS_CLOSED_DIMENSION(d) ((d)->type == DIMENSION_TYPE_CLOSED)
-
-#define IS_INTEGER_TYPE(type) (type == INT2OID || type == INT4OID || type == INT8OID)
-
-#define IS_TIMESTAMP_TYPE(type) (type == TIMESTAMPOID || type == TIMESTAMPTZOID || type == DATEOID)
-
 #define IS_VALID_OPEN_DIM_TYPE(type)                                                               \
 	(IS_INTEGER_TYPE(type) || IS_TIMESTAMP_TYPE(type) || ts_type_is_int8_binary_compatible(type))
 

src/time_utils.h

@@ -40,38 +40,33 @@
 #define TS_TIME_NOBEGIN (PG_INT64_MIN)
 #define TS_TIME_NOEND (PG_INT64_MAX)
 
-#define TS_TIME_IS_INTEGER_TIME(type) (type == INT2OID || type == INT4OID || type == INT8OID)
+#define IS_INTEGER_TYPE(type) (type == INT2OID || type == INT4OID || type == INT8OID)
-#define TS_TIME_IS_DATE_TIMESTAMP_TIME(type)                                                       \
+#define IS_TIMESTAMP_TYPE(type) (type == TIMESTAMPOID || type == TIMESTAMPTZOID || type == DATEOID)
-	(type == TIMESTAMPTZOID || type == TIMESTAMPOID || type == DATEOID)
+#define IS_VALID_TIME_TYPE(type) (IS_INTEGER_TYPE(type) || IS_TIMESTAMP_TYPE(type))
-
-#define TS_TIME_IS_VALID_TYPE(type)                                                                \
-	(TS_TIME_IS_INTEGER_TIME(type) || TS_TIME_IS_DATE_TIMESTAMP_TIME(type))
 
 #define TS_TIME_DATUM_IS_MIN(timeval, type) (timeval == ts_time_datum_get_min(type))
 #define TS_TIME_DATUM_IS_MAX(timeval, type) (timeval == ts_time_datum_get_max(type))
 #define TS_TIME_DATUM_IS_END(timeval, type)                                                        \
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && timeval == ts_time_datum_get_end(type)))
+	(IS_TIMESTAMP_TYPE(type) && timeval == ts_time_datum_get_end(type)))
 #define TS_TIME_DATUM_IS_NOBEGIN(timeval, type)                                                    \
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && (timeval == ts_time_datum_get_nobegin(type)))
+	(IS_TIMESTAMP_TYPE(type) && (timeval == ts_time_datum_get_nobegin(type)))
 #define TS_TIME_DATUM_IS_NOEND(timeval, type)                                                      \
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && (timeval == ts_time_datum_get_noend(type)))
+	(IS_TIMESTAMP_TYPE(type) && (timeval == ts_time_datum_get_noend(type)))
 
 #define TS_TIME_DATUM_NOT_FINITE(timeval, type)                                                    \
-	(TS_TIME_IS_INTEGER_TIME(type) || TS_TIME_DATUM_IS_NOBEGIN(timeval, type) ||                   \
+	(IS_INTEGER_TYPE(type) || TS_TIME_DATUM_IS_NOBEGIN(timeval, type) ||                           \
 	 TS_TIME_DATUM_IS_NOEND(timeval, type))
 
 #define TS_TIME_IS_MIN(timeval, type) (timeval == ts_time_get_min(type))
 #define TS_TIME_IS_MAX(timeval, type) (timeval == ts_time_get_max(type))
-#define TS_TIME_IS_END(timeval, type)                                                              \
+#define TS_TIME_IS_END(timeval, type) (IS_TIMESTAMP_TYPE(type) && timeval == ts_time_get_end(type))
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && timeval == ts_time_get_end(type))
 #define TS_TIME_IS_NOBEGIN(timeval, type)                                                          \
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && timeval == ts_time_get_nobegin(type))
+	(IS_TIMESTAMP_TYPE(type) && timeval == ts_time_get_nobegin(type))
 #define TS_TIME_IS_NOEND(timeval, type)                                                            \
-	(TS_TIME_IS_DATE_TIMESTAMP_TIME(type) && timeval == ts_time_get_noend(type))
+	(IS_TIMESTAMP_TYPE(type) && timeval == ts_time_get_noend(type))
 
 #define TS_TIME_NOT_FINITE(timeval, type)                                                          \
-	(TS_TIME_IS_INTEGER_TIME(type) || TS_TIME_IS_NOBEGIN(timeval, type) ||                         \
+	(IS_INTEGER_TYPE(type) || TS_TIME_IS_NOBEGIN(timeval, type) || TS_TIME_IS_NOEND(timeval, type))
-	 TS_TIME_IS_NOEND(timeval, type))
 
 extern TSDLLEXPORT int64 ts_time_value_from_arg(Datum arg, Oid argtype, Oid timetype);
 extern TSDLLEXPORT Datum ts_time_datum_convert_arg(Datum arg, Oid *argtype, Oid timetype);

src/utils.c

@@ -121,7 +121,7 @@ ts_time_value_to_internal(Datum time_val, Oid type_oid)
 
 	/* Handle custom time types. We currently only support binary coercible
 	 * types */
-	if (!TS_TIME_IS_VALID_TYPE(type_oid))
+	if (!IS_VALID_TIME_TYPE(type_oid))
 	{
 		if (ts_type_is_int8_binary_compatible(type_oid))
 			return DatumGetInt64(time_val);
@@ -129,7 +129,7 @@ ts_time_value_to_internal(Datum time_val, Oid type_oid)
 		elog(ERROR, "unknown time type OID %d", type_oid);
 	}
 
-	if (TS_TIME_IS_INTEGER_TIME(type_oid))
+	if (IS_INTEGER_TYPE(type_oid))
 	{
 		/* Integer time types have no distinction between min, max and
 		 * infinity. We don't want min and max to be turned into infinity for

test/expected/create_chunks.out

@@ -210,10 +210,10 @@ select set_chunk_time_interval(NULL,NULL::interval);
 ERROR:  invalid main_table: cannot be NULL
 -- should fail since time column is an int
 SELECT set_chunk_time_interval('chunk_test', INTERVAL '1 minute');
-ERROR:  invalid interval: must be an integer type for integer dimensions
+ERROR:  invalid interval type for integer dimension
 -- should fail since its not a valid way to represent time
 SELECT set_chunk_time_interval('chunk_test', 'foo'::TEXT);
-ERROR:  invalid interval: must be an interval or integer type
+ERROR:  invalid interval type for integer dimension
 SELECT set_chunk_time_interval('chunk_test', NULL::BIGINT);
 ERROR:  invalid interval: an explicit interval must be specified
 SELECT set_chunk_time_interval('chunk_test2', NULL::BIGINT);

test/expected/create_hypertable.out

@@ -184,10 +184,10 @@ NOTICE:  adding not-null constraint to column "time2"
 -- Test add_dimension: only integral should work on BIGINT columns
 \set ON_ERROR_STOP 0
 SELECT add_dimension('dim_test_time', 'time3', chunk_time_interval => INTERVAL '1 day');
-ERROR:  invalid interval: must be an integer type for integer dimensions
+ERROR:  invalid interval type for bigint dimension
 -- string is not a valid type
 SELECT add_dimension('dim_test_time', 'time3', chunk_time_interval => 'foo'::TEXT);
-ERROR:  invalid interval: must be an interval or integer type
+ERROR:  invalid interval type for bigint dimension
 \set ON_ERROR_STOP 1
 SELECT add_dimension('dim_test_time', 'time3', chunk_time_interval => 500);
 NOTICE:  adding not-null constraint to column "time3"

test/expected/custom_type-11.out

@@ -59,6 +59,11 @@ CREATE OPERATOR >= (
 );
 \c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER
 CREATE TABLE customtype_test(time_custom customtype, val int);
+\set ON_ERROR_STOP 0
+-- Using interval type for chunk time interval should fail with custom time type
+SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => INTERVAL '1 day', create_default_indexes=>false);
+ERROR:  invalid interval type for customtype dimension
+\set ON_ERROR_STOP 1
 SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => 10e6::bigint, create_default_indexes=>false);
 NOTICE:  adding not-null constraint to column "time_custom"
       create_hypertable       

test/expected/custom_type-12.out

@@ -59,6 +59,11 @@ CREATE OPERATOR >= (
 );
 \c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER
 CREATE TABLE customtype_test(time_custom customtype, val int);
+\set ON_ERROR_STOP 0
+-- Using interval type for chunk time interval should fail with custom time type
+SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => INTERVAL '1 day', create_default_indexes=>false);
+ERROR:  invalid interval type for customtype dimension
+\set ON_ERROR_STOP 1
 SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => 10e6::bigint, create_default_indexes=>false);
 NOTICE:  adding not-null constraint to column "time_custom"
       create_hypertable       

test/expected/timestamp.out

@@ -1274,5 +1274,5 @@ ERROR:  invalid interval: must be between 1 and 32767
 SELECT test.interval_to_internal('TEXT'::regtype, 32768::bigint);
 ERROR:  invalid dimension type: "testcol" must be an integer, date or timestamp
 SELECT test.interval_to_internal('INT'::regtype, INTERVAL '1 day');
-ERROR:  invalid interval: must be an integer type for integer dimensions
+ERROR:  invalid interval type for integer dimension
 \set ON_ERROR_STOP 1

test/sql/custom_type.sql.in

@@ -65,6 +65,11 @@ CREATE OPERATOR >= (
 \c :TEST_DBNAME :ROLE_DEFAULT_PERM_USER
 
 CREATE TABLE customtype_test(time_custom customtype, val int);
+\set ON_ERROR_STOP 0
+-- Using interval type for chunk time interval should fail with custom time type
+SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => INTERVAL '1 day', create_default_indexes=>false);
+\set ON_ERROR_STOP 1
+
 SELECT create_hypertable('customtype_test', 'time_custom', chunk_time_interval => 10e6::bigint, create_default_indexes=>false);
 
 INSERT INTO customtype_test VALUES ('2001-01-01 01:02:03'::customtype, 10);

tsl/src/continuous_aggs/invalidation_threshold.c

@@ -281,11 +281,11 @@ invalidation_threshold_compute(const ContinuousAgg *cagg, const InternalTimeRang
 	bool max_refresh = false;
 	Hypertable *ht = ts_hypertable_get_by_id(cagg->data.raw_hypertable_id);
 
-	if (TS_TIME_IS_INTEGER_TIME(refresh_window->type))
+	if (IS_TIMESTAMP_TYPE(refresh_window->type))
-		max_refresh = TS_TIME_IS_MAX(refresh_window->end, refresh_window->type);
-	else
 		max_refresh = TS_TIME_IS_END(refresh_window->end, refresh_window->type) ||
 					  TS_TIME_IS_NOEND(refresh_window->end, refresh_window->type);
+	else
+		max_refresh = TS_TIME_IS_MAX(refresh_window->end, refresh_window->type);
 
 	if (max_refresh)
 	{