From 9c243d447799ca0cb0245495cf86beba07aa84b6 Mon Sep 17 00:00:00 2001 From: Onur Tirtir Date: Fri, 26 Jan 2024 15:10:35 +0300 Subject: [PATCH 01/11] Improve check_gucs_are_alphabetically_sorted.sh (#7460) Apparently https://github.com/citusdata/citus/pull/7452 was not enough, need to consider the GUC-like expressions only within RegisterCitusConfigVariables function. --- ci/check_gucs_are_alphabetically_sorted.sh | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/ci/check_gucs_are_alphabetically_sorted.sh b/ci/check_gucs_are_alphabetically_sorted.sh index 3d368e708..214a5c9cf 100755 --- a/ci/check_gucs_are_alphabetically_sorted.sh +++ b/ci/check_gucs_are_alphabetically_sorted.sh @@ -4,7 +4,22 @@ set -euo pipefail # shellcheck disable=SC1091 source ci/ci_helpers.sh +# Find the line that exactly matches "RegisterCitusConfigVariables(void)" in +# shared_library_init.c. grep command returns something like +# "934:RegisterCitusConfigVariables(void)" and we extract the line number +# with cut. +RegisterCitusConfigVariables_begin_linenumber=$(grep -n "^RegisterCitusConfigVariables(void)$" src/backend/distributed/shared_library_init.c | cut -d: -f1) + +# Consider the lines starting from $RegisterCitusConfigVariables_begin_linenumber, +# grep the first line that starts with "}" and extract the line number with cut +# as in the previous step. +RegisterCitusConfigVariables_length=$(tail -n +$RegisterCitusConfigVariables_begin_linenumber src/backend/distributed/shared_library_init.c | grep -n -m 1 "^}$" | cut -d: -f1) + +# extract the function definition of RegisterCitusConfigVariables into a temp file +tail -n +$RegisterCitusConfigVariables_begin_linenumber src/backend/distributed/shared_library_init.c | head -n $(($RegisterCitusConfigVariables_length)) > RegisterCitusConfigVariables_func_def.out + # extract citus gucs in the form of "citus.X" -grep -P "^[\t][\t]\"citus\.[a-zA-Z_0-9]+\"" src/backend/distributed/shared_library_init.c > gucs.out +grep -P "^[\t][\t]\"citus\.[a-zA-Z_0-9]+\"" RegisterCitusConfigVariables_func_def.out > gucs.out sort -c gucs.out rm gucs.out +rm RegisterCitusConfigVariables_func_def.out From 58bddca01caa60a70147a09abce4966790541fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 13:16:14 +0300 Subject: [PATCH 02/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 77a5c878c..492c90ce7 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -1699,7 +1699,7 @@ IsStatementSupportedIn2PC(Node *parsetree) /* * DoesStatementRequireMarkDistributedFor2PC returns true if the statement should be marked - * as distributed in 2pc + * as distributed when executed from a non-main database. */ static bool DoesStatementRequireMarkDistributedFor2PC(Node *parsetree) From 35334617128062adbcb448d414f3cf8d25c7596b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 13:16:27 +0300 Subject: [PATCH 03/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 492c90ce7..dcd582120 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -1677,7 +1677,8 @@ RunPostprocessMainDBCommand(Node *parsetree) /* - * IsStatementSupportedIn2Pc returns true if the statement is supported in 2pc + * IsStatementSupportedIn2Pc returns true if the statement is supported from a + * non-main database. */ static bool IsStatementSupportedIn2PC(Node *parsetree) From 9632798dc530c40c84162c283d3cf3bff0f7ac37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 16:32:54 +0300 Subject: [PATCH 04/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index dcd582120..cd5eea9e4 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -96,14 +96,19 @@ "SELECT citus_internal.mark_object_distributed(%d, %s, %d, %s)" /* - * TwoPcStatementInfo is used to determine whether a statement is supported in 2PC - * and whether it should be marked as distributed in 2PC. + * NonMainDbDistributedStatementInfo is used to determine whether a statement is + * supported from non-main databases and whether it should be marked as + * distributed explicitly (*). + * + * We always have to mark such the objects created "as distributed" but while for + * some object types we can delegate this to main database, for some others we have + * to explicitly send a command to all nodes in this code-path to achieve this. */ -typedef struct TwoPcStatementInfo +typedef struct NonMainDbDistributedStatementInfo { int statementType; - bool markAsDistributed; -} TwoPcStatementInfo; + bool explicitlyMarkAsDistributed; +} NonMainDbDistributedStatementInfo; /* * twoPcSupportedStatements is a list of statements that are supported in 2PC. From 42864d706964419b57493ed07b0f11e556efc0c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 16:33:27 +0300 Subject: [PATCH 05/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index cd5eea9e4..39a0b996f 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -111,13 +111,10 @@ typedef struct NonMainDbDistributedStatementInfo } NonMainDbDistributedStatementInfo; /* - * twoPcSupportedStatements is a list of statements that are supported in 2PC. - * The list is used to determine whether a statement is supported in 2PC and - * whether it should be marked as distributed in 2PC. - * We use this array to avoid hardcoding the list of supported statements in - * multiple places. + * NonMainDbSupportedStatements is an array of statements that are supported + * from non-main databases. */ -const TwoPcStatementInfo twoPcSupportedStatements[] = { +static const NonMainDbDistributedStatementInfo NonMainDbSupportedStatements[] = { { T_GrantRoleStmt, false }, { T_CreateRoleStmt, true } }; From 2f45493366a071d0b2ef6d59347db8a2b5064198 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 16:33:42 +0300 Subject: [PATCH 06/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 39a0b996f..c020205bf 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -149,7 +149,7 @@ static bool IsDropSchemaOrDB(Node *parsetree); static bool ShouldCheckUndistributeCitusLocalTables(void); static void RunPreprocessMainDBCommand(Node *parsetree, const char *queryString); static void RunPostprocessMainDBCommand(Node *parsetree); -static bool IsStatementSupportedIn2PC(Node *parsetree); +static bool IsStatementSupportedInNonMainDb(Node *parsetree); static bool DoesStatementRequireMarkDistributedFor2PC(Node *parsetree); /* From 510e5581adf64be2c7993b215f9855194524c2e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrkan=20=C4=B0ndibay?= Date: Mon, 29 Jan 2024 16:33:49 +0300 Subject: [PATCH 07/11] Update src/backend/distributed/commands/utility_hook.c Co-authored-by: Onur Tirtir --- src/backend/distributed/commands/utility_hook.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index c020205bf..0fc2bff3b 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -150,7 +150,7 @@ static bool ShouldCheckUndistributeCitusLocalTables(void); static void RunPreprocessMainDBCommand(Node *parsetree, const char *queryString); static void RunPostprocessMainDBCommand(Node *parsetree); static bool IsStatementSupportedInNonMainDb(Node *parsetree); -static bool DoesStatementRequireMarkDistributedFor2PC(Node *parsetree); +static bool StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree); /* * ProcessUtilityParseTree is a convenience method to create a PlannedStmt out of From e2e868166c0c68da55023d85888f704fd81d11a6 Mon Sep 17 00:00:00 2001 From: gurkanindibay Date: Mon, 29 Jan 2024 16:34:37 +0300 Subject: [PATCH 08/11] Fixes naming suggestions --- .../distributed/commands/utility_hook.c | 24 +++++++++---------- 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 0fc2bff3b..435af514f 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -1630,7 +1630,7 @@ DropSchemaOrDBInProgress(void) static void RunPreprocessMainDBCommand(Node *parsetree, const char *queryString) { - if (!IsStatementSupportedIn2PC(parsetree)) + if (!IsStatementSupportedInNonMainDb(parsetree)) { return; } @@ -1656,8 +1656,8 @@ RunPreprocessMainDBCommand(Node *parsetree, const char *queryString) static void RunPostprocessMainDBCommand(Node *parsetree) { - if (!IsStatementSupportedIn2PC(parsetree) || - !DoesStatementRequireMarkDistributedFor2PC(parsetree)) + if (!IsStatementSupportedInNonMainDb(parsetree) || + !StatementRequiresMarkDistributedFromNonMainDb(parsetree)) { return; } @@ -1683,14 +1683,14 @@ RunPostprocessMainDBCommand(Node *parsetree) * non-main database. */ static bool -IsStatementSupportedIn2PC(Node *parsetree) +IsStatementSupportedInNonMainDb(Node *parsetree) { NodeTag type = nodeTag(parsetree); - for (int i = 0; i < sizeof(twoPcSupportedStatements) / - sizeof(twoPcSupportedStatements[0]); i++) + for (int i = 0; i < sizeof(NonMainDbSupportedStatements) / + sizeof(NonMainDbSupportedStatements[0]); i++) { - if (type == twoPcSupportedStatements[i].statementType) + if (type == NonMainDbSupportedStatements[i].statementType) { return true; } @@ -1705,16 +1705,16 @@ IsStatementSupportedIn2PC(Node *parsetree) * as distributed when executed from a non-main database. */ static bool -DoesStatementRequireMarkDistributedFor2PC(Node *parsetree) +StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree) { NodeTag type = nodeTag(parsetree); - for (int i = 0; i < sizeof(twoPcSupportedStatements) / - sizeof(twoPcSupportedStatements[0]); i++) + for (int i = 0; i < sizeof(NonMainDbSupportedStatements) / + sizeof(NonMainDbSupportedStatements[0]); i++) { - if (type == twoPcSupportedStatements[i].statementType) + if (type == NonMainDbSupportedStatements[i].statementType) { - return twoPcSupportedStatements[i].markAsDistributed; + return NonMainDbSupportedStatements[i].explicitlyMarkAsDistributed; } } From 754d5899f2d59dc4efe36439b63f526c1e0b4795 Mon Sep 17 00:00:00 2001 From: gurkanindibay Date: Mon, 29 Jan 2024 17:50:27 +0300 Subject: [PATCH 09/11] Refactors MarkObjectDistributed logic --- .../distributed/commands/utility_hook.c | 58 ++++++++++++++----- 1 file changed, 45 insertions(+), 13 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 435af514f..5bc6426d2 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -110,6 +110,12 @@ typedef struct NonMainDbDistributedStatementInfo bool explicitlyMarkAsDistributed; } NonMainDbDistributedStatementInfo; +typedef struct ObjectInfo +{ + char *name; + Oid id; +} ObjectInfo; + /* * NonMainDbSupportedStatements is an array of statements that are supported * from non-main databases. @@ -151,6 +157,8 @@ static void RunPreprocessMainDBCommand(Node *parsetree, const char *queryString) static void RunPostprocessMainDBCommand(Node *parsetree); static bool IsStatementSupportedInNonMainDb(Node *parsetree); static bool StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree); +static ObjectInfo GetObjectInfo(Node *parsetree); +static void MarkObjectDistributedInNonMainDb( Node *parsetree, ObjectInfo objectInfo); /* * ProcessUtilityParseTree is a convenience method to create a PlannedStmt out of @@ -1662,21 +1670,45 @@ RunPostprocessMainDBCommand(Node *parsetree) return; } - if (IsA(parsetree, CreateRoleStmt)) - { - StringInfo mainDBQuery = makeStringInfo(); - CreateRoleStmt *createRoleStmt = castNode(CreateRoleStmt, parsetree); - Oid roleOid = get_role_oid(createRoleStmt->role, false); - appendStringInfo(mainDBQuery, - MARK_OBJECT_DISTRIBUTED, - AuthIdRelationId, - quote_literal_cstr(createRoleStmt->role), - roleOid, - quote_literal_cstr(CurrentUserName())); - RunCitusMainDBQuery(mainDBQuery->data); - } + ObjectInfo objectInfo = GetObjectInfo(parsetree); + MarkObjectDistributedInNonMainDb(parsetree, objectInfo); } +/* + * GetObjectInfo returns the name and oid of the object in the given parsetree. +*/ +static ObjectInfo GetObjectInfo(Node *parsetree) +{ + ObjectInfo info; + + if (IsA(parsetree, CreateRoleStmt)) + { + CreateRoleStmt *stmt = castNode(CreateRoleStmt, parsetree); + info.name = stmt->role; + info.id = get_role_oid(stmt->role, false); + } + // Add else if branches for other statement types + + return info; +} + +/* + * MarkObjectDistributedInNonMainDb marks the given object as distributed on the + * non-main database. +*/ +static void MarkObjectDistributedInNonMainDb( Node *parsetree, ObjectInfo objectInfo) +{ + StringInfo mainDBQuery = makeStringInfo(); + appendStringInfo(mainDBQuery, + MARK_OBJECT_DISTRIBUTED, + AuthIdRelationId, + quote_literal_cstr(objectInfo.name), + objectInfo.id, + quote_literal_cstr(CurrentUserName())); + RunCitusMainDBQuery(mainDBQuery->data); +} + + /* * IsStatementSupportedIn2Pc returns true if the statement is supported from a From 71a733cdecc4d697449382d6998adcc7a35f2f05 Mon Sep 17 00:00:00 2001 From: gurkanindibay Date: Mon, 29 Jan 2024 17:50:52 +0300 Subject: [PATCH 10/11] Fixes indentation --- .../distributed/commands/utility_hook.c | 53 ++++++++++--------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 5bc6426d2..14b38e273 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -112,8 +112,8 @@ typedef struct NonMainDbDistributedStatementInfo typedef struct ObjectInfo { - char *name; - Oid id; + char *name; + Oid id; } ObjectInfo; /* @@ -158,7 +158,7 @@ static void RunPostprocessMainDBCommand(Node *parsetree); static bool IsStatementSupportedInNonMainDb(Node *parsetree); static bool StatementRequiresMarkDistributedFromNonMainDb(Node *parsetree); static ObjectInfo GetObjectInfo(Node *parsetree); -static void MarkObjectDistributedInNonMainDb( Node *parsetree, ObjectInfo objectInfo); +static void MarkObjectDistributedInNonMainDb(Node *parsetree, ObjectInfo objectInfo); /* * ProcessUtilityParseTree is a convenience method to create a PlannedStmt out of @@ -1674,42 +1674,45 @@ RunPostprocessMainDBCommand(Node *parsetree) MarkObjectDistributedInNonMainDb(parsetree, objectInfo); } + /* * GetObjectInfo returns the name and oid of the object in the given parsetree. -*/ -static ObjectInfo GetObjectInfo(Node *parsetree) + */ +static ObjectInfo +GetObjectInfo(Node *parsetree) { - ObjectInfo info; + ObjectInfo info; - if (IsA(parsetree, CreateRoleStmt)) - { - CreateRoleStmt *stmt = castNode(CreateRoleStmt, parsetree); - info.name = stmt->role; - info.id = get_role_oid(stmt->role, false); - } - // Add else if branches for other statement types + if (IsA(parsetree, CreateRoleStmt)) + { + CreateRoleStmt *stmt = castNode(CreateRoleStmt, parsetree); + info.name = stmt->role; + info.id = get_role_oid(stmt->role, false); + } + /* Add else if branches for other statement types */ - return info; + return info; } + /* * MarkObjectDistributedInNonMainDb marks the given object as distributed on the * non-main database. -*/ -static void MarkObjectDistributedInNonMainDb( Node *parsetree, ObjectInfo objectInfo) + */ +static void +MarkObjectDistributedInNonMainDb(Node *parsetree, ObjectInfo objectInfo) { - StringInfo mainDBQuery = makeStringInfo(); - appendStringInfo(mainDBQuery, - MARK_OBJECT_DISTRIBUTED, - AuthIdRelationId, - quote_literal_cstr(objectInfo.name), - objectInfo.id, - quote_literal_cstr(CurrentUserName())); - RunCitusMainDBQuery(mainDBQuery->data); + StringInfo mainDBQuery = makeStringInfo(); + appendStringInfo(mainDBQuery, + MARK_OBJECT_DISTRIBUTED, + AuthIdRelationId, + quote_literal_cstr(objectInfo.name), + objectInfo.id, + quote_literal_cstr(CurrentUserName())); + RunCitusMainDBQuery(mainDBQuery->data); } - /* * IsStatementSupportedIn2Pc returns true if the statement is supported from a * non-main database. From 4553e1ea5b2b6fb4be74f931f319f051a76c272a Mon Sep 17 00:00:00 2001 From: gurkanindibay Date: Mon, 29 Jan 2024 17:57:09 +0300 Subject: [PATCH 11/11] Fixes indentation --- src/backend/distributed/commands/utility_hook.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/backend/distributed/commands/utility_hook.c b/src/backend/distributed/commands/utility_hook.c index 14b38e273..5e7fccda0 100644 --- a/src/backend/distributed/commands/utility_hook.c +++ b/src/backend/distributed/commands/utility_hook.c @@ -1689,6 +1689,7 @@ GetObjectInfo(Node *parsetree) info.name = stmt->role; info.id = get_role_oid(stmt->role, false); } + /* Add else if branches for other statement types */ return info;