From 04b374fc01dfd1dbe33031ac76fb77debbcef24f Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Tue, 16 Jan 2024 15:37:18 +0300 Subject: [PATCH 1/3] Fix upgrade tests (#7413) Adding upgrade_basic_before_non_mixed.sql file because while upgrade_basic_after_non_mixed exist, its before variation didn't exist as we don't have any "before" steps. However, run_test.py assumes that all "after" files do have a "before" variation as well. So this PR adds an empty upgrade_basic_before_non_mixed.sql file. Also, given that we don't have such a version called as 12.1devel anymore, change it to 12.1.1. And finally, let CI skip testing flakyness for upgrade tests both because it's quite hard to get flaky-test-detection job working for upgrade tests and also because in the end it is not much useful to test upgrade tests against flakyness. --- .github/workflows/build_and_test.yml | 24 +++++++++++++++---- .../before_citus_upgrade_coord_schedule | 2 +- src/test/regress/before_pg_upgrade_schedule | 2 +- src/test/regress/citus_tests/common.py | 2 +- .../upgrade_basic_before_non_mixed.out | 0 .../sql/upgrade_basic_before_non_mixed.sql | 0 6 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 src/test/regress/expected/upgrade_basic_before_non_mixed.out create mode 100644 src/test/regress/sql/upgrade_basic_before_non_mixed.sql diff --git a/.github/workflows/build_and_test.yml b/.github/workflows/build_and_test.yml index f820fae4c..16ff091e7 100644 --- a/.github/workflows/build_and_test.yml +++ b/.github/workflows/build_and_test.yml @@ -471,14 +471,30 @@ jobs: run: |- detected_changes=$(git diff origin/main... --name-only --diff-filter=AM | (grep 'src/test/regress/sql/.*\.sql\|src/test/regress/spec/.*\.spec\|src/test/regress/citus_tests/test/test_.*\.py' || true)) tests=${detected_changes} - if [ -z "$tests" ]; then - echo "No test found." + + # split the tests to be skipped --today we only skip upgrade tests + skipped_tests="" + not_skipped_tests="" + for test in $tests; do + if [[ $test =~ ^src/test/regress/sql/upgrade_ ]]; then + skipped_tests="$skipped_tests $test" + else + not_skipped_tests="$not_skipped_tests $test" + fi + done + + if [ ! -z "$skipped_tests" ]; then + echo "Skipped tests " $skipped_tests + fi + + if [ -z "$not_skipped_tests" ]; then + echo "Not detected any tests that flaky test detection should run" else - echo "Detected tests " $tests + echo "Detected tests " $not_skipped_tests fi echo 'tests<> $GITHUB_OUTPUT - echo "$tests" >> "$GITHUB_OUTPUT" + echo "$not_skipped_tests" >> "$GITHUB_OUTPUT" echo 'EOF' >> $GITHUB_OUTPUT test-flakyness: if: ${{ needs.test-flakyness-pre.outputs.tests != ''}} diff --git a/src/test/regress/before_citus_upgrade_coord_schedule b/src/test/regress/before_citus_upgrade_coord_schedule index 1195058d6..cc6afd30d 100644 --- a/src/test/regress/before_citus_upgrade_coord_schedule +++ b/src/test/regress/before_citus_upgrade_coord_schedule @@ -1,5 +1,5 @@ # this schedule is to be run on only coordinators -test: upgrade_basic_before +test: upgrade_basic_before upgrade_basic_before_non_mixed test: upgrade_pg_dist_cleanup_before test: upgrade_post_11_before diff --git a/src/test/regress/before_pg_upgrade_schedule b/src/test/regress/before_pg_upgrade_schedule index 05810d3d5..95957f8ce 100644 --- a/src/test/regress/before_pg_upgrade_schedule +++ b/src/test/regress/before_pg_upgrade_schedule @@ -1,5 +1,5 @@ # The basic tests runs analyze which depends on shard numbers -test: multi_test_helpers multi_test_helpers_superuser +test: multi_test_helpers multi_test_helpers_superuser upgrade_basic_before_non_mixed test: multi_test_catalog_views test: upgrade_basic_before test: upgrade_ref2ref_before diff --git a/src/test/regress/citus_tests/common.py b/src/test/regress/citus_tests/common.py index 99e419267..2135a0eba 100644 --- a/src/test/regress/citus_tests/common.py +++ b/src/test/regress/citus_tests/common.py @@ -92,7 +92,7 @@ PG_MAJOR_VERSION = get_pg_major_version() OLDEST_SUPPORTED_CITUS_VERSION_MATRIX = { 14: "10.2.0", 15: "11.1.5", - 16: "12.1devel", + 16: "12.1.1", } OLDEST_SUPPORTED_CITUS_VERSION = OLDEST_SUPPORTED_CITUS_VERSION_MATRIX[PG_MAJOR_VERSION] diff --git a/src/test/regress/expected/upgrade_basic_before_non_mixed.out b/src/test/regress/expected/upgrade_basic_before_non_mixed.out new file mode 100644 index 000000000..e69de29bb diff --git a/src/test/regress/sql/upgrade_basic_before_non_mixed.sql b/src/test/regress/sql/upgrade_basic_before_non_mixed.sql new file mode 100644 index 000000000..e69de29bb From 21464adfecea1f8b1604356909420303f2041bed Mon Sep 17 00:00:00 2001 From: Karina <55838532+Green-Chan@users.noreply.github.com> Date: Wed, 17 Jan 2024 16:39:07 +0300 Subject: [PATCH 2/3] Make isolation_update_node test system independent (#7423) Test isolation_update_node fails on some systems with the following error: ``` -s2: WARNING: connection to the remote node non-existent:57637 failed with the following error: could not translate host name "non-existent" to address: Name or service not known +s2: WARNING: connection to the remote node non-existent:57637 failed with the following error: could not translate host name "non-existent" to address: Temporary failure in name resolution ``` This slightly modifies an already existing [normalization rule](https://github.com/citusdata/citus/blob/739c6d26df56d38cda7f5420cccca947874d71e6/src/test/regress/bin/normalize.sed#L217-L218) to fix it. Co-authored-by: Karina Litskevich --- src/test/regress/bin/normalize.sed | 2 +- src/test/regress/expected/isolation_update_node.out | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/regress/bin/normalize.sed b/src/test/regress/bin/normalize.sed index 1d293e964..fb51bdc33 100644 --- a/src/test/regress/bin/normalize.sed +++ b/src/test/regress/bin/normalize.sed @@ -215,7 +215,7 @@ s/^(ERROR: The index name \(test_index_creation1_p2020_09_26)_([0-9])+_(tenant_ s/^(DEBUG: the index name on the shards of the partition is too long, switching to sequential and local execution mode to prevent self deadlocks: test_index_creation1_p2020_09_26)_([0-9])+_(tenant_id_timeperiod_idx)/\1_xxxxxx_\3/g # normalize errors for not being able to connect to a non-existing host -s/could not translate host name "foobar" to address: .*$/could not translate host name "foobar" to address: /g +s/could not translate host name "([A-Za-z0-9\.\-]+)" to address: .*$/could not translate host name "\1" to address: /g # ignore PL/pgSQL line numbers that differ on Mac builds s/(CONTEXT: PL\/pgSQL function .* line )([0-9]+)/\1XX/g diff --git a/src/test/regress/expected/isolation_update_node.out b/src/test/regress/expected/isolation_update_node.out index 1a1c65ec8..4b0a5f680 100644 --- a/src/test/regress/expected/isolation_update_node.out +++ b/src/test/regress/expected/isolation_update_node.out @@ -250,7 +250,7 @@ count step s1-commit-prepared: COMMIT prepared 'label'; -s2: WARNING: connection to the remote node non-existent:57637 failed with the following error: could not translate host name "non-existent" to address: Name or service not known +s2: WARNING: connection to the remote node non-existent:57637 failed with the following error: could not translate host name "non-existent" to address: step s2-execute-prepared: EXECUTE foo; From 51e607878b375c2860e2dd75f36de8cce246da50 Mon Sep 17 00:00:00 2001 From: zhjwpku Date: Wed, 17 Jan 2024 22:30:23 +0800 Subject: [PATCH 3/3] remove a duplicate forward declaration and polish some comments (#7371) remove a duplicate forward declaration and polish some comments Signed-off-by: Zhao Junwang --- .../distributed/executor/adaptive_executor.c | 10 ++++------ .../distributed/executor/multi_executor.c | 2 +- .../planner/fast_path_router_planner.c | 2 +- .../planner/multi_logical_planner.c | 6 +++--- .../planner/multi_router_planner.c | 20 +++++++++---------- src/include/distributed/distributed_planner.h | 2 +- .../distributed/multi_physical_planner.h | 6 +++--- 7 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/backend/distributed/executor/adaptive_executor.c b/src/backend/distributed/executor/adaptive_executor.c index b5566985a..1b0277f2e 100644 --- a/src/backend/distributed/executor/adaptive_executor.c +++ b/src/backend/distributed/executor/adaptive_executor.c @@ -401,7 +401,7 @@ typedef struct WorkerPool /* * Placement executions destined for worker node, but not assigned to any - * connection and not ready to start. + * connection and ready to start. */ dlist_head readyTaskQueue; int readyTaskCount; @@ -492,8 +492,6 @@ typedef struct WorkerSession } WorkerSession; -struct TaskPlacementExecution; - /* GUC, determining whether Citus opens 1 connection per task */ bool ForceMaxQueryParallelization = false; int MaxAdaptiveExecutorPoolSize = 16; @@ -585,7 +583,7 @@ typedef enum TaskPlacementExecutionState } TaskPlacementExecutionState; /* - * TaskPlacementExecution represents the an execution of a command + * TaskPlacementExecution represents the execution of a command * on a shard placement. */ typedef struct TaskPlacementExecution @@ -1908,7 +1906,7 @@ RunDistributedExecution(DistributedExecution *execution) /* * Iterate until all the tasks are finished. Once all the tasks - * are finished, ensure that that all the connection initializations + * are finished, ensure that all the connection initializations * are also finished. Otherwise, those connections are terminated * abruptly before they are established (or failed). Instead, we let * the ConnectionStateMachine() to properly handle them. @@ -3118,7 +3116,7 @@ ConnectionStateMachine(WorkerSession *session) * * We can only retry connection when the remote transaction has * not started over the connection. Otherwise, we'd have to deal - * with restoring the transaction state, which iis beyond our + * with restoring the transaction state, which is beyond our * purpose at this time. */ RemoteTransaction *transaction = &connection->remoteTransaction; diff --git a/src/backend/distributed/executor/multi_executor.c b/src/backend/distributed/executor/multi_executor.c index 45a791af4..386a278b4 100644 --- a/src/backend/distributed/executor/multi_executor.c +++ b/src/backend/distributed/executor/multi_executor.c @@ -168,7 +168,7 @@ CitusExecutorRun(QueryDesc *queryDesc, executorBoundParams = queryDesc->params; /* - * We do some potentially time consuming operations our self now before we hand of + * We do some potentially time consuming operations ourself now before we hand off * control to postgres' executor. To make sure that time spent is accurately measured * we remove the totaltime instrumentation from the queryDesc. Instead we will start * and stop the instrumentation of the total time and put it back on the queryDesc diff --git a/src/backend/distributed/planner/fast_path_router_planner.c b/src/backend/distributed/planner/fast_path_router_planner.c index 531075f9e..59f80bb40 100644 --- a/src/backend/distributed/planner/fast_path_router_planner.c +++ b/src/backend/distributed/planner/fast_path_router_planner.c @@ -252,7 +252,7 @@ FastPathRouterQuery(Query *query, Node **distributionKeyValue) /* * Distribution column must be used in a simple equality match check and it must be - * place at top level conjustion operator. In simple words, we should have + * place at top level conjunction operator. In simple words, we should have * WHERE dist_key = VALUE [AND ....]; * * We're also not allowing any other appearances of the distribution key in the quals. diff --git a/src/backend/distributed/planner/multi_logical_planner.c b/src/backend/distributed/planner/multi_logical_planner.c index f62e309f2..5201195c7 100644 --- a/src/backend/distributed/planner/multi_logical_planner.c +++ b/src/backend/distributed/planner/multi_logical_planner.c @@ -715,8 +715,8 @@ MultiNodeTree(Query *queryTree) /* - * ContainsReadIntermediateResultFunction determines whether an expresion tree contains - * a call to the read_intermediate_result function. + * ContainsReadIntermediateResultFunction determines whether an expression tree + * contains a call to the read_intermediate_result function. */ bool ContainsReadIntermediateResultFunction(Node *node) @@ -726,7 +726,7 @@ ContainsReadIntermediateResultFunction(Node *node) /* - * ContainsReadIntermediateResultArrayFunction determines whether an expresion + * ContainsReadIntermediateResultArrayFunction determines whether an expression * tree contains a call to the read_intermediate_results(result_ids, format) * function. */ diff --git a/src/backend/distributed/planner/multi_router_planner.c b/src/backend/distributed/planner/multi_router_planner.c index 620d506a0..44f955a32 100644 --- a/src/backend/distributed/planner/multi_router_planner.c +++ b/src/backend/distributed/planner/multi_router_planner.c @@ -434,7 +434,7 @@ ExtractSelectRangeTableEntry(Query *query) * for the given modification query. * * The function errors out if the input query is not a - * modify query (e.g., INSERT, UPDATE or DELETE). So, this + * modify query (e.g., INSERT, UPDATE, DELETE or MERGE). So, this * function is not expected to be called on SELECT queries. */ Oid @@ -2271,13 +2271,13 @@ SelectsFromDistributedTable(List *rangeTableList, Query *query) /* - * RouterQuery runs router pruning logic for SELECT, UPDATE, DELETE, and MERGE queries. - * If there are shards present and query is routable, all RTEs have been updated - * to point to the relevant shards in the originalQuery. Also, placementList is - * filled with the list of worker nodes that has all the required shard placements - * for the query execution. anchorShardId is set to the first pruned shardId of - * the given query. Finally, relationShardList is filled with the list of - * relation-to-shard mappings for the query. + * PlanRouterQuery runs router pruning logic for SELECT, UPDATE, DELETE, and + * MERGE queries. If there are shards present and query is routable, all RTEs + * have been updated to point to the relevant shards in the originalQuery. Also, + * placementList is filled with the list of worker nodes that has all the + * required shard placements for the query execution. anchorShardId is set to + * the first pruned shardId of the given query. Finally, relationShardList is + * filled with the list of relation-to-shard mappings for the query. * * If the given query is not routable, it fills planningError with the related * DeferredErrorMessage. The caller can check this error message to see if query @@ -2510,7 +2510,7 @@ AllShardsColocated(List *relationShardList) if (currentTableType == RANGE_DISTRIBUTED || currentTableType == APPEND_DISTRIBUTED) { - /* we do not have further strict colocation chceks */ + /* we do not have further strict colocation checks */ continue; } } @@ -2932,7 +2932,7 @@ TargetShardIntervalsForRestrictInfo(RelationRestrictionContext *restrictionConte } /* - * Different resrictions might have different partition columns. + * Different restrictions might have different partition columns. * We report partition column value if there is only one. */ if (multiplePartitionValuesExist) diff --git a/src/include/distributed/distributed_planner.h b/src/include/distributed/distributed_planner.h index d7234e4bc..23540f6f6 100644 --- a/src/include/distributed/distributed_planner.h +++ b/src/include/distributed/distributed_planner.h @@ -104,7 +104,7 @@ typedef struct FastPathRestrictionContext * Set to true when distKey = Param; in the queryTree */ bool distributionKeyHasParam; -}FastPathRestrictionContext; +} FastPathRestrictionContext; typedef struct PlannerRestrictionContext { diff --git a/src/include/distributed/multi_physical_planner.h b/src/include/distributed/multi_physical_planner.h index 60c5c9783..475a41b37 100644 --- a/src/include/distributed/multi_physical_planner.h +++ b/src/include/distributed/multi_physical_planner.h @@ -238,8 +238,8 @@ typedef struct Task TaskQuery taskQuery; /* - * A task can have multiple queries, in which case queryCount will be > 1. If - * a task has more one query, then taskQuery->queryType == TASK_QUERY_TEXT_LIST. + * A task can have multiple queries, in which case queryCount will be > 1, and + * taskQuery->queryType == TASK_QUERY_TEXT_LIST. */ int queryCount; @@ -290,7 +290,7 @@ typedef struct Task /* * When we evaluate functions and parameters in the query string then - * we should no longer send the list of parameters long with the + * we should no longer send the list of parameters along with the * query. */ bool parametersInQueryStringResolved;