test test test

citusdata · Feb 9, 2022 · a2c4c06 · a2c4c06
1 parent 0d7e235
commit a2c4c06
Show file tree

Hide file tree

Showing 8 changed files with 149 additions and 36 deletions.
diff --git a/src/backend/distributed/deparser/citus_ruleutils.c b/src/backend/distributed/deparser/citus_ruleutils.c
@@ -80,7 +80,7 @@ static void deparse_index_columns(StringInfo buffer, List *indexParameterList,
 static void AppendStorageParametersToString(StringInfo stringBuffer,
 											List *optionList);
 static void simple_quote_literal(StringInfo buf, const char *val);
-static SubscriptingRef * TargetListEntryExprFindSubscriptingRef(Expr *expr);
+static SubscriptingRef * TargetEntryExprFindSubsRef(Expr *expr);
 static char * flatten_reloptions(Oid relid);
 static void AddVacuumParams(ReindexStmt *reindexStmt, StringInfo buffer);
 
@@ -1374,7 +1374,7 @@ ExpandMergedSubscriptingRefEntries(List *targetEntryList)
 		Expr *expr = targetEntry->expr;
 		while (expr)
 		{
-			SubscriptingRef *subsRef = TargetListEntryExprFindSubscriptingRef(expr);
+			SubscriptingRef *subsRef = TargetEntryExprFindSubsRef(expr);
 			if (!subsRef)
 			{
 				break;
@@ -1420,10 +1420,9 @@ ExpandMergedSubscriptingRefEntries(List *targetEntryList)
 
 
 /*
- * TargetListEntryExprFindSubscriptingRef searches given Expr --assuming that
- * it is part of a target list entry-- to see if it directly (i.e.: itself)
- * or indirectly (e.g.: behind some level of coercions) holds a SubscriptingRef
- * node.
+ * TargetEntryExprFindSubsRef searches given Expr --assuming that it is part
+ * of a target list entry-- to see if it directly (i.e.: itself) or indirectly
+ * (e.g.: behind some level of coercions) holds a SubscriptingRef node.
  *
  * Returns the original SubscriptingRef node on success or NULL otherwise.
  *
@@ -1432,7 +1431,7 @@ ExpandMergedSubscriptingRefEntries(List *targetEntryList)
  * node types.
  */
 static SubscriptingRef *
-TargetListEntryExprFindSubscriptingRef(Expr *expr)
+TargetEntryExprFindSubsRef(Expr *expr)
 {
 	Node *node = (Node *) expr;
 	while (node)
@@ -1459,31 +1458,22 @@ TargetListEntryExprFindSubscriptingRef(Expr *expr)
 		}
 		else if (IsA(node, CoerceToDomain))
 		{
-			/*
-			 * Similarly, if FieldStore nodes were expected here, then it would
-			 * be possible to see implicit coercions on front of them too, but
-			 * is not the case atm. Need to uncomment the following too if we
-			 * decide supporting such commands.
-			 *
-			 * """
-			 *   CoerceToDomain *coerceToDomain = (CoerceToDomain *) node;
-			 *   if (coerceToDomain->coercionformat != COERCE_IMPLICIT_CAST)
-			 *   {
-			 *     // not an implicit coercion, cannot reach to a SubscriptingRef
-			 *     break;
-			 *   }
-			 *   node = (Node *) coerceToDomain->arg;
-			 * """
-			 */
-			ereport(ERROR, (errmsg("unexpectedly got CoerceToDomain object when "
-								   "generating shard query")));
+			CoerceToDomain *coerceToDomain = (CoerceToDomain *) node;
+			if (coerceToDomain->coercionformat != COERCE_IMPLICIT_CAST)
+			{
+				/* not an implicit coercion, cannot reach to a SubscriptingRef */
+				break;
+			}
+
+			node = (Node *) coerceToDomain->arg;
 		}
 		else if (IsA(node, SubscriptingRef))
 		{
 			return (SubscriptingRef *) node;
 		}
 		else
 		{
+			/* got a node that we are not interested in */
 			break;
 		}
 	}

diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c
@@ -707,8 +707,8 @@ ModifyPartialQuerySupported(Query *queryTree, bool multiShardQuery,
 				/*
 				 * DELETE cannot do field indirection already, so assert here.
 				 *
-				 * NB: See TargetListEntryExprFindSubscriptingRef if you decide
-				 *     removing this error check.
+				 * NB: See TargetEntryExprFindSubsRef if you decide removing
+				 *     this error check.
 				 */
 				Assert(commandType == CMD_UPDATE || commandType == CMD_INSERT);
 				return DeferredError(ERRCODE_FEATURE_NOT_SUPPORTED,

diff --git a/src/test/regress/expected/multi_deparse_shard_query.out b/src/test/regress/expected/multi_deparse_shard_query.out
@@ -3,10 +3,6 @@
 --
 SET citus.next_shard_id TO 13100000;
 SET citus.shard_replication_factor TO 1;
-CREATE FUNCTION deparse_shard_query_test(text)
-	RETURNS VOID
-	AS 'citus'
- 	LANGUAGE C STRICT;
 -- create the first table
 CREATE TABLE raw_events_1
 	(tenant_id bigint,

diff --git a/src/test/regress/expected/multi_test_helpers.out b/src/test/regress/expected/multi_test_helpers.out
@@ -124,3 +124,7 @@ BEGIN
   END LOOP;
 END;
 $$ LANGUAGE plpgsql;
+CREATE OR REPLACE FUNCTION deparse_shard_query_test(text)
+	RETURNS VOID
+	AS 'citus'
+ 	LANGUAGE C STRICT;
diff --git a/src/test/regress/expected/subscripting_op.out b/src/test/regress/expected/subscripting_op.out
@@ -46,5 +46,75 @@ SELECT * FROM arr_subs_update ORDER BY 1,2,3,4;
   2 | {4,5,6}                      | bar      |        70
 (2 rows)
 
+CREATE DOMAIN single_int_dom AS int[] CHECK (VALUE[1] != 0);
+CREATE DOMAIN dummy_dom AS single_int_dom CHECK (VALUE[2] != 5);
+-- Citus doesn't propagate DOMAIN objects
+SELECT run_command_on_workers(
+$$
+CREATE DOMAIN subscripting_op.single_int_dom AS INT[] CHECK (VALUE[1] != 0);
+CREATE DOMAIN subscripting_op.dummy_dom AS subscripting_op.single_int_dom CHECK (VALUE[2] != 5);
+$$);
+       run_command_on_workers
+---------------------------------------------------------------------
+ (localhost,57637,t,"CREATE DOMAIN")
+ (localhost,57638,t,"CREATE DOMAIN")
+(2 rows)
+
+CREATE TABLE dummy_dom_test (id int, dummy_dom_col dummy_dom);
+SELECT create_distributed_table('dummy_dom_test', 'id');
+ create_distributed_table
+---------------------------------------------------------------------
+
+(1 row)
+
+INSERT INTO dummy_dom_test VALUES (1, '{1,2,3}'), (2, '{6,7,8}');
+UPDATE dummy_dom_test
+SET dummy_dom_col[2] = 50,
+    dummy_dom_col[1] = 60;
+SELECT * FROM dummy_dom_test ORDER BY 1,2;
+ id | dummy_dom_col
+---------------------------------------------------------------------
+  1 | {60,50,3}
+  2 | {60,50,8}
+(2 rows)
+
+CREATE TYPE two_ints as (if1 int, if2 int[]);
+CREATE DOMAIN two_ints_dom AS two_ints CHECK ((VALUE).if1 > 0);
+-- Citus doesn't propagate DOMAIN objects
+SELECT run_command_on_workers(
+$$
+CREATE DOMAIN subscripting_op.two_ints_dom AS subscripting_op.two_ints CHECK ((VALUE).if1 > 0);
+$$);
+       run_command_on_workers
+---------------------------------------------------------------------
+ (localhost,57637,t,"CREATE DOMAIN")
+ (localhost,57638,t,"CREATE DOMAIN")
+(2 rows)
+
+CREATE TABLE two_ints_dom_indirection_test (id int, two_ints_dom_col two_ints_dom);
+SELECT create_distributed_table('two_ints_dom_indirection_test', 'id');
+ create_distributed_table
+---------------------------------------------------------------------
+
+(1 row)
+
+INSERT INTO two_ints_dom_indirection_test VALUES (1, '(5, "{1,2,3}")'), (2, '(50, "{10,20,30}")');
+-- Citus planner already doesn't allow doing field indirection (e.g.:
+-- insert/update <composite type>.<field>) and we have an extra guard against
+-- that in deparser for future implementations; so here we test that by using
+-- deparse_shard_query_test() as well.
+-- i) planner would throw an error
+UPDATE two_ints_dom_indirection_test
+SET two_ints_dom_col.if2[1] = 50,
+    two_ints_dom_col.if2[3] = 60;
+ERROR:  inserting or modifying composite type fields is not supported
+-- ii) deparser would throw an error
+SELECT public.deparse_shard_query_test(
+$$
+UPDATE two_ints_dom_indirection_test
+SET two_ints_dom_col.if2[1] = 50,
+    two_ints_dom_col.if2[3] = 60;
+$$);
+ERROR:  unexpectedly got FieldStore object when generating shard query
 SET client_min_messages TO WARNING;
 DROP SCHEMA subscripting_op CASCADE;
diff --git a/src/test/regress/sql/multi_deparse_shard_query.sql b/src/test/regress/sql/multi_deparse_shard_query.sql
@@ -6,11 +6,6 @@
 SET citus.next_shard_id TO 13100000;
 SET citus.shard_replication_factor TO 1;
 
-CREATE FUNCTION deparse_shard_query_test(text)
-	RETURNS VOID
-	AS 'citus'
- 	LANGUAGE C STRICT;
-
 -- create the first table
 CREATE TABLE raw_events_1
 	(tenant_id bigint,

diff --git a/src/test/regress/sql/multi_test_helpers.sql b/src/test/regress/sql/multi_test_helpers.sql
@@ -135,3 +135,8 @@ BEGIN
 END;
 $$ LANGUAGE plpgsql;
 
+CREATE OR REPLACE FUNCTION deparse_shard_query_test(text)
+	RETURNS VOID
+	AS 'citus'
+ 	LANGUAGE C STRICT;
+
diff --git a/src/test/regress/sql/subscripting_op.sql b/src/test/regress/sql/subscripting_op.sql
@@ -40,5 +40,58 @@ DO UPDATE SET arr[0]=100, arr[1]=200, arr[5]=500;
 
 SELECT * FROM arr_subs_update ORDER BY 1,2,3,4;
 
+CREATE DOMAIN single_int_dom AS int[] CHECK (VALUE[1] != 0);
+CREATE DOMAIN dummy_dom AS single_int_dom CHECK (VALUE[2] != 5);
+
+-- Citus doesn't propagate DOMAIN objects
+SELECT run_command_on_workers(
+$$
+CREATE DOMAIN subscripting_op.single_int_dom AS INT[] CHECK (VALUE[1] != 0);
+CREATE DOMAIN subscripting_op.dummy_dom AS subscripting_op.single_int_dom CHECK (VALUE[2] != 5);
+$$);
+
+CREATE TABLE dummy_dom_test (id int, dummy_dom_col dummy_dom);
+SELECT create_distributed_table('dummy_dom_test', 'id');
+
+INSERT INTO dummy_dom_test VALUES (1, '{1,2,3}'), (2, '{6,7,8}');
+
+UPDATE dummy_dom_test
+SET dummy_dom_col[2] = 50,
+    dummy_dom_col[1] = 60;
+
+SELECT * FROM dummy_dom_test ORDER BY 1,2;
+
+CREATE TYPE two_ints as (if1 int, if2 int[]);
+CREATE DOMAIN two_ints_dom AS two_ints CHECK ((VALUE).if1 > 0);
+
+-- Citus doesn't propagate DOMAIN objects
+SELECT run_command_on_workers(
+$$
+CREATE DOMAIN subscripting_op.two_ints_dom AS subscripting_op.two_ints CHECK ((VALUE).if1 > 0);
+$$);
+
+CREATE TABLE two_ints_dom_indirection_test (id int, two_ints_dom_col two_ints_dom);
+SELECT create_distributed_table('two_ints_dom_indirection_test', 'id');
+
+INSERT INTO two_ints_dom_indirection_test VALUES (1, '(5, "{1,2,3}")'), (2, '(50, "{10,20,30}")');
+
+-- Citus planner already doesn't allow doing field indirection (e.g.:
+-- insert/update <composite type>.<field>) and we have an extra guard against
+-- that in deparser for future implementations; so here we test that by using
+-- deparse_shard_query_test() as well.
+
+-- i) planner would throw an error
+UPDATE two_ints_dom_indirection_test
+SET two_ints_dom_col.if2[1] = 50,
+    two_ints_dom_col.if2[3] = 60;
+
+-- ii) deparser would throw an error
+SELECT public.deparse_shard_query_test(
+$$
+UPDATE two_ints_dom_indirection_test
+SET two_ints_dom_col.if2[1] = 50,
+    two_ints_dom_col.if2[3] = 60;
+$$);
+
 SET client_min_messages TO WARNING;
 DROP SCHEMA subscripting_op CASCADE;