Merge pull request #2424 from citusdata/clear_intermediate_results

Make sure not to leak intermediate result folders on the workers

src/backend/distributed/transaction/transaction_management.c

@@ -281,12 +281,32 @@ CoordinatedTransactionCallback(XactEvent event, void *arg)
 
 		case XACT_EVENT_PREPARE:
 		{
+			/*
+			 * This callback is only relevant for worker queries since
+			 * distributed queries cannot be executed with 2PC, see
+			 * XACT_EVENT_PRE_PREPARE.
+			 *
+			 * We should remove the intermediate results before unsetting the
+			 * distributed transaction id. That is necessary, otherwise Citus
+			 * would try to remove a non-existing folder and leak some of the
+			 * existing folders that are associated with distributed transaction
+			 * ids on the worker nodes.
+			 */
+			RemoveIntermediateResultsDirectory();
+
 			UnSetDistributedTransactionId();
 			break;
 		}
 
 		case XACT_EVENT_PRE_COMMIT:
 		{
+			/*
+			 * If the distributed query involves 2PC, we already removed
+			 * the intermediate result directory on XACT_EVENT_PREPARE. However,
+			 * if not, we should remove it here on the COMMIT. Since
+			 * RemoveIntermediateResultsDirectory() is idempotent, we're safe
+			 * to call it here again even if the transaction involves 2PC.
+			 */
 			RemoveIntermediateResultsDirectory();
 
 			/* nothing further to do if there's no managed remote xacts */

src/test/regress/expected/ensure_no_intermediate_data_leak.out

@@ -0,0 +1,20 @@
+------
+-- THIS TEST SHOULD IDEALLY BE EXECUTED AT THE END OF 
+-- THE REGRESSION TEST SUITE  TO MAKE SURE THAT WE 
+-- CLEAR ALL INTERMEDIATE RESULTS ON BOTH THE COORDINATOR 
+-- AND ON THE WORKERS. HOWEVER, WE HAVE SOME ISSUES AROUND
+-- WINDOWS SUPPORT, FAILURES IN TASK-TRACKER EXECUTOR
+-- SO WE DISABLE THIS TEST ON WINDOWS
+------
+SELECT pg_ls_dir('base/pgsql_job_cache') WHERE citus_version() NOT ILIKE '%windows%';
+ pg_ls_dir 
+-----------
+(0 rows)
+
+SELECT run_command_on_workers($$SELECT pg_ls_dir('base/pgsql_job_cache') WHERE citus_version() NOT ILIKE '%windows%'$$);
+ run_command_on_workers 
+------------------------
+ (localhost,57637,t,"")
+ (localhost,57638,t,"")
+(2 rows)
+

src/test/regress/multi_schedule

@@ -34,6 +34,13 @@ test: multi_create_table_constraints multi_master_protocol multi_load_data multi
 test: multi_behavioral_analytics_basics multi_behavioral_analytics_single_shard_queries multi_insert_select_non_pushable_queries multi_insert_select
 test: multi_insert_select_window multi_shard_update_delete window_functions dml_recursive recursive_dml_with_different_planners_executors
 
+# ---------
+# at the end of the regression tests regaring recursively planned modifications
+# ensure that we don't leak any intermediate results
+# This test should not run in parallel with any other tests
+# ---------
+test: ensure_no_intermediate_data_leak
+
 # ----------
 # Tests for partitioning support
 # ----------
@@ -260,3 +267,4 @@ test: multi_cache_invalidation
 # multi_task_string_size tests task string size checks
 # ---------
 test: multi_task_string_size
+

src/test/regress/sql/ensure_no_intermediate_data_leak.sql

@@ -0,0 +1,12 @@
+
+------
+-- THIS TEST SHOULD IDEALLY BE EXECUTED AT THE END OF 
+-- THE REGRESSION TEST SUITE  TO MAKE SURE THAT WE 
+-- CLEAR ALL INTERMEDIATE RESULTS ON BOTH THE COORDINATOR 
+-- AND ON THE WORKERS. HOWEVER, WE HAVE SOME ISSUES AROUND
+-- WINDOWS SUPPORT, FAILURES IN TASK-TRACKER EXECUTOR
+-- SO WE DISABLE THIS TEST ON WINDOWS
+------
+
+SELECT pg_ls_dir('base/pgsql_job_cache') WHERE citus_version() NOT ILIKE '%windows%';
+SELECT run_command_on_workers($$SELECT pg_ls_dir('base/pgsql_job_cache') WHERE citus_version() NOT ILIKE '%windows%'$$);