Merge pull request #4198 from citusdata/disallow-volatile-subquery-in-updates

Disallow volatile functions on single shard update subqueries

src/backend/distributed/planner/multi_router_planner.c

@@ -136,9 +136,12 @@ static DeferredErrorMessage * DeferErrorIfUnsupportedModifyQueryWithCitusLocalTa
 	RTEListProperties *rteListProperties, Oid targetRelationId);
 static DeferredErrorMessage * DeferErrorIfUnsupportedModifyQueryWithPostgresLocalTable(
 	RTEListProperties *rteListProperties, Oid targetRelationId);
-static DeferredErrorMessage * MultiShardModifyQuerySupported(Query *originalQuery,
+static DeferredErrorMessage * MultiShardUpdateDeleteSupported(Query *originalQuery,
-															 PlannerRestrictionContext *
+															  PlannerRestrictionContext *
-															 plannerRestrictionContext);
+															  plannerRestrictionContext);
+static DeferredErrorMessage * SingleShardUpdateDeleteSupported(Query *originalQuery,
+															   PlannerRestrictionContext *
+															   plannerRestrictionContext);
 static bool HasDangerousJoinUsing(List *rtableList, Node *jtnode);
 static bool MasterIrreducibleExpression(Node *expression, bool *varArgument,
 										bool *badCoalesce);
@@ -1061,10 +1064,20 @@ ModifyQuerySupported(Query *queryTree, Query *originalQuery, bool multiShardQuer
 		}
 	}
 
-	if (commandType != CMD_INSERT && multiShardQuery)
+	if (commandType != CMD_INSERT)
 	{
-		DeferredErrorMessage *errorMessage = MultiShardModifyQuerySupported(
+		DeferredErrorMessage *errorMessage = NULL;
-			originalQuery, plannerRestrictionContext);
+
+		if (multiShardQuery)
+		{
+			errorMessage = MultiShardUpdateDeleteSupported(originalQuery,
+														   plannerRestrictionContext);
+		}
+		else
+		{
+			errorMessage = SingleShardUpdateDeleteSupported(originalQuery,
+															plannerRestrictionContext);
+		}
 
 		if (errorMessage != NULL)
 		{
@@ -1217,12 +1230,12 @@ ErrorIfOnConflictNotSupported(Query *queryTree)
 
 
 /*
- * MultiShardModifyQuerySupported returns the error message if the modify query is
+ * MultiShardUpdateDeleteSupported returns the error message if the update/delete is
  * not pushdownable, otherwise it returns NULL.
  */
 static DeferredErrorMessage *
-MultiShardModifyQuerySupported(Query *originalQuery,
+MultiShardUpdateDeleteSupported(Query *originalQuery,
-							   PlannerRestrictionContext *plannerRestrictionContext)
+								PlannerRestrictionContext *plannerRestrictionContext)
 {
 	DeferredErrorMessage *errorMessage = NULL;
 	RangeTblEntry *resultRangeTable = ExtractResultRelationRTE(originalQuery);
@@ -1261,6 +1274,35 @@ MultiShardModifyQuerySupported(Query *originalQuery,
 }
 
 
+/*
+ * SingleShardUpdateDeleteSupported returns the error message if the update/delete query is
+ * not routable, otherwise it returns NULL.
+ */
+static DeferredErrorMessage *
+SingleShardUpdateDeleteSupported(Query *originalQuery,
+								 PlannerRestrictionContext *plannerRestrictionContext)
+{
+	DeferredErrorMessage *errorMessage = NULL;
+
+	/*
+	 * We currently do not support volatile functions in update/delete statements because
+	 * the function evaluation logic does not know how to distinguish volatile functions
+	 * (that need to be evaluated per row) from stable functions (that need to be evaluated per query),
+	 * and it is also not safe to push the volatile functions down on replicated tables.
+	 */
+	if (FindNodeMatchingCheckFunction((Node *) originalQuery,
+									  CitusIsVolatileFunction))
+	{
+		errorMessage = DeferredError(ERRCODE_FEATURE_NOT_SUPPORTED,
+									 "functions used in UPDATE queries on distributed "
+									 "tables must not be VOLATILE",
+									 NULL, NULL);
+	}
+
+	return errorMessage;
+}
+
+
 /*
  * HasDangerousJoinUsing search jointree for unnamed JOIN USING. Check the
  * implementation of has_dangerous_join_using in ruleutils.

src/test/regress/expected/multi_router_planner.out

@@ -2314,11 +2314,6 @@ DEBUG:  query has a single distribution column value: 1
  52
 (7 rows)
 
--- https://github.com/citusdata/citus/issues/3624
-UPDATE articles_hash SET id = id
-WHERE author_id = 1 AND title IN (SELECT name FROM authors_reference WHERE random() > 0.5);
-DEBUG:  Creating router plan
-DEBUG:  query has a single distribution column value: 1
 SET client_min_messages to 'NOTICE';
 -- test that a connection failure marks placements invalid
 SET citus.shard_replication_factor TO 2;

src/test/regress/expected/multi_shard_update_delete.out

@@ -704,6 +704,12 @@ SET    value_2 = 5 * random()
 FROM   events_test_table
 WHERE  users_test_table.user_id = events_test_table.user_id;
 ERROR:  functions used in UPDATE queries on distributed tables must not be VOLATILE
+UPDATE users_test_table
+SET    value_1 = 3
+WHERE  user_id = 1 AND value_1 IN (SELECT value_1
+                                   FROM users_test_table
+                                   WHERE user_id = 1 AND value_2 > random());
+ERROR:  functions used in UPDATE queries on distributed tables must not be VOLATILE
 -- Recursive modify planner does not take care of following test because the query
 -- is fully pushdownable, yet not allowed because it would lead to inconsistent replicas.
 UPDATE users_test_table

src/test/regress/sql/multi_router_planner.sql

@@ -1126,10 +1126,6 @@ SELECT id
 	WHERE author_id = 1
 	ORDER BY 1;
 
--- https://github.com/citusdata/citus/issues/3624
-UPDATE articles_hash SET id = id
-WHERE author_id = 1 AND title IN (SELECT name FROM authors_reference WHERE random() > 0.5);
-
 SET client_min_messages to 'NOTICE';
 
 -- test that a connection failure marks placements invalid

src/test/regress/sql/multi_shard_update_delete.sql

@@ -579,6 +579,12 @@ SET    value_2 = 5 * random()
 FROM   events_test_table
 WHERE  users_test_table.user_id = events_test_table.user_id;
 
+UPDATE users_test_table
+SET    value_1 = 3
+WHERE  user_id = 1 AND value_1 IN (SELECT value_1
+                                   FROM users_test_table
+                                   WHERE user_id = 1 AND value_2 > random());
+
 -- Recursive modify planner does not take care of following test because the query
 -- is fully pushdownable, yet not allowed because it would lead to inconsistent replicas.
 UPDATE users_test_table