From 5f40adcddbaaefd1ad32122c4b945fa44730ffac Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Thu, 25 Aug 2016 13:22:30 -0600 Subject: [PATCH 1/6] rework changes to limit to only rewriting ROW() references inside CREATE INDEX index parameters. Also fix ruleutils_95.c#get_variable so that Citus can actually select system columns from worker shard tables. --- .../distributed/relay/relay_event_utility.c | 77 ++++++++++++++++++- src/backend/distributed/utils/ruleutils_95.c | 2 +- 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/src/backend/distributed/relay/relay_event_utility.c b/src/backend/distributed/relay/relay_event_utility.c index 66dd80fa3..fc6f91e66 100644 --- a/src/backend/distributed/relay/relay_event_utility.c +++ b/src/backend/distributed/relay/relay_event_utility.c @@ -30,6 +30,7 @@ #include "distributed/relay_utility.h" #include "lib/stringinfo.h" #include "nodes/nodes.h" +#include "nodes/nodeFuncs.h" #include "nodes/parsenodes.h" #include "nodes/pg_list.h" #include "nodes/primnodes.h" @@ -41,6 +42,13 @@ #include "utils/palloc.h" #include "utils/relcache.h" +/* expression tree walker context for rewriting row references */ +typedef struct +{ + char *relationName; + uint64 shardId; +} RowRefWalkerState; + /* Local functions forward declarations */ static bool TypeAddIndexConstraint(const AlterTableCmd *command); @@ -48,7 +56,9 @@ static bool TypeDropIndexConstraint(const AlterTableCmd *command, const RangeVar *relation, uint64 shardId); static void AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId); static void SetSchemaNameIfNotExist(char **schemaName, char *newSchemaName); - +static bool ExtendRowReferencesWalker(Node *node, RowRefWalkerState *state); +static void AppendShardIdToRowReferences(IndexStmt *statement, char *relationName, uint64 + shardId); /* * RelayEventExtendNames extends relation names in the given parse tree for @@ -315,6 +325,7 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId) /* prefix with schema name if it is not added already */ SetSchemaNameIfNotExist(relationSchemaName, schemaName); + AppendShardIdToRowReferences(indexStmt, *relationName, shardId); AppendShardIdToName(relationName, shardId); AppendShardIdToName(indexName, shardId); break; @@ -536,6 +547,70 @@ AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId) } +static bool +ExtendRowReferencesWalker(Node *node, RowRefWalkerState *state) +{ + if (node == NULL) + { + return false; + } + + if (IsA(node, IndexElem)) + { + IndexElem *indexElem = (IndexElem *) node; + + return raw_expression_tree_walker(indexElem->expr, ExtendRowReferencesWalker, + state); + } + else if (IsA(node, ColumnRef)) + { + ColumnRef *columnRef = (ColumnRef *) node; + ListCell *fieldsCell; + + /* + * Append the shardId to any ColumnRef String values that are + * equal to the relationName. These are actually ROW(relname) + * references. + */ + foreach(fieldsCell, columnRef->fields) + { + Value *fieldValue = (Value *) lfirst(fieldsCell); + + if (IsA(fieldValue, String)) + { + char *columnName = strVal(fieldValue); + + if (strncmp(columnName, state->relationName, NAMEDATALEN) == 0) + { + AppendShardIdToName(&columnName, state->shardId); + fieldsCell->data.ptr_value = makeString(columnName); + } + } + } + } + + return raw_expression_tree_walker(node, ExtendRowReferencesWalker, (void *) state); +} + + +/* + * AppendShardIdToRowReferences finds ColumnRef nodes that directly reference + * a column with the same name as the relation and extends those names with the + * given shardId. + */ +static void +AppendShardIdToRowReferences(IndexStmt *indexStmt, char *relationName, uint64 shardId) +{ + RowRefWalkerState state; + + state.relationName = relationName; + state.shardId = shardId; + + raw_expression_tree_walker((Node *) indexStmt->indexParams, ExtendRowReferencesWalker, + &state); +} + + /* * SetSchemaNameIfNotExist function checks whether schemaName is set and if it is not set * it sets its value to given newSchemaName. diff --git a/src/backend/distributed/utils/ruleutils_95.c b/src/backend/distributed/utils/ruleutils_95.c index 5e5cf5e13..34e53c26a 100644 --- a/src/backend/distributed/utils/ruleutils_95.c +++ b/src/backend/distributed/utils/ruleutils_95.c @@ -3624,7 +3624,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) else { /* System column - name is fixed, get it from the catalog */ - attname = get_rte_attribute_name(rte, attnum); + attname = get_relid_attribute_name(rte->relid, attnum); } if (refname && (context->varprefix || attname == NULL)) From fd5cbcc7f8c7543cc5eb3590ef5fcee55b17b921 Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Wed, 31 Aug 2016 14:09:27 -0600 Subject: [PATCH 2/6] cleanup based on code review by @jasonmp85 --- .../distributed/relay/relay_event_utility.c | 80 ++++++++----------- src/backend/distributed/utils/ruleutils_95.c | 16 +++- 2 files changed, 49 insertions(+), 47 deletions(-) diff --git a/src/backend/distributed/relay/relay_event_utility.c b/src/backend/distributed/relay/relay_event_utility.c index fc6f91e66..06ecb59e1 100644 --- a/src/backend/distributed/relay/relay_event_utility.c +++ b/src/backend/distributed/relay/relay_event_utility.c @@ -45,9 +45,8 @@ /* expression tree walker context for rewriting row references */ typedef struct { - char *relationName; uint64 shardId; -} RowRefWalkerState; +} ColumnRefWalkerState; /* Local functions forward declarations */ @@ -56,9 +55,7 @@ static bool TypeDropIndexConstraint(const AlterTableCmd *command, const RangeVar *relation, uint64 shardId); static void AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId); static void SetSchemaNameIfNotExist(char **schemaName, char *newSchemaName); -static bool ExtendRowReferencesWalker(Node *node, RowRefWalkerState *state); -static void AppendShardIdToRowReferences(IndexStmt *statement, char *relationName, uint64 - shardId); +static bool UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state); /* * RelayEventExtendNames extends relation names in the given parse tree for @@ -297,6 +294,7 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId) case T_IndexStmt: { IndexStmt *indexStmt = (IndexStmt *) parseTree; + ColumnRefWalkerState state; char **relationName = &(indexStmt->relation->relname); char **indexName = &(indexStmt->idxname); char **relationSchemaName = &(indexStmt->relation->schemaname); @@ -322,10 +320,14 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId) ereport(ERROR, (errmsg("cannot extend name for null index name"))); } + /* extend ColumnRef nodes in the IndexStmt with the shardId */ + state.shardId = shardId; + raw_expression_tree_walker((Node *) indexStmt->indexParams, + UpdateWholeRowColumnReferencesWalker, &state); + /* prefix with schema name if it is not added already */ SetSchemaNameIfNotExist(relationSchemaName, schemaName); - AppendShardIdToRowReferences(indexStmt, *relationName, shardId); AppendShardIdToName(relationName, shardId); AppendShardIdToName(indexName, shardId); break; @@ -548,8 +550,10 @@ AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId) static bool -ExtendRowReferencesWalker(Node *node, RowRefWalkerState *state) +UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state) { + bool walkIsComplete = false; + if (node == NULL) { return false; @@ -559,55 +563,41 @@ ExtendRowReferencesWalker(Node *node, RowRefWalkerState *state) { IndexElem *indexElem = (IndexElem *) node; - return raw_expression_tree_walker(indexElem->expr, ExtendRowReferencesWalker, - state); + walkIsComplete = raw_expression_tree_walker(indexElem->expr, + UpdateWholeRowColumnReferencesWalker, + state); } else if (IsA(node, ColumnRef)) { ColumnRef *columnRef = (ColumnRef *) node; - ListCell *fieldsCell; + Node *lastField = llast(columnRef->fields); - /* - * Append the shardId to any ColumnRef String values that are - * equal to the relationName. These are actually ROW(relname) - * references. - */ - foreach(fieldsCell, columnRef->fields) + if (IsA(lastField, A_Star)) { - Value *fieldValue = (Value *) lfirst(fieldsCell); + /* + * ColumnRef fields list ends with an A_Star, so we can blindly + * extend the penultimate element with the shardId. + */ + Value *relnameValue; + int len = list_length(columnRef->fields); - if (IsA(fieldValue, String)) - { - char *columnName = strVal(fieldValue); + relnameValue = list_nth(columnRef->fields, len - 2); + Assert(IsA(relnameValue, String)); - if (strncmp(columnName, state->relationName, NAMEDATALEN) == 0) - { - AppendShardIdToName(&columnName, state->shardId); - fieldsCell->data.ptr_value = makeString(columnName); - } - } + AppendShardIdToName(&relnameValue->val.str, state->shardId); } + + /* might be more than one ColumnRef to visit */ + walkIsComplete = false; + } + else + { + walkIsComplete = raw_expression_tree_walker(node, + UpdateWholeRowColumnReferencesWalker, + state); } - return raw_expression_tree_walker(node, ExtendRowReferencesWalker, (void *) state); -} - - -/* - * AppendShardIdToRowReferences finds ColumnRef nodes that directly reference - * a column with the same name as the relation and extends those names with the - * given shardId. - */ -static void -AppendShardIdToRowReferences(IndexStmt *indexStmt, char *relationName, uint64 shardId) -{ - RowRefWalkerState state; - - state.relationName = relationName; - state.shardId = shardId; - - raw_expression_tree_walker((Node *) indexStmt->indexParams, ExtendRowReferencesWalker, - &state); + return walkIsComplete; } diff --git a/src/backend/distributed/utils/ruleutils_95.c b/src/backend/distributed/utils/ruleutils_95.c index 34e53c26a..4802e66fb 100644 --- a/src/backend/distributed/utils/ruleutils_95.c +++ b/src/backend/distributed/utils/ruleutils_95.c @@ -3623,8 +3623,20 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) } else { - /* System column - name is fixed, get it from the catalog */ - attname = get_relid_attribute_name(rte->relid, attnum); + CitusRTEKind rtekind; + + rtekind = GetRangeTblKind(rte); + + if (rtekind == CITUS_RTE_SHARD || rtekind == CITUS_RTE_REMOTE_QUERY) + { + /* System column on a Citus shared/remote relation */ + attname = get_relid_attribute_name(rte->relid, attnum); + } + else + { + /* System column - name is fixed, get it from the catalog */ + attname = get_rte_attribute_name(rte, attnum); + } } if (refname && (context->varprefix || attname == NULL)) From ef64fa89ce4c0f5cc59c27da6e7fa394ec74257b Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Wed, 31 Aug 2016 15:02:04 -0600 Subject: [PATCH 3/6] add tests --- .../regress/expected/multi_index_statements.out | 10 +++++++--- .../regress/expected/multi_simple_queries.out | 16 ++++++++++++++++ src/test/regress/sql/multi_index_statements.sql | 3 +++ src/test/regress/sql/multi_simple_queries.sql | 12 ++++++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/test/regress/expected/multi_index_statements.out b/src/test/regress/expected/multi_index_statements.out index 2a77a0e83..e1ed574ac 100644 --- a/src/test/regress/expected/multi_index_statements.out +++ b/src/test/regress/expected/multi_index_statements.out @@ -70,6 +70,7 @@ HINT: You can enable two-phase commit for extra safety with: SET citus.multi_sh CREATE INDEX lineitem_partkey_desc_index ON lineitem (l_partkey DESC); CREATE INDEX lineitem_partial_index ON lineitem (l_shipdate) WHERE l_shipdate < '1995-01-01'; +CREATE INDEX lineitem_colref_index ON lineitem (record_ne(lineitem.*, NULL)); SET client_min_messages = ERROR; -- avoid version dependant warning about WAL CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey); CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range(a); @@ -85,19 +86,20 @@ SELECT * FROM pg_indexes WHERE tablename = 'lineitem' or tablename like 'index_t public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON index_test_hash USING btree (a, b) public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range USING btree (a) public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON index_test_range USING btree (a, b) + public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON lineitem USING btree (record_ne(lineitem.*, NULL::record)) public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey) public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON lineitem USING btree (l_orderkey) public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON lineitem USING btree (l_partkey DESC) public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON lineitem USING btree (l_orderkey, l_linenumber) public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON lineitem USING btree (l_shipdate) -(10 rows) +(11 rows) \c - - - :worker_1_port SELECT count(*) FROM pg_indexes WHERE tablename = (SELECT relname FROM pg_class WHERE relname LIKE 'lineitem%' ORDER BY relname LIMIT 1); count ------- - 6 + 7 (1 row) SELECT count(*) FROM pg_indexes WHERE tablename LIKE 'index_test_hash%'; @@ -161,13 +163,14 @@ SELECT * FROM pg_indexes WHERE tablename = 'lineitem' or tablename like 'index_t public | index_test_hash | index_test_hash_index_a_b | | CREATE UNIQUE INDEX index_test_hash_index_a_b ON index_test_hash USING btree (a, b) public | index_test_range | index_test_range_index_a | | CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range USING btree (a) public | index_test_range | index_test_range_index_a_b | | CREATE UNIQUE INDEX index_test_range_index_a_b ON index_test_range USING btree (a, b) + public | lineitem | lineitem_colref_index | | CREATE INDEX lineitem_colref_index ON lineitem USING btree (record_ne(lineitem.*, NULL::record)) public | lineitem | lineitem_orderkey_hash_index | | CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey) public | lineitem | lineitem_orderkey_index | | CREATE INDEX lineitem_orderkey_index ON lineitem USING btree (l_orderkey) public | lineitem | lineitem_partial_index | | CREATE INDEX lineitem_partial_index ON lineitem USING btree (l_shipdate) WHERE (l_shipdate < '01-01-1995'::date) public | lineitem | lineitem_partkey_desc_index | | CREATE INDEX lineitem_partkey_desc_index ON lineitem USING btree (l_partkey DESC) public | lineitem | lineitem_pkey | | CREATE UNIQUE INDEX lineitem_pkey ON lineitem USING btree (l_orderkey, l_linenumber) public | lineitem | lineitem_time_index | | CREATE INDEX lineitem_time_index ON lineitem USING btree (l_shipdate) -(10 rows) +(11 rows) -- -- DROP INDEX @@ -183,6 +186,7 @@ ERROR: dropping indexes concurrently on distributed tables is currently unsuppo DROP INDEX lineitem_orderkey_index; DROP INDEX lineitem_partkey_desc_index; DROP INDEX lineitem_partial_index; +DROP INDEX lineitem_colref_index; -- Verify that we handle if exists statements correctly DROP INDEX non_existent_index; ERROR: index "non_existent_index" does not exist diff --git a/src/test/regress/expected/multi_simple_queries.out b/src/test/regress/expected/multi_simple_queries.out index 8ea598c34..35bdf3e98 100644 --- a/src/test/regress/expected/multi_simple_queries.out +++ b/src/test/regress/expected/multi_simple_queries.out @@ -502,4 +502,20 @@ DEBUG: pruning merge fetch taskId 11 DETAIL: Creating dependency on merge taskId 14 ERROR: cannot use real time executor with repartition jobs HINT: Set citus.task_executor_type to "task-tracker". +-- system columns from shard tables can be queried retrieved +SELECT count(*) FROM ( + SELECT tableoid, ctid, cmin, cmax, xmin, xmax + FROM articles + WHERE tableoid IS NOT NULL OR + ctid IS NOT NULL OR + cmin IS NOT NULL OR + cmax IS NOT NULL OR + xmin IS NOT NULL OR + xmax IS NOT NULL +) x; + count +------- + 50 +(1 row) + SET client_min_messages to 'NOTICE'; diff --git a/src/test/regress/sql/multi_index_statements.sql b/src/test/regress/sql/multi_index_statements.sql index a71d5b82d..268fd90fb 100644 --- a/src/test/regress/sql/multi_index_statements.sql +++ b/src/test/regress/sql/multi_index_statements.sql @@ -43,6 +43,8 @@ CREATE INDEX lineitem_partkey_desc_index ON lineitem (l_partkey DESC); CREATE INDEX lineitem_partial_index ON lineitem (l_shipdate) WHERE l_shipdate < '1995-01-01'; +CREATE INDEX lineitem_colref_index ON lineitem (record_ne(lineitem.*, NULL)); + SET client_min_messages = ERROR; -- avoid version dependant warning about WAL CREATE INDEX lineitem_orderkey_hash_index ON lineitem USING hash (l_partkey); CREATE UNIQUE INDEX index_test_range_index_a ON index_test_range(a); @@ -97,6 +99,7 @@ DROP INDEX CONCURRENTLY lineitem_orderkey_index; DROP INDEX lineitem_orderkey_index; DROP INDEX lineitem_partkey_desc_index; DROP INDEX lineitem_partial_index; +DROP INDEX lineitem_colref_index; -- Verify that we handle if exists statements correctly diff --git a/src/test/regress/sql/multi_simple_queries.sql b/src/test/regress/sql/multi_simple_queries.sql index 7e3cf9f3c..beaf16947 100644 --- a/src/test/regress/sql/multi_simple_queries.sql +++ b/src/test/regress/sql/multi_simple_queries.sql @@ -260,4 +260,16 @@ SELECT * FROM articles a, articles b WHERE a.id = b.id AND a.author_id = 1; +-- system columns from shard tables can be queried retrieved +SELECT count(*) FROM ( + SELECT tableoid, ctid, cmin, cmax, xmin, xmax + FROM articles + WHERE tableoid IS NOT NULL OR + ctid IS NOT NULL OR + cmin IS NOT NULL OR + cmax IS NOT NULL OR + xmin IS NOT NULL OR + xmax IS NOT NULL +) x; + SET client_min_messages to 'NOTICE'; From 8ca64e657dd5a916068b16b4097a1a8511d7c6eb Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Fri, 2 Sep 2016 10:56:24 -0600 Subject: [PATCH 4/6] cleanup based on review by @jasonmp85 --- .../distributed/relay/relay_event_utility.c | 37 +++++++++---------- src/backend/distributed/utils/ruleutils_95.c | 21 ++++------- .../regress/expected/multi_simple_queries.out | 2 +- src/test/regress/sql/multi_simple_queries.sql | 2 +- 4 files changed, 26 insertions(+), 36 deletions(-) diff --git a/src/backend/distributed/relay/relay_event_utility.c b/src/backend/distributed/relay/relay_event_utility.c index 06ecb59e1..76b5f0177 100644 --- a/src/backend/distributed/relay/relay_event_utility.c +++ b/src/backend/distributed/relay/relay_event_utility.c @@ -42,20 +42,13 @@ #include "utils/palloc.h" #include "utils/relcache.h" -/* expression tree walker context for rewriting row references */ -typedef struct -{ - uint64 shardId; -} ColumnRefWalkerState; - - /* Local functions forward declarations */ static bool TypeAddIndexConstraint(const AlterTableCmd *command); static bool TypeDropIndexConstraint(const AlterTableCmd *command, const RangeVar *relation, uint64 shardId); static void AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId); static void SetSchemaNameIfNotExist(char **schemaName, char *newSchemaName); -static bool UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state); +static bool UpdateWholeRowColumnReferencesWalker(Node *node, uint64 *shardId); /* * RelayEventExtendNames extends relation names in the given parse tree for @@ -294,7 +287,6 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId) case T_IndexStmt: { IndexStmt *indexStmt = (IndexStmt *) parseTree; - ColumnRefWalkerState state; char **relationName = &(indexStmt->relation->relname); char **indexName = &(indexStmt->idxname); char **relationSchemaName = &(indexStmt->relation->schemaname); @@ -321,9 +313,8 @@ RelayEventExtendNames(Node *parseTree, char *schemaName, uint64 shardId) } /* extend ColumnRef nodes in the IndexStmt with the shardId */ - state.shardId = shardId; - raw_expression_tree_walker((Node *) indexStmt->indexParams, - UpdateWholeRowColumnReferencesWalker, &state); + UpdateWholeRowColumnReferencesWalker((Node *) indexStmt->indexParams, + &shardId); /* prefix with schema name if it is not added already */ SetSchemaNameIfNotExist(relationSchemaName, schemaName); @@ -549,8 +540,16 @@ AppendShardIdToConstraintName(AlterTableCmd *command, uint64 shardId) } +/* + * UpdateWholeRowColumnReferencesWalker extends ColumnRef nodes that end with A_Star + * with the given shardId. + * + * ColumnRefs that don't reference A_Star are not extended as catalog access isn't + * allowed here and we don't otherwise have enough context to disambiguate a + * field name that is identical to the table name. + */ static bool -UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state) +UpdateWholeRowColumnReferencesWalker(Node *node, uint64 *shardId) { bool walkIsComplete = false; @@ -565,7 +564,7 @@ UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state) walkIsComplete = raw_expression_tree_walker(indexElem->expr, UpdateWholeRowColumnReferencesWalker, - state); + shardId); } else if (IsA(node, ColumnRef)) { @@ -578,13 +577,11 @@ UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state) * ColumnRef fields list ends with an A_Star, so we can blindly * extend the penultimate element with the shardId. */ - Value *relnameValue; - int len = list_length(columnRef->fields); - - relnameValue = list_nth(columnRef->fields, len - 2); + int colrefFieldCount = list_length(columnRef->fields); + Value *relnameValue = list_nth(columnRef->fields, colrefFieldCount - 2); Assert(IsA(relnameValue, String)); - AppendShardIdToName(&relnameValue->val.str, state->shardId); + AppendShardIdToName(&relnameValue->val.str, *shardId); } /* might be more than one ColumnRef to visit */ @@ -594,7 +591,7 @@ UpdateWholeRowColumnReferencesWalker(Node *node, ColumnRefWalkerState *state) { walkIsComplete = raw_expression_tree_walker(node, UpdateWholeRowColumnReferencesWalker, - state); + shardId); } return walkIsComplete; diff --git a/src/backend/distributed/utils/ruleutils_95.c b/src/backend/distributed/utils/ruleutils_95.c index 4802e66fb..e10bcaf98 100644 --- a/src/backend/distributed/utils/ruleutils_95.c +++ b/src/backend/distributed/utils/ruleutils_95.c @@ -3621,22 +3621,15 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) attname = colinfo->colnames[attnum - 1]; Assert(attname != NULL); } + else if (GetRangeTblKind(rte) == CITUS_RTE_SHARD) + { + /* System column on a Citus shared/remote relation */ + attname = get_relid_attribute_name(rte->relid, attnum); + } else { - CitusRTEKind rtekind; - - rtekind = GetRangeTblKind(rte); - - if (rtekind == CITUS_RTE_SHARD || rtekind == CITUS_RTE_REMOTE_QUERY) - { - /* System column on a Citus shared/remote relation */ - attname = get_relid_attribute_name(rte->relid, attnum); - } - else - { - /* System column - name is fixed, get it from the catalog */ - attname = get_rte_attribute_name(rte, attnum); - } + /* System column - name is fixed, get it from the catalog */ + attname = get_rte_attribute_name(rte, attnum); } if (refname && (context->varprefix || attname == NULL)) diff --git a/src/test/regress/expected/multi_simple_queries.out b/src/test/regress/expected/multi_simple_queries.out index 35bdf3e98..29af5caf4 100644 --- a/src/test/regress/expected/multi_simple_queries.out +++ b/src/test/regress/expected/multi_simple_queries.out @@ -502,7 +502,7 @@ DEBUG: pruning merge fetch taskId 11 DETAIL: Creating dependency on merge taskId 14 ERROR: cannot use real time executor with repartition jobs HINT: Set citus.task_executor_type to "task-tracker". --- system columns from shard tables can be queried retrieved +-- system columns from shard tables can be queried and retrieved SELECT count(*) FROM ( SELECT tableoid, ctid, cmin, cmax, xmin, xmax FROM articles diff --git a/src/test/regress/sql/multi_simple_queries.sql b/src/test/regress/sql/multi_simple_queries.sql index beaf16947..98f73cec4 100644 --- a/src/test/regress/sql/multi_simple_queries.sql +++ b/src/test/regress/sql/multi_simple_queries.sql @@ -260,7 +260,7 @@ SELECT * FROM articles a, articles b WHERE a.id = b.id AND a.author_id = 1; --- system columns from shard tables can be queried retrieved +-- system columns from shard tables can be queried and retrieved SELECT count(*) FROM ( SELECT tableoid, ctid, cmin, cmax, xmin, xmax FROM articles From 3b024cac34741d6f6825bf5de142e190f62e7782 Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Fri, 2 Sep 2016 11:07:44 -0600 Subject: [PATCH 5/6] adjust comment --- src/backend/distributed/utils/ruleutils_95.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/utils/ruleutils_95.c b/src/backend/distributed/utils/ruleutils_95.c index e10bcaf98..ffc8e79b0 100644 --- a/src/backend/distributed/utils/ruleutils_95.c +++ b/src/backend/distributed/utils/ruleutils_95.c @@ -3623,7 +3623,7 @@ get_variable(Var *var, int levelsup, bool istoplevel, deparse_context *context) } else if (GetRangeTblKind(rte) == CITUS_RTE_SHARD) { - /* System column on a Citus shared/remote relation */ + /* System column on a Citus shard */ attname = get_relid_attribute_name(rte->relid, attnum); } else From 2b5bd3815317e4d79b9c6002532ad4286864d71a Mon Sep 17 00:00:00 2001 From: "Eric B. Ridge" Date: Fri, 2 Sep 2016 14:23:29 -0600 Subject: [PATCH 6/6] disallow creating distributed tables when the table has WITH (OIDS) set, as per @jasonmp85. --- .../commands/create_distributed_table.c | 17 +++++++++++++++++ .../regress/expected/multi_create_shards.out | 6 ++++++ src/test/regress/sql/multi_create_shards.sql | 7 +++++++ 3 files changed, 30 insertions(+) diff --git a/src/backend/distributed/commands/create_distributed_table.c b/src/backend/distributed/commands/create_distributed_table.c index 22402b9af..e725dc0b0 100644 --- a/src/backend/distributed/commands/create_distributed_table.c +++ b/src/backend/distributed/commands/create_distributed_table.c @@ -50,6 +50,7 @@ static void RecordDistributedRelationDependencies(Oid distributedRelationId, static Oid SupportFunctionForColumn(Var *partitionColumn, Oid accessMethodId, int16 supportFunctionNumber); static bool LocalTableEmpty(Oid tableId); +static void ErrorIfTableHasOids(Relation relation); /* exports for SQL callable functions */ @@ -101,6 +102,7 @@ master_create_distributed_table(PG_FUNCTION_ARGS) distributedRelationName = RelationGetRelationName(distributedRelation); EnsureTableOwner(distributedRelationId); + ErrorIfTableHasOids(distributedRelation); /* open system catalog and insert new tuple */ pgDistPartition = heap_open(DistPartitionRelationId(), RowExclusiveLock); @@ -446,3 +448,18 @@ LocalTableEmpty(Oid tableId) return localTableEmpty; } + + +/* + * ErrorIfTableHasOids raises an ERROR if a to-be-distributed table + * has the WITH (OIDS) option set. + */ +static void +ErrorIfTableHasOids(Relation relation) +{ + if (relation->rd_att->tdhasoid) + { + ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("WITH (OIDS) not supported on distributed tables"))); + } +} diff --git a/src/test/regress/expected/multi_create_shards.out b/src/test/regress/expected/multi_create_shards.out index cf3dfc3cd..bc7629380 100644 --- a/src/test/regress/expected/multi_create_shards.out +++ b/src/test/regress/expected/multi_create_shards.out @@ -40,6 +40,12 @@ CREATE TABLE table_to_distribute ( json_data json, test_type_data dummy_type ); +-- use the table WITH (OIDS) set +ALTER TABLE table_to_distribute SET WITH OIDS; +SELECT master_create_distributed_table('table_to_distribute', 'id', 'hash'); +ERROR: WITH (OIDS) not supported on distributed tables +-- revert WITH (OIDS) from above +ALTER TABLE table_to_distribute SET WITHOUT OIDS; -- use an index instead of table name SELECT master_create_distributed_table('table_to_distribute_pkey', 'id', 'hash'); ERROR: cannot distribute relation: table_to_distribute_pkey diff --git a/src/test/regress/sql/multi_create_shards.sql b/src/test/regress/sql/multi_create_shards.sql index c5e38fa63..b49f0c0f4 100644 --- a/src/test/regress/sql/multi_create_shards.sql +++ b/src/test/regress/sql/multi_create_shards.sql @@ -52,6 +52,13 @@ CREATE TABLE table_to_distribute ( test_type_data dummy_type ); +-- use the table WITH (OIDS) set +ALTER TABLE table_to_distribute SET WITH OIDS; +SELECT master_create_distributed_table('table_to_distribute', 'id', 'hash'); + +-- revert WITH (OIDS) from above +ALTER TABLE table_to_distribute SET WITHOUT OIDS; + -- use an index instead of table name SELECT master_create_distributed_table('table_to_distribute_pkey', 'id', 'hash');