From 544570708bba4b984ef86b82e19b91179608d2f9 Mon Sep 17 00:00:00 2001 From: Robin Thomas Date: Fri, 12 Aug 2016 09:46:29 -0400 Subject: [PATCH] Remove pg_dist_shard.shardalias, LoadShardAlias, and all usage of these. Draft of migration script to drop the column. Removed regression test of non-null shardalias. --- .../distributed/citus--5.2-1--5.3-0.sql | 5 ++ src/backend/distributed/citus.sql | 1 - .../master/master_delete_protocol.c | 15 +--- .../master/master_metadata_utility.c | 70 ------------------- .../master/master_stage_protocol.c | 26 +++---- .../planner/multi_physical_planner.c | 44 +++--------- .../distributed/master_metadata_utility.h | 1 - src/include/distributed/pg_dist_shard.h | 8 +-- src/test/regress/.gitignore | 2 + src/test/regress/expected/multi_table_ddl.out | 4 +- .../multi_verify_no_join_with_alias.out | 22 ------ src/test/regress/multi_schedule | 1 - .../regress/multi_task_tracker_extra_schedule | 1 - .../sql/multi_verify_no_join_with_alias.sql | 27 ------- 14 files changed, 34 insertions(+), 193 deletions(-) create mode 100644 src/backend/distributed/citus--5.2-1--5.3-0.sql delete mode 100644 src/test/regress/expected/multi_verify_no_join_with_alias.out delete mode 100644 src/test/regress/sql/multi_verify_no_join_with_alias.sql diff --git a/src/backend/distributed/citus--5.2-1--5.3-0.sql b/src/backend/distributed/citus--5.2-1--5.3-0.sql new file mode 100644 index 000000000..0488a2b13 --- /dev/null +++ b/src/backend/distributed/citus--5.2-1--5.3-0.sql @@ -0,0 +1,5 @@ +/* citus--5.2-1--5.3-0.sql */ + +ALTER TABLE pg_catalog.pg_dist_shard +DROP COLUMN IF EXISTS shardalias; + diff --git a/src/backend/distributed/citus.sql b/src/backend/distributed/citus.sql index 60cc75079..ab9e61909 100644 --- a/src/backend/distributed/citus.sql +++ b/src/backend/distributed/citus.sql @@ -42,7 +42,6 @@ CREATE TABLE citus.pg_dist_shard( logicalrelid oid NOT NULL, shardid int8 NOT NULL, shardstorage "char" NOT NULL, - shardalias text, shardminvalue text, shardmaxvalue text ); diff --git a/src/backend/distributed/master/master_delete_protocol.c b/src/backend/distributed/master/master_delete_protocol.c index 967be29fe..905fb703d 100644 --- a/src/backend/distributed/master/master_delete_protocol.c +++ b/src/backend/distributed/master/master_delete_protocol.c @@ -325,18 +325,9 @@ DropShards(Oid relationId, char *schemaName, char *relationName, Assert(shardInterval->relationId == relationId); - /* if shard doesn't have an alias, extend regular table name */ - shardAlias = LoadShardAlias(relationId, shardId); - if (shardAlias == NULL) - { - appendStringInfoString(shardName, relationName); - AppendShardIdToStringInfo(shardName, shardId); - } - else - { - appendStringInfoString(shardName, shardAlias); - } - + /* Build shard relation name. */ + appendStringInfoString(shardName, relationName); + AppendShardIdToStringInfo(shardName, shardId); quotedShardName = quote_qualified_identifier(schemaName, shardName->data); shardPlacementList = ShardPlacementList(shardId); diff --git a/src/backend/distributed/master/master_metadata_utility.c b/src/backend/distributed/master/master_metadata_utility.c index 97faa6518..c92dbcfdd 100644 --- a/src/backend/distributed/master/master_metadata_utility.c +++ b/src/backend/distributed/master/master_metadata_utility.c @@ -138,72 +138,6 @@ AllocateUint64(uint64 value) } -/* - * LoadShardAlias finds the row for given relation and shardId in pg_dist_shard, - * finds the shard alias in this row if any, and then deep copies this alias. - */ -char * -LoadShardAlias(Oid relationId, uint64 shardId) -{ - SysScanDesc scanDescriptor = NULL; - ScanKeyData scanKey[1]; - int scanKeyCount = 1; - HeapTuple heapTuple = NULL; - Datum shardAliasDatum = 0; - bool shardAliasNull = false; - char *shardAlias = NULL; - - Relation pgDistShard = heap_open(DistShardRelationId(), AccessShareLock); - TupleDesc tupleDescriptor = RelationGetDescr(pgDistShard); - - ScanKeyInit(&scanKey[0], Anum_pg_dist_shard_shardid, - BTEqualStrategyNumber, F_INT8EQ, Int64GetDatum(shardId)); - - scanDescriptor = systable_beginscan(pgDistShard, - DistShardShardidIndexId(), true, - NULL, scanKeyCount, scanKey); - - /* - * Normally, we should have at most one tuple here as we have a unique index - * on shardId. However, if users want to drop this uniqueness constraint, - * and look up the shardalias based on the relation and shardId pair, we - * still allow that. We don't have any users relaying on this feature. Thus, - * we may consider to remove this check. - */ - heapTuple = systable_getnext(scanDescriptor); - while (HeapTupleIsValid(heapTuple)) - { - Form_pg_dist_shard pgDistShardForm = (Form_pg_dist_shard) GETSTRUCT(heapTuple); - if (pgDistShardForm->logicalrelid == relationId) - { - break; - } - - heapTuple = systable_getnext(scanDescriptor); - } - - /* if no tuple found, error out */ - if (!HeapTupleIsValid(heapTuple)) - { - ereport(ERROR, (errmsg("could not find valid entry for relationId: %u " - "and shard " UINT64_FORMAT, relationId, shardId))); - } - - /* if shard alias exists, deep copy cstring */ - shardAliasDatum = heap_getattr(heapTuple, Anum_pg_dist_shard_shardalias, - tupleDescriptor, &shardAliasNull); - if (!shardAliasNull) - { - shardAlias = TextDatumGetCString(shardAliasDatum); - } - - systable_endscan(scanDescriptor); - heap_close(pgDistShard, AccessShareLock); - - return shardAlias; -} - - /* * CopyShardInterval copies fields from the specified source ShardInterval * into the fields of the provided destination ShardInterval. @@ -405,15 +339,11 @@ InsertShardRow(Oid relationId, uint64 shardId, char storageType, { values[Anum_pg_dist_shard_shardminvalue - 1] = PointerGetDatum(shardMinValue); values[Anum_pg_dist_shard_shardmaxvalue - 1] = PointerGetDatum(shardMaxValue); - - /* we always set shard alias to null */ - isNulls[Anum_pg_dist_shard_shardalias - 1] = true; } else { isNulls[Anum_pg_dist_shard_shardminvalue - 1] = true; isNulls[Anum_pg_dist_shard_shardmaxvalue - 1] = true; - isNulls[Anum_pg_dist_shard_shardalias - 1] = true; } /* open shard relation and insert new tuple */ diff --git a/src/backend/distributed/master/master_stage_protocol.c b/src/backend/distributed/master/master_stage_protocol.c index 49941cc8f..0f7b69fb8 100644 --- a/src/backend/distributed/master/master_stage_protocol.c +++ b/src/backend/distributed/master/master_stage_protocol.c @@ -238,13 +238,9 @@ master_append_table_to_shard(PG_FUNCTION_ARGS) shardSchemaOid = get_rel_namespace(relationId); shardSchemaName = get_namespace_name(shardSchemaOid); - /* if shard doesn't have an alias, extend regular table name */ - shardTableName = LoadShardAlias(relationId, shardId); - if (shardTableName == NULL) - { - shardTableName = get_rel_name(relationId); - AppendShardIdToName(&shardTableName, shardId); - } + /* Build shard table name. */ + shardTableName = get_rel_name(relationId); + AppendShardIdToName(&shardTableName, shardId); shardQualifiedName = quote_qualified_identifier(shardSchemaName, shardTableName); @@ -493,19 +489,15 @@ UpdateShardStatistics(int64 shardId) text *minValue = NULL; text *maxValue = NULL; - /* if shard doesn't have an alias, extend regular table name */ - shardQualifiedName = LoadShardAlias(relationId, shardId); - if (shardQualifiedName == NULL) - { - char *shardName = get_rel_name(relationId); + /* Build shard qualified name. */ + char *shardName = get_rel_name(relationId); - Oid schemaId = get_rel_namespace(relationId); - char *schemaName = get_namespace_name(schemaId); + Oid schemaId = get_rel_namespace(relationId); + char *schemaName = get_namespace_name(schemaId); - AppendShardIdToName(&shardName, shardId); + AppendShardIdToName(&shardName, shardId); - shardQualifiedName = quote_qualified_identifier(schemaName, shardName); - } + shardQualifiedName = quote_qualified_identifier(schemaName, shardName); shardPlacementList = FinalizedShardPlacementList(shardId); diff --git a/src/backend/distributed/planner/multi_physical_planner.c b/src/backend/distributed/planner/multi_physical_planner.c index 071b2d2d8..457ced0ec 100644 --- a/src/backend/distributed/planner/multi_physical_planner.c +++ b/src/backend/distributed/planner/multi_physical_planner.c @@ -3753,27 +3753,13 @@ ShardFetchQueryString(uint64 shardId) char *shardSchemaName = NULL; char *shardTableName = NULL; - /* - * If user specified a shard alias in pg_dist_shard, error out and display a - * message explaining the limitation. - */ - char *shardAliasName = LoadShardAlias(shardInterval->relationId, shardId); - if (shardAliasName != NULL) - { - ereport(ERROR, (errmsg("cannot fetch shard " UINT64_FORMAT, shardId), - errdetail("Fetching shards with aliases is currently " - "unsupported"))); - } - else - { - /* construct the shard name */ - Oid shardSchemaId = get_rel_namespace(shardInterval->relationId); - char *tableName = get_rel_name(shardInterval->relationId); + /* construct the shard name */ + Oid shardSchemaId = get_rel_namespace(shardInterval->relationId); + char *tableName = get_rel_name(shardInterval->relationId); - shardSchemaName = get_namespace_name(shardSchemaId); - shardTableName = pstrdup(tableName); - AppendShardIdToName(&shardTableName, shardId); - } + shardSchemaName = get_namespace_name(shardSchemaId); + shardTableName = pstrdup(tableName); + AppendShardIdToName(&shardTableName, shardId); shardFetchQuery = makeStringInfo(); if (storageType == SHARD_STORAGE_TABLE || storageType == SHARD_STORAGE_RELAY || @@ -3986,21 +3972,11 @@ FragmentAlias(RangeTblEntry *rangeTableEntry, RangeTableFragment *fragment) aliasName = relationName; /* - * If user specified a shard name in pg_dist_shard, use that name in alias. - * Otherwise, set shard name in alias to _. + * Set shard name in alias to _. */ - shardAliasName = LoadShardAlias(relationId, shardId); - if (shardAliasName != NULL) - { - fragmentName = shardAliasName; - } - else - { - char *shardName = pstrdup(relationName); - AppendShardIdToName(&shardName, shardId); - - fragmentName = shardName; - } + char *shardName = pstrdup(relationName); + AppendShardIdToName(&shardName, shardId); + fragmentName = shardName; } else if (fragmentType == CITUS_RTE_REMOTE_QUERY) { diff --git a/src/include/distributed/master_metadata_utility.h b/src/include/distributed/master_metadata_utility.h index 3df148fbc..d7610d310 100644 --- a/src/include/distributed/master_metadata_utility.h +++ b/src/include/distributed/master_metadata_utility.h @@ -59,7 +59,6 @@ typedef struct ShardPlacement extern List * LoadShardIntervalList(Oid relationId); extern int ShardIntervalCount(Oid relationId); extern List * LoadShardList(Oid relationId); -extern char * LoadShardAlias(Oid relationId, uint64 shardId); extern void CopyShardInterval(ShardInterval *srcInterval, ShardInterval *destInterval); extern uint64 ShardLength(uint64 shardId); extern List * FinalizedShardPlacementList(uint64 shardId); diff --git a/src/include/distributed/pg_dist_shard.h b/src/include/distributed/pg_dist_shard.h index 5a1dab298..7d3700130 100644 --- a/src/include/distributed/pg_dist_shard.h +++ b/src/include/distributed/pg_dist_shard.h @@ -26,7 +26,6 @@ typedef struct FormData_pg_dist_shard int64 shardid; /* global shardId representing remote partition */ char shardstorage; /* shard storage type; see codes below */ #ifdef CATALOG_VARLEN /* variable-length fields start here */ - text shardalias; /* user specified table name for shard, if any */ text shardminvalue; /* partition key's minimum value in shard */ text shardmaxvalue; /* partition key's maximum value in shard */ #endif @@ -43,13 +42,12 @@ typedef FormData_pg_dist_shard *Form_pg_dist_shard; * compiler constants for pg_dist_shards * ---------------- */ -#define Natts_pg_dist_shard 6 +#define Natts_pg_dist_shard 5 #define Anum_pg_dist_shard_logicalrelid 1 #define Anum_pg_dist_shard_shardid 2 #define Anum_pg_dist_shard_shardstorage 3 -#define Anum_pg_dist_shard_shardalias 4 -#define Anum_pg_dist_shard_shardminvalue 5 -#define Anum_pg_dist_shard_shardmaxvalue 6 +#define Anum_pg_dist_shard_shardminvalue 4 +#define Anum_pg_dist_shard_shardmaxvalue 5 /* * Valid values for shard storage types include relay file, foreign table, diff --git a/src/test/regress/.gitignore b/src/test/regress/.gitignore index 7573addc9..2b837bc9b 100644 --- a/src/test/regress/.gitignore +++ b/src/test/regress/.gitignore @@ -5,3 +5,5 @@ /tmp_check/ /results/ /log/ +/regression.out +/regression.diffs diff --git a/src/test/regress/expected/multi_table_ddl.out b/src/test/regress/expected/multi_table_ddl.out index 59f398bb4..712da8bd3 100644 --- a/src/test/regress/expected/multi_table_ddl.out +++ b/src/test/regress/expected/multi_table_ddl.out @@ -53,8 +53,8 @@ SELECT * FROM pg_dist_partition; (0 rows) SELECT * FROM pg_dist_shard; - logicalrelid | shardid | shardstorage | shardalias | shardminvalue | shardmaxvalue ---------------+---------+--------------+------------+---------------+--------------- + logicalrelid | shardid | shardstorage | shardminvalue | shardmaxvalue +--------------+---------+--------------+---------------+--------------- (0 rows) SELECT * FROM pg_dist_shard_placement; diff --git a/src/test/regress/expected/multi_verify_no_join_with_alias.out b/src/test/regress/expected/multi_verify_no_join_with_alias.out deleted file mode 100644 index 0e6e0df20..000000000 --- a/src/test/regress/expected/multi_verify_no_join_with_alias.out +++ /dev/null @@ -1,22 +0,0 @@ --- --- MULTI_VERIFY_NO_JOIN_WITH_ALIAS --- --- This test checks that we simply emit an error message instead of trying to --- fetch and join a shard which has an alias set. -ALTER SEQUENCE pg_catalog.pg_dist_shardid_seq RESTART 1020000; -ALTER SEQUENCE pg_catalog.pg_dist_jobid_seq RESTART 1020000; --- Show that the join works without an alias -SELECT COUNT(*) FROM lineitem, part WHERE l_partkey = p_partkey; - count -------- - 61 -(1 row) - --- Assign an alias to the parts shard -UPDATE pg_dist_shard SET shardalias = 'my_alias' WHERE shardid = 290000; --- Attempt a join which uses this shard -SELECT COUNT(*) FROM lineitem, part WHERE l_partkey = p_partkey; -ERROR: cannot fetch shard 290000 -DETAIL: Fetching shards with aliases is currently unsupported --- Remove the alias from the parts shard -UPDATE pg_dist_shard SET shardalias = NULL WHERE shardid = 290000; diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index 3f159ee7c..7ea2e94d6 100644 --- a/src/test/regress/multi_schedule +++ b/src/test/regress/multi_schedule @@ -44,7 +44,6 @@ test: multi_query_directory_cleanup test: multi_task_assignment_policy test: multi_utility_statements test: multi_dropped_column_aliases -test: multi_verify_no_join_with_alias test: multi_binary_master_copy_format test: multi_prepare_sql multi_prepare_plsql diff --git a/src/test/regress/multi_task_tracker_extra_schedule b/src/test/regress/multi_task_tracker_extra_schedule index 27276f83c..7482d4f42 100644 --- a/src/test/regress/multi_task_tracker_extra_schedule +++ b/src/test/regress/multi_task_tracker_extra_schedule @@ -39,7 +39,6 @@ test: multi_query_directory_cleanup test: multi_task_assignment_policy test: multi_utility_statements test: multi_dropped_column_aliases -test: multi_verify_no_join_with_alias # ---------- # Parallel TPC-H tests to check our distributed execution behavior diff --git a/src/test/regress/sql/multi_verify_no_join_with_alias.sql b/src/test/regress/sql/multi_verify_no_join_with_alias.sql deleted file mode 100644 index 18e446df1..000000000 --- a/src/test/regress/sql/multi_verify_no_join_with_alias.sql +++ /dev/null @@ -1,27 +0,0 @@ --- --- MULTI_VERIFY_NO_JOIN_WITH_ALIAS --- - --- This test checks that we simply emit an error message instead of trying to --- fetch and join a shard which has an alias set. - - -ALTER SEQUENCE pg_catalog.pg_dist_shardid_seq RESTART 1020000; -ALTER SEQUENCE pg_catalog.pg_dist_jobid_seq RESTART 1020000; - - --- Show that the join works without an alias - -SELECT COUNT(*) FROM lineitem, part WHERE l_partkey = p_partkey; - --- Assign an alias to the parts shard - -UPDATE pg_dist_shard SET shardalias = 'my_alias' WHERE shardid = 290000; - --- Attempt a join which uses this shard - -SELECT COUNT(*) FROM lineitem, part WHERE l_partkey = p_partkey; - --- Remove the alias from the parts shard - -UPDATE pg_dist_shard SET shardalias = NULL WHERE shardid = 290000;