From 51d410bb7bffdb1be52ae4063d8362acd9c42141 Mon Sep 17 00:00:00 2001 From: Ahmet Gedemenli Date: Wed, 4 Aug 2021 12:39:48 +0300 Subject: [PATCH] Add check for alphabetically sorted gucs Move to a separate script Add the new script to readme --- .circleci/config.yml | 3 + ci/README.md | 6 + ci/check_gucs_are_alphabetically_sorted.sh | 10 ++ src/backend/distributed/shared_library_init.c | 124 +++++++++--------- 4 files changed, 81 insertions(+), 62 deletions(-) create mode 100755 ci/check_gucs_are_alphabetically_sorted.sh diff --git a/.circleci/config.yml b/.circleci/config.yml index 55eaa51c4..a3605457f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -82,6 +82,9 @@ jobs: - run: name: 'Check if all CI scripts are actually run' command: ci/check_all_ci_scripts_are_run.sh + - run: + name: 'Check if all GUCs are sorted alphabetically' + command: ci/check_gucs_are_alphabetically_sorted.sh check-sql-snapshots: docker: diff --git a/ci/README.md b/ci/README.md index 6e221b351..04375158c 100644 --- a/ci/README.md +++ b/ci/README.md @@ -358,3 +358,9 @@ This script checks and fixes issues with `.gitignore` rules: 2. Makes sure we do not commit any generated files that should be ignored. If there is an ignored file in the git tree, the user is expected to review the files that are removed from the git tree and commit them. + +## `check_gucs_are_alphabetically_sorted.sh` + +This script checks the order of the GUCs defined in `shared_library_init.c`. +To solve this failure, please check `shared_library_init.c` and make sure that the GUC +definitions are in alphabetical order. diff --git a/ci/check_gucs_are_alphabetically_sorted.sh b/ci/check_gucs_are_alphabetically_sorted.sh new file mode 100755 index 000000000..a769ae4fb --- /dev/null +++ b/ci/check_gucs_are_alphabetically_sorted.sh @@ -0,0 +1,10 @@ +#!/bin/bash + +set -euo pipefail +# shellcheck disable=SC1091 +source ci/ci_helpers.sh + +# extract citus gucs in the form of "citus.X" +grep -o -E "(\.*\"citus.\w+\")," src/backend/distributed/shared_library_init.c > gucs.out +sort -c gucs.out +rm gucs.out diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index dfcb05748..9d2f9113e 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -614,19 +614,6 @@ RegisterCitusConfigVariables(void) GUC_STANDARD, NULL, NULL, NULL); - DefineCustomRealVariable( - "citus.count_distinct_error_rate", - gettext_noop("Desired error rate when calculating count(distinct) " - "approximates using the postgresql-hll extension. " - "0.0 disables approximations for count(distinct); 1.0 " - "provides no guarantees about the accuracy of results."), - NULL, - &CountDistinctErrorRate, - 0.0, 0.0, 1.0, - PGC_USERSET, - GUC_STANDARD, - NULL, NULL, NULL); - DefineCustomIntVariable( "citus.copy_switchover_threshold", gettext_noop("Sets the threshold for copy to be switched " @@ -643,6 +630,19 @@ RegisterCitusConfigVariables(void) GUC_UNIT_BYTE | GUC_NO_SHOW_ALL, NULL, NULL, NULL); + DefineCustomRealVariable( + "citus.count_distinct_error_rate", + gettext_noop("Desired error rate when calculating count(distinct) " + "approximates using the postgresql-hll extension. " + "0.0 disables approximations for count(distinct); 1.0 " + "provides no guarantees about the accuracy of results."), + NULL, + &CountDistinctErrorRate, + 0.0, 0.0, 1.0, + PGC_USERSET, + GUC_STANDARD, + NULL, NULL, NULL); + DefineCustomBoolVariable( "citus.defer_drop_after_shard_move", gettext_noop("When enabled a shard move will mark the original shards " @@ -691,18 +691,6 @@ RegisterCitusConfigVariables(void) GUC_STANDARD, NULL, NULL, NULL); - DefineCustomBoolVariable( - "citus.enable_manual_changes_to_shards", - gettext_noop("Enables dropping and truncating known shards."), - gettext_noop("Set to false by default. If set to true, enables " - "dropping and truncating shards on the coordinator " - "(or the workers with metadata)"), - &EnableManualChangesToShards, - false, - PGC_USERSET, - GUC_NO_SHOW_ALL, - NULL, NULL, NULL); - DefineCustomRealVariable( "citus.distributed_deadlock_detection_factor", gettext_noop("Sets the time to wait before checking for distributed " @@ -716,6 +704,17 @@ RegisterCitusConfigVariables(void) GUC_STANDARD, ErrorIfNotASuitableDeadlockFactor, NULL, NULL); + DefineCustomBoolVariable( + "citus.enable_alter_database_owner", + gettext_noop("Enables propagating ALTER DATABASE ... OWNER TO ... statements to " + "workers"), + NULL, + &EnableAlterDatabaseOwner, + false, + PGC_USERSET, + GUC_NO_SHOW_ALL, + NULL, NULL, NULL); + DefineCustomBoolVariable( "citus.enable_alter_role_propagation", gettext_noop("Enables propagating ALTER ROLE statements to workers (excluding " @@ -737,17 +736,6 @@ RegisterCitusConfigVariables(void) GUC_NO_SHOW_ALL, NULL, NULL, NULL); - DefineCustomBoolVariable( - "citus.enable_alter_database_owner", - gettext_noop("Enables propagating ALTER DATABASE ... OWNER TO ... statements to " - "workers"), - NULL, - &EnableAlterDatabaseOwner, - false, - PGC_USERSET, - GUC_NO_SHOW_ALL, - NULL, NULL, NULL); - DefineCustomBoolVariable( "citus.enable_binary_protocol", gettext_noop( @@ -759,6 +747,18 @@ RegisterCitusConfigVariables(void) GUC_STANDARD, NULL, NULL, NULL); + DefineCustomBoolVariable( + "citus.enable_cost_based_connection_establishment", + gettext_noop("When enabled the connection establishment times " + "and task execution times into account for deciding " + "whether or not to establish new connections."), + NULL, + &EnableCostBasedConnectionEstablishment, + true, + PGC_USERSET, + GUC_NO_SHOW_ALL, + NULL, NULL, NULL); + DefineCustomBoolVariable( "citus.enable_create_type_propagation", gettext_noop("Enables propagating of CREATE TYPE statements to workers"), @@ -845,6 +845,18 @@ RegisterCitusConfigVariables(void) GUC_STANDARD, NULL, NULL, NULL); + DefineCustomBoolVariable( + "citus.enable_manual_changes_to_shards", + gettext_noop("Enables dropping and truncating known shards."), + gettext_noop("Set to false by default. If set to true, enables " + "dropping and truncating shards on the coordinator " + "(or the workers with metadata)"), + &EnableManualChangesToShards, + false, + PGC_USERSET, + GUC_NO_SHOW_ALL, + NULL, NULL, NULL); + DefineCustomStringVariable( "citus.enable_manual_metadata_changes_for_user", gettext_noop("Enables some helper UDFs to modify metadata " @@ -1005,18 +1017,6 @@ RegisterCitusConfigVariables(void) 0, NULL, NULL, NULL); - DefineCustomBoolVariable( - "citus.enable_cost_based_connection_establishment", - gettext_noop("When enabled the connection establishment times " - "and task execution times into account for deciding " - "whether or not to establish new connections."), - NULL, - &EnableCostBasedConnectionEstablishment, - true, - PGC_USERSET, - GUC_NO_SHOW_ALL, - NULL, NULL, NULL); - DefineCustomBoolVariable( "citus.explain_distributed_queries", gettext_noop("Enables Explain for distributed queries."), @@ -1029,20 +1029,6 @@ RegisterCitusConfigVariables(void) GUC_NO_SHOW_ALL, NULL, NULL, NULL); - DefineCustomBoolVariable( - "citus.function_opens_transaction_block", - gettext_noop("Open transaction blocks for function calls"), - gettext_noop("When enabled, Citus will always send a BEGIN to workers when " - "running distributed queres in a function. When disabled, the " - "queries may be committed immediately after the statemnent " - "completes. Disabling this flag is dangerous, it is only provided " - "for backwards compatibility with pre-8.2 behaviour."), - &FunctionOpensTransactionBlock, - true, - PGC_USERSET, - GUC_NO_SHOW_ALL, - NULL, NULL, NULL); - DefineCustomBoolVariable( "citus.force_max_query_parallelization", gettext_noop("Open as many connections as possible to maximize query " @@ -1059,6 +1045,20 @@ RegisterCitusConfigVariables(void) GUC_NO_SHOW_ALL, NULL, NULL, NULL); + DefineCustomBoolVariable( + "citus.function_opens_transaction_block", + gettext_noop("Open transaction blocks for function calls"), + gettext_noop("When enabled, Citus will always send a BEGIN to workers when " + "running distributed queres in a function. When disabled, the " + "queries may be committed immediately after the statemnent " + "completes. Disabling this flag is dangerous, it is only provided " + "for backwards compatibility with pre-8.2 behaviour."), + &FunctionOpensTransactionBlock, + true, + PGC_USERSET, + GUC_NO_SHOW_ALL, + NULL, NULL, NULL); + DefineCustomIntVariable( "citus.isolation_test_session_process_id", NULL,