From ca0968aaa073b1fb51ff2714f988a871f754d1d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Nordstr=C3=B6m?= Date: Fri, 17 Nov 2017 15:27:38 +0100 Subject: [PATCH] Make all partitioning functions take anyelement argument All partitioning functions now has the signature `int func(anyelement)`. This cleans up some special handling that was necessary to support the legacy partitioning function that expected text input. --- sql/partitioning.sql | 2 +- sql/updates/post-0.6.1--0.7.0-dev.sql | 15 ++++ sql/util_internal_table_ddl.sql | 27 ++----- src/parse_rewrite.c | 28 +------ src/partitioning.c | 101 ++++++++++++++------------ src/process_utility.c | 4 +- test/expected/drop_chunks.out | 4 +- test/expected/hash.out | 49 +++++++++++++ test/expected/partitioning.out | 12 +-- test/sql/drop_chunks.sql | 4 +- test/sql/hash.sql | 10 +++ test/sql/util.sql | 6 +- 12 files changed, 152 insertions(+), 110 deletions(-) diff --git a/sql/partitioning.sql b/sql/partitioning.sql index 0430b9499..3f403542e 100644 --- a/sql/partitioning.sql +++ b/sql/partitioning.sql @@ -1,5 +1,5 @@ -- Deprecated partition hash function -CREATE OR REPLACE FUNCTION _timescaledb_internal.get_partition_for_key(val text) +CREATE OR REPLACE FUNCTION _timescaledb_internal.get_partition_for_key(val anyelement) RETURNS int AS '$libdir/timescaledb', 'get_partition_for_key' LANGUAGE C IMMUTABLE STRICT; diff --git a/sql/updates/post-0.6.1--0.7.0-dev.sql b/sql/updates/post-0.6.1--0.7.0-dev.sql index 9395afc3d..ca256e8c4 100644 --- a/sql/updates/post-0.6.1--0.7.0-dev.sql +++ b/sql/updates/post-0.6.1--0.7.0-dev.sql @@ -5,6 +5,9 @@ DECLARE chunk_constraint_row _timescaledb_catalog.chunk_constraint; chunk_row _timescaledb_catalog.chunk; BEGIN + -- Need to do this update in two loops: first remove constraints, then add back. + -- This is because we can only remove the old partitioning function when + -- there are no constraints on the tables referencing the old function FOR chunk_constraint_row IN SELECT cc.* FROM _timescaledb_catalog.chunk_constraint cc @@ -15,7 +18,19 @@ BEGIN SELECT * INTO STRICT chunk_row FROM _timescaledb_catalog.chunk c WHERE c.id = chunk_constraint_row.chunk_id; EXECUTE format('ALTER TABLE %I.%I DROP CONSTRAINT %I', chunk_row.schema_name, chunk_row.table_name, chunk_constraint_row.constraint_name); + END LOOP; + DROP FUNCTION _timescaledb_internal.get_partition_for_key(text); + + FOR chunk_constraint_row IN + SELECT cc.* + FROM _timescaledb_catalog.chunk_constraint cc + INNER JOIN _timescaledb_catalog.dimension_slice ds ON (cc.dimension_slice_id = ds.id) + INNER JOIN _timescaledb_catalog.dimension d ON (ds.dimension_id = d.id) + WHERE d.partitioning_func IS NOT NULL + LOOP + SELECT * INTO STRICT chunk_row FROM _timescaledb_catalog.chunk c WHERE c.id = chunk_constraint_row.chunk_id; PERFORM _timescaledb_internal.chunk_constraint_add_table_constraint(chunk_constraint_row); END LOOP; + END$$; diff --git a/sql/util_internal_table_ddl.sql b/sql/util_internal_table_ddl.sql index 42d834a04..f2d8d7f51 100644 --- a/sql/util_internal_table_ddl.sql +++ b/sql/util_internal_table_ddl.sql @@ -92,9 +92,7 @@ $BODY$ DECLARE dimension_slice_row _timescaledb_catalog.dimension_slice; dimension_row _timescaledb_catalog.dimension; - proargtype OID; - typecast TEXT = ''; - parts TEXT[]; + parts TEXT[]; BEGIN SELECT * INTO STRICT dimension_slice_row FROM _timescaledb_catalog.dimension_slice @@ -105,39 +103,26 @@ BEGIN WHERE id = dimension_slice_row.dimension_id; IF dimension_row.partitioning_func IS NOT NULL THEN - SELECT proargtypes[0] INTO STRICT proargtype - FROM pg_proc p, pg_namespace n - WHERE n.nspname = dimension_row.partitioning_func_schema - AND p.proname = dimension_row.partitioning_func - AND n.oid = p.pronamespace; - - -- Check if we are using a legacy partitioning function - -- that only takes text input - IF proargtype = 'text'::regtype THEN - typecast := '::text'; - END IF; IF _timescaledb_internal.dimension_is_finite(dimension_slice_row.range_start) THEN parts = parts || format( $$ - %1$I.%2$I(%3$I%4$s) >= %5$L + %1$I.%2$I(%3$I) >= %4$L $$, - dimension_row.partitioning_func_schema, + dimension_row.partitioning_func_schema, dimension_row.partitioning_func, - dimension_row.column_name, - typecast, + dimension_row.column_name, dimension_slice_row.range_start); END IF; IF _timescaledb_internal.dimension_is_finite(dimension_slice_row.range_end) THEN parts = parts || format( $$ - %1$I.%2$I(%3$I%4$s) < %5$L + %1$I.%2$I(%3$I) < %4$L $$, - dimension_row.partitioning_func_schema, + dimension_row.partitioning_func_schema, dimension_row.partitioning_func, dimension_row.column_name, - typecast, dimension_slice_row.range_end); END IF; diff --git a/src/parse_rewrite.c b/src/parse_rewrite.c index 265f952b3..89df53100 100644 --- a/src/parse_rewrite.c +++ b/src/parse_rewrite.c @@ -68,32 +68,8 @@ create_partition_func_equals_const(ParseState *pstate, PartitioningInfo *pi, Var Node *f_var; Node *f_const; - if (pi->partfunc.paramtype == TEXTOID) - { - /* Path for deprecated partitioning function taking text input */ - if (var_expr->vartype == TEXTOID) - { - var_node = (Node *) copyObject(var_expr); - const_node = (Node *) copyObject(const_expr); - } - else - { - var_node = coerce_to_target_type(pstate, (Node *) var_expr, - var_expr->vartype, - TEXTOID, -1, COERCION_EXPLICIT, - COERCE_EXPLICIT_CAST, -1); - const_node = coerce_to_target_type(pstate, (Node *) const_expr, - const_expr->consttype, - TEXTOID, -1, COERCION_EXPLICIT, - COERCE_EXPLICIT_CAST, -1); - } - } - else - { - /* Path for partitioning func taking anyelement */ - var_node = (Node *) copyObject(var_expr); - const_node = (Node *) copyObject(const_expr); - } + var_node = (Node *) copyObject(var_expr); + const_node = (Node *) copyObject(const_expr); args_func_var = list_make1(var_node); args_func_const = list_make1(const_node); diff --git a/src/partitioning.c b/src/partitioning.c index e2e497892..5263657f6 100644 --- a/src/partitioning.c +++ b/src/partitioning.c @@ -32,15 +32,15 @@ partitioning_func_set_func_fmgr(PartitioningFunc *pf) FuncnameGetCandidates(partitioning_func_qualified_name(pf), 1, NULL, false, false, false); - if (funclist == NULL || funclist->next) - elog(ERROR, "Could not resolve the partitioning function"); + while (funclist != NULL && + (funclist->nargs != 1 || funclist->args[0] != ANYELEMENTOID)) + funclist = funclist->next; + + if (NULL == funclist) + elog(ERROR, "Partitioning function not found"); pf->paramtype = funclist->args[0]; - if (!(funclist->nargs == 1 && - (pf->paramtype == TEXTOID || pf->paramtype == ANYELEMENTOID))) - elog(ERROR, "Invalid partitioning function signature"); - fmgr_info_cxt(funclist->oid, &pf->func_fmgr, CurrentMemoryContext); } @@ -132,25 +132,12 @@ partitioning_info_create(const char *schema, /* * Apply the partitioning function of a hypertable to a value. * - * We support both partitioning functions with the legacy signature int - * func(text) and the new signature int func(anyelement). + * We support both partitioning functions with the signature int + * func(anyelement). */ int32 partitioning_func_apply(PartitioningInfo *pinfo, Datum value) { - if (pinfo->partfunc.paramtype == TEXTOID) - { - /* Legacy function signature. We need to convert the datum to text. */ - Oid funcid = find_text_coercion_func(pinfo->typcache_entry->type_id); - - if (!OidIsValid(funcid)) - elog(ERROR, "Could not coerce type %u to text", - pinfo->typcache_entry->type_id); - - value = OidFunctionCall1(funcid, value); - value = CStringGetTextDatum(DatumGetCString(value)); - } - return DatumGetInt32(FunctionCall1(&pinfo->partfunc.func_fmgr, value)); } @@ -168,32 +155,6 @@ partitioning_func_apply_tuple(PartitioningInfo *pinfo, HeapTuple tuple, TupleDes return partitioning_func_apply(pinfo, value); } -/* _timescaledb_catalog.get_partition_for_key(key TEXT) RETURNS INT */ -PGDLLEXPORT Datum get_partition_for_key(PG_FUNCTION_ARGS); - -PG_FUNCTION_INFO_V1(get_partition_for_key); - -/* - * Deprecated function to calculate partition hash values. - * - * This function assumes text input only. - */ -Datum -get_partition_for_key(PG_FUNCTION_ARGS) -{ - struct varlena *data = PG_GETARG_VARLENA_PP(0); - uint32 hash_u; - int32 res; - - hash_u = DatumGetUInt32(hash_any((unsigned char *) VARDATA_ANY(data), - VARSIZE_ANY_EXHDR(data))); - - res = (int32) (hash_u & 0x7fffffff); /* Only positive numbers */ - - PG_FREE_IF_COPY(data, 0); - PG_RETURN_INT32(res); -} - /* * Resolve the type of the argument passed to a function. * @@ -235,6 +196,52 @@ resolve_function_argtype(FunctionCallInfo fcinfo) return argtype; } +/* _timescaledb_catalog.get_partition_for_key(key anyelement) RETURNS INT */ +PGDLLEXPORT Datum get_partition_for_key(PG_FUNCTION_ARGS); + +PG_FUNCTION_INFO_V1(get_partition_for_key); + +/* + * Partition hash function that first converts all inputs to text before + * hashing. + */ +Datum +get_partition_for_key(PG_FUNCTION_ARGS) +{ + Datum arg = PG_GETARG_DATUM(0); + struct varlena *data; + Oid argtype; + uint32 hash_u; + int32 res; + + if (PG_NARGS() != 1) + elog(ERROR, "Unexpected number of arguments to partitioning function"); + + argtype = resolve_function_argtype(fcinfo); + + if (argtype != TEXTOID) + { + /* Not TEXT input -> need to convert to text */ + Oid funcid = find_text_coercion_func(argtype); + + if (!OidIsValid(funcid)) + elog(ERROR, "Could not coerce type %u to text", + argtype); + + arg = OidFunctionCall1(funcid, arg); + arg = CStringGetTextDatum(DatumGetCString(arg)); + } + + data = DatumGetTextPP(arg); + hash_u = DatumGetUInt32(hash_any((unsigned char *) VARDATA_ANY(data), + VARSIZE_ANY_EXHDR(data))); + + res = (int32) (hash_u & 0x7fffffff); /* Only positive numbers */ + + PG_FREE_IF_COPY(data, 0); + PG_RETURN_INT32(res); +} + PGDLLEXPORT Datum get_partition_hash(PG_FUNCTION_ARGS); PG_FUNCTION_INFO_V1(get_partition_hash); diff --git a/src/process_utility.c b/src/process_utility.c index 5aa25672d..f152b3f39 100644 --- a/src/process_utility.c +++ b/src/process_utility.c @@ -113,11 +113,11 @@ typedef struct ProcessUtilityArgs ParamListInfo params; DestReceiver *dest; char *completion_tag; -} ProcessUtilityArgs; +} ProcessUtilityArgs; /* Call the default ProcessUtility and handle PostgreSQL version differences */ static void -prev_ProcessUtility(ProcessUtilityArgs * args) +prev_ProcessUtility(ProcessUtilityArgs *args) { if (prev_ProcessUtility_hook != NULL) { diff --git a/test/expected/drop_chunks.out b/test/expected/drop_chunks.out index b1128c967..ce1a845fc 100644 --- a/test/expected/drop_chunks.out +++ b/test/expected/drop_chunks.out @@ -40,13 +40,13 @@ WHERE h.schema_name = 'public' AND (h.table_name = 'drop_chunk_test1' OR h.table --------+------+------+------- (0 rows) -SELECT _timescaledb_internal.get_partition_for_key('dev1'); +SELECT _timescaledb_internal.get_partition_for_key('dev1'::text); get_partition_for_key ----------------------- 1129986420 (1 row) -SELECT _timescaledb_internal.get_partition_for_key('dev7'); +SELECT _timescaledb_internal.get_partition_for_key('dev7'::varchar(5)); get_partition_for_key ----------------------- 449729092 diff --git a/test/expected/hash.out b/test/expected/hash.out index 1a05ae833..b0a57bea0 100644 --- a/test/expected/hash.out +++ b/test/expected/hash.out @@ -250,3 +250,52 @@ SELECT _timescaledb_internal.get_partition_hash(id::text) FROM hash_test; 1516350201 (1 row) +-- Test legacy function that converts values to text first +SELECT _timescaledb_internal.get_partition_for_key('4b6a5eec-b344-11e7-abc4-cec278b6b50a'::text); + get_partition_for_key +----------------------- + 934882099 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key('4b6a5eec-b344-11e7-abc4-cec278b6b50a'::varchar); + get_partition_for_key +----------------------- + 934882099 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key(187); + get_partition_for_key +----------------------- + 1161071810 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key(187::bigint); + get_partition_for_key +----------------------- + 1161071810 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key(187::numeric); + get_partition_for_key +----------------------- + 1161071810 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key(187::double precision); + get_partition_for_key +----------------------- + 1161071810 +(1 row) + +SELECT _timescaledb_internal.get_partition_for_key(int4range(10, 20)); + get_partition_for_key +----------------------- + 505239042 +(1 row) + +SELECT _timescaledb_internal.get_partition_hash('08002b:010203'::macaddr); + get_partition_hash +-------------------- + 294987870 +(1 row) + diff --git a/test/expected/partitioning.out b/test/expected/partitioning.out index 5f79e75e4..768d5f065 100644 --- a/test/expected/partitioning.out +++ b/test/expected/partitioning.out @@ -22,23 +22,23 @@ SELECT * FROM test.show_constraintsp('_timescaledb_internal._hyper_1_%_chunk'); Table | Constraint | Type | Columns | Index | Expr ----------------------------------------+--------------+------+----------+-------+------------------------------------------------------------------------------------------------------------------------------------------------ _timescaledb_internal._hyper_1_1_chunk | constraint_1 | c | {time} | - | (("time" >= 'Wed Feb 22 16:00:00 2017 PST'::timestamp with time zone) AND ("time" < 'Fri Mar 24 17:00:00 2017 PDT'::timestamp with time zone)) - _timescaledb_internal._hyper_1_1_chunk | constraint_2 | c | {device} | - | (_timescaledb_internal.get_partition_for_key((device)::text) >= 1073741823) + _timescaledb_internal._hyper_1_1_chunk | constraint_2 | c | {device} | - | (_timescaledb_internal.get_partition_for_key(device) >= 1073741823) _timescaledb_internal._hyper_1_2_chunk | constraint_1 | c | {time} | - | (("time" >= 'Wed Feb 22 16:00:00 2017 PST'::timestamp with time zone) AND ("time" < 'Fri Mar 24 17:00:00 2017 PDT'::timestamp with time zone)) - _timescaledb_internal._hyper_1_2_chunk | constraint_3 | c | {device} | - | (_timescaledb_internal.get_partition_for_key((device)::text) < 1073741823) + _timescaledb_internal._hyper_1_2_chunk | constraint_3 | c | {device} | - | (_timescaledb_internal.get_partition_for_key(device) < 1073741823) (4 rows) -- Make sure constraint exclusion works on device column EXPLAIN (verbose, costs off) SELECT * FROM part_legacy WHERE device = 1; - QUERY PLAN -------------------------------------------------------------------------------------------------------------------------------------------------- + QUERY PLAN +----------------------------------------------------------------------------------------------------------------------------------------- Append -> Seq Scan on public.part_legacy Output: part_legacy."time", part_legacy.temp, part_legacy.device - Filter: ((part_legacy.device = 1) AND (_timescaledb_internal.get_partition_for_key((part_legacy.device)::text) = 1516350201)) + Filter: ((part_legacy.device = 1) AND (_timescaledb_internal.get_partition_for_key(part_legacy.device) = 1516350201)) -> Seq Scan on _timescaledb_internal._hyper_1_1_chunk Output: _hyper_1_1_chunk."time", _hyper_1_1_chunk.temp, _hyper_1_1_chunk.device - Filter: ((_hyper_1_1_chunk.device = 1) AND (_timescaledb_internal.get_partition_for_key((_hyper_1_1_chunk.device)::text) = 1516350201)) + Filter: ((_hyper_1_1_chunk.device = 1) AND (_timescaledb_internal.get_partition_for_key(_hyper_1_1_chunk.device) = 1516350201)) (7 rows) CREATE TABLE part_new(time timestamptz, temp float, device int); diff --git a/test/sql/drop_chunks.sql b/test/sql/drop_chunks.sql index c02ec4bad..488812ccf 100644 --- a/test/sql/drop_chunks.sql +++ b/test/sql/drop_chunks.sql @@ -16,8 +16,8 @@ WHERE h.schema_name = 'public' AND (h.table_name = 'drop_chunk_test1' OR h.table \dt "_timescaledb_internal".* -SELECT _timescaledb_internal.get_partition_for_key('dev1'); -SELECT _timescaledb_internal.get_partition_for_key('dev7'); +SELECT _timescaledb_internal.get_partition_for_key('dev1'::text); +SELECT _timescaledb_internal.get_partition_for_key('dev7'::varchar(5)); INSERT INTO PUBLIC.drop_chunk_test1 VALUES(1, 1.0, 'dev1'); INSERT INTO PUBLIC.drop_chunk_test1 VALUES(2, 2.0, 'dev1'); diff --git a/test/sql/hash.sql b/test/sql/hash.sql index 22729b50b..8a7e24f12 100644 --- a/test/sql/hash.sql +++ b/test/sql/hash.sql @@ -62,3 +62,13 @@ SELECT _timescaledb_internal.get_partition_hash(id) FROM hash_test; SELECT _timescaledb_internal.get_partition_hash(value) FROM hash_test; -- Test coerced value SELECT _timescaledb_internal.get_partition_hash(id::text) FROM hash_test; + +-- Test legacy function that converts values to text first +SELECT _timescaledb_internal.get_partition_for_key('4b6a5eec-b344-11e7-abc4-cec278b6b50a'::text); +SELECT _timescaledb_internal.get_partition_for_key('4b6a5eec-b344-11e7-abc4-cec278b6b50a'::varchar); +SELECT _timescaledb_internal.get_partition_for_key(187); +SELECT _timescaledb_internal.get_partition_for_key(187::bigint); +SELECT _timescaledb_internal.get_partition_for_key(187::numeric); +SELECT _timescaledb_internal.get_partition_for_key(187::double precision); +SELECT _timescaledb_internal.get_partition_for_key(int4range(10, 20)); +SELECT _timescaledb_internal.get_partition_hash('08002b:010203'::macaddr); diff --git a/test/sql/util.sql b/test/sql/util.sql index 72b611922..315a4071d 100644 --- a/test/sql/util.sql +++ b/test/sql/util.sql @@ -3,7 +3,7 @@ DO $$ BEGIN - ASSERT( _timescaledb_internal.get_partition_for_key('') = 669664877 ); - ASSERT( _timescaledb_internal.get_partition_for_key('dev1') = 1129986420 ); - ASSERT( _timescaledb_internal.get_partition_for_key('longlonglonglongpartitionkey') = 1169179734); + ASSERT( _timescaledb_internal.get_partition_for_key(''::text) = 669664877 ); + ASSERT( _timescaledb_internal.get_partition_for_key('dev1'::text) = 1129986420 ); + ASSERT( _timescaledb_internal.get_partition_for_key('longlonglonglongpartitionkey'::text) = 1169179734); END$$;