From a1151c2395dab15f6803ced0d80d5b0a597175b5 Mon Sep 17 00:00:00 2001 From: Ying Xu <32597660+yxu2162@users.noreply.github.com> Date: Fri, 20 May 2022 13:47:58 -0700 Subject: [PATCH] Clear metadatacache during abort for create extension (#5907) * Bug fix for bug #5876. Memset MetadataCacheSystem every time there is an abort * Created an ObjectAccessHook that saves the transactionlevel of when citus was created and will clear metadatacache if that transaction level is rolled back. Added additional tests to make sure metadatacache is cleared --- .../distributed/metadata/metadata_cache.c | 24 +++++++ src/backend/distributed/shared_library_init.c | 35 ++++++++++ .../transaction/transaction_management.c | 43 ++++++++++++ src/include/distributed/metadata_cache.h | 2 + .../regress/expected/multi_drop_extension.out | 61 +++++++++++++++++ src/test/regress/sql/multi_drop_extension.sql | 67 +++++++++++++++++++ 6 files changed, 232 insertions(+) diff --git a/src/backend/distributed/metadata/metadata_cache.c b/src/backend/distributed/metadata/metadata_cache.c index 92d288b72..26371b45a 100644 --- a/src/backend/distributed/metadata/metadata_cache.c +++ b/src/backend/distributed/metadata/metadata_cache.c @@ -186,6 +186,9 @@ bool EnableVersionChecks = true; /* version checks are enabled */ static bool citusVersionKnownCompatible = false; +/* Variable to determine if we are in the process of creating citus */ +static int CreateCitusTransactionLevel = 0; + /* Hash table for informations about each partition */ static HTAB *DistTableCacheHash = NULL; static List *DistTableCacheExpired = NIL; @@ -1986,6 +1989,27 @@ CitusHasBeenLoadedInternal(void) } +/* + * GetCitusCreationLevel returns the level of the transaction creating citus + */ +int +GetCitusCreationLevel(void) +{ + return CreateCitusTransactionLevel; +} + + +/* + * Sets the value of CreateCitusTransactionLevel based on int received which represents the + * nesting level of the transaction that created the Citus extension + */ +void +SetCreateCitusTransactionLevel(int val) +{ + CreateCitusTransactionLevel = val; +} + + /* * CheckCitusVersion checks whether there is a version mismatch between the * available version and the loaded version or between the installed version diff --git a/src/backend/distributed/shared_library_init.c b/src/backend/distributed/shared_library_init.c index e72fd2fb3..65c12e92d 100644 --- a/src/backend/distributed/shared_library_init.c +++ b/src/backend/distributed/shared_library_init.c @@ -24,8 +24,11 @@ #include "safe_lib.h" #include "catalog/pg_authid.h" +#include "catalog/objectaccess.h" +#include "catalog/pg_extension.h" #include "citus_version.h" #include "commands/explain.h" +#include "commands/extension.h" #include "common/string.h" #include "executor/executor.h" #include "distributed/backend_data.h" @@ -141,9 +144,12 @@ static int ReplicationModel = REPLICATION_MODEL_STREAMING; /* we override the application_name assign_hook and keep a pointer to the old one */ static GucStringAssignHook OldApplicationNameAssignHook = NULL; +static object_access_hook_type PrevObjectAccessHook = NULL; void _PG_init(void); +static void CitusObjectAccessHook(ObjectAccessType access, Oid classId, Oid objectId, int + subId, void *arg); static void DoInitialCleanup(void); static void ResizeStackToMaximumDepth(void); static void multi_log_hook(ErrorData *edata); @@ -381,6 +387,9 @@ _PG_init(void) DoInitialCleanup(); } + PrevObjectAccessHook = object_access_hook; + object_access_hook = CitusObjectAccessHook; + /* ensure columnar module is loaded at the right time */ load_file(COLUMNAR_MODULE_NAME, false); @@ -2327,3 +2336,29 @@ IsSuperuser(char *roleName) return isSuperuser; } + + +/* + * CitusObjectAccessHook is called when an object is created. + * + * We currently use it to track CREATE EXTENSION citus; operations to make sure we + * clear the metadata if the transaction is rolled back. + */ +static void +CitusObjectAccessHook(ObjectAccessType access, Oid classId, Oid objectId, int subId, + void *arg) +{ + if (PrevObjectAccessHook) + { + PrevObjectAccessHook(access, classId, objectId, subId, arg); + } + + /* Checks if the access is post_create and that it's an extension id */ + if (access == OAT_POST_CREATE && classId == ExtensionRelationId) + { + /* There's currently an engine bug that makes it difficult to check + * the provided objectId with extension oid so we will set the value + * regardless if it's citus being created */ + SetCreateCitusTransactionLevel(GetCurrentTransactionNestLevel()); + } +} diff --git a/src/backend/distributed/transaction/transaction_management.c b/src/backend/distributed/transaction/transaction_management.c index f4472ba95..a46a5f198 100644 --- a/src/backend/distributed/transaction/transaction_management.c +++ b/src/backend/distributed/transaction/transaction_management.c @@ -40,6 +40,7 @@ #include "distributed/version_compat.h" #include "distributed/worker_log_messages.h" #include "distributed/commands.h" +#include "distributed/metadata_cache.h" #include "utils/hsearch.h" #include "utils/guc.h" #include "utils/memutils.h" @@ -318,6 +319,17 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) /* empty the CommitContext to ensure we're not leaking memory */ MemoryContextSwitchTo(previousContext); MemoryContextReset(CommitContext); + + /* Set CreateCitusTransactionLevel to 0 since original transaction is about to be + * committed. + */ + + if (GetCitusCreationLevel() > 0) + { + /* Check CitusCreationLevel was correctly decremented to 1 */ + Assert(GetCitusCreationLevel() == 1); + SetCreateCitusTransactionLevel(0); + } break; } @@ -362,6 +374,20 @@ CoordinatedTransactionCallback(XactEvent event, void *arg) ResetGlobalVariables(); + /* + * Clear MetadataCache table if we're aborting from a CREATE EXTENSION Citus + * so that any created OIDs from the table are cleared and invalidated. We + * also set CreateCitusTransactionLevel to 0 since that process has been aborted + */ + if (GetCitusCreationLevel() > 0) + { + /* Checks CitusCreationLevel correctly decremented to 1 */ + Assert(GetCitusCreationLevel() == 1); + + InvalidateMetadataSystemCache(); + SetCreateCitusTransactionLevel(0); + } + /* * Make sure that we give the shared connections back to the shared * pool if any. This operation is a no-op if the reserved connections @@ -609,6 +635,13 @@ CoordinatedSubTransactionCallback(SubXactEvent event, SubTransactionId subId, CoordinatedRemoteTransactionsSavepointRelease(subId); } PopSubXact(subId); + + /* Set CachedDuringCitusCreation to one level lower to represent citus creation is done */ + + if (GetCitusCreationLevel() == GetCurrentTransactionNestLevel()) + { + SetCreateCitusTransactionLevel(GetCitusCreationLevel() - 1); + } break; } @@ -631,6 +664,16 @@ CoordinatedSubTransactionCallback(SubXactEvent event, SubTransactionId subId, } PopSubXact(subId); + /* + * Clear MetadataCache table if we're aborting from a CREATE EXTENSION Citus + * so that any created OIDs from the table are cleared and invalidated. We + * also set CreateCitusTransactionLevel to 0 since subtransaction has been aborted + */ + if (GetCitusCreationLevel() == GetCurrentTransactionNestLevel()) + { + InvalidateMetadataSystemCache(); + SetCreateCitusTransactionLevel(0); + } break; } diff --git a/src/include/distributed/metadata_cache.h b/src/include/distributed/metadata_cache.h index f06924148..051a697c5 100644 --- a/src/include/distributed/metadata_cache.h +++ b/src/include/distributed/metadata_cache.h @@ -144,6 +144,8 @@ extern bool IsCitusTableType(Oid relationId, CitusTableType tableType); extern bool IsCitusTableTypeCacheEntry(CitusTableCacheEntry *tableEtnry, CitusTableType tableType); +extern void SetCreateCitusTransactionLevel(int val); +extern int GetCitusCreationLevel(void); extern bool IsCitusTable(Oid relationId); extern bool IsCitusTableViaCatalog(Oid relationId); extern char PgDistPartitionViaCatalog(Oid relationId); diff --git a/src/test/regress/expected/multi_drop_extension.out b/src/test/regress/expected/multi_drop_extension.out index 21d20ab47..2b2175367 100644 --- a/src/test/regress/expected/multi_drop_extension.out +++ b/src/test/regress/expected/multi_drop_extension.out @@ -74,6 +74,67 @@ DROP SCHEMA test_schema CASCADE; NOTICE: drop cascades to 2 other objects DROP EXTENSION citus CASCADE; \set VERBOSITY DEFAULT +-- Test if metadatacache is cleared after a rollback +BEGIN; +CREATE EXTENSION citus; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; +-- Test if metadatacache is cleared for rollback subtransations +BEGIN; +SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +ROLLBACK TO SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +COMMIT; +DROP EXTENSION citus; +-- Test if metadatacache is cleared if subtransaction commits but parent rollsback +BEGIN; +SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +RELEASE SAVEPOINT my_savepoint; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; +-- Test if metadatacache is cleared if we release a savepoint and rollback +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +RELEASE SAVEPOINT s1; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; +-- Test if metadatacache is cleared on a rollback in a nested subtransaction +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +ROLLBACK TO s1; +CREATE EXTENSION citus; +COMMIT; +DROP EXTENSION citus; +-- Test if metadatacache is cleared after columnar table is made and rollback happens +BEGIN; +SAVEPOINT s1; +CREATE EXTENSION citus; +SAVEPOINT s2; +CREATE TABLE foo1 (i int) using columnar; +SAVEPOINT s3; +ROLLBACK TO SAVEPOINT s1; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; +-- Test with a release and rollback in transactions +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +RELEASE SAVEPOINT s1; +SAVEPOINT s3; +SAVEPOINT s4; +ROLLBACK TO SAVEPOINT s3; +ROLLBACK; CREATE EXTENSION citus; -- this function is dropped in Citus10, added here for tests CREATE OR REPLACE FUNCTION pg_catalog.master_create_worker_shards(table_name text, shard_count integer, diff --git a/src/test/regress/sql/multi_drop_extension.sql b/src/test/regress/sql/multi_drop_extension.sql index 7df4a19c0..8fd1daf27 100644 --- a/src/test/regress/sql/multi_drop_extension.sql +++ b/src/test/regress/sql/multi_drop_extension.sql @@ -67,6 +67,73 @@ DROP SCHEMA test_schema CASCADE; DROP EXTENSION citus CASCADE; \set VERBOSITY DEFAULT +-- Test if metadatacache is cleared after a rollback +BEGIN; +CREATE EXTENSION citus; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; + +-- Test if metadatacache is cleared for rollback subtransations +BEGIN; +SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +ROLLBACK TO SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +COMMIT; +DROP EXTENSION citus; + +-- Test if metadatacache is cleared if subtransaction commits but parent rollsback +BEGIN; +SAVEPOINT my_savepoint; +CREATE EXTENSION citus; +RELEASE SAVEPOINT my_savepoint; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; + +-- Test if metadatacache is cleared if we release a savepoint and rollback +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +RELEASE SAVEPOINT s1; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; + +-- Test if metadatacache is cleared on a rollback in a nested subtransaction +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +ROLLBACK TO s1; +CREATE EXTENSION citus; +COMMIT; +DROP EXTENSION citus; + +-- Test if metadatacache is cleared after columnar table is made and rollback happens +BEGIN; +SAVEPOINT s1; +CREATE EXTENSION citus; +SAVEPOINT s2; +CREATE TABLE foo1 (i int) using columnar; +SAVEPOINT s3; +ROLLBACK TO SAVEPOINT s1; +ROLLBACK; +CREATE EXTENSION citus; +DROP EXTENSION citus; + +-- Test with a release and rollback in transactions +BEGIN; +SAVEPOINT s1; +SAVEPOINT s2; +CREATE EXTENSION citus; +RELEASE SAVEPOINT s1; +SAVEPOINT s3; +SAVEPOINT s4; +ROLLBACK TO SAVEPOINT s3; +ROLLBACK; CREATE EXTENSION citus; -- this function is dropped in Citus10, added here for tests