From b484d9b234b276c539c53d1be76b4747ee5a74d0 Mon Sep 17 00:00:00 2001 From: Burak Velioglu Date: Thu, 18 Nov 2021 19:33:56 +0300 Subject: [PATCH 1/4] Make object locking explicit while adding dependencies --- src/backend/distributed/commands/dependencies.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/backend/distributed/commands/dependencies.c b/src/backend/distributed/commands/dependencies.c index 258f5ec51..0f7bf627b 100644 --- a/src/backend/distributed/commands/dependencies.c +++ b/src/backend/distributed/commands/dependencies.c @@ -88,6 +88,17 @@ EnsureDependenciesExistOnAllNodes(const ObjectAddress *target) */ List *workerNodeList = ActivePrimaryNonCoordinatorNodeList(RowShareLock); + /* + * Lock dependent objects explicitly to make sure same DDL command won't be sent + * multiple times from parallel sessions. Having IF EXISTS may not handle locking + * issues if sent from parallel sessions. + */ + foreach_ptr(dependency, dependenciesWithCommands) + { + LockDatabaseObject(dependency->classId, dependency->objectId, + dependency->objectSubId, RowExclusiveLock); + } + /* * right after we acquired the lock we mark our objects as distributed, these changes * will not become visible before we have successfully created all the objects on our From baeaca7bc50636b75293de85a2b400577be62a93 Mon Sep 17 00:00:00 2001 From: Burak Velioglu Date: Fri, 19 Nov 2021 10:51:56 +0300 Subject: [PATCH 2/4] Update comment --- src/backend/distributed/commands/dependencies.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/backend/distributed/commands/dependencies.c b/src/backend/distributed/commands/dependencies.c index 0f7bf627b..42dd387a2 100644 --- a/src/backend/distributed/commands/dependencies.c +++ b/src/backend/distributed/commands/dependencies.c @@ -90,8 +90,7 @@ EnsureDependenciesExistOnAllNodes(const ObjectAddress *target) /* * Lock dependent objects explicitly to make sure same DDL command won't be sent - * multiple times from parallel sessions. Having IF EXISTS may not handle locking - * issues if sent from parallel sessions. + * multiple times from parallel sessions. */ foreach_ptr(dependency, dependenciesWithCommands) { From 3a68263cc7992f1983ac92e6c96a2f43db5731b9 Mon Sep 17 00:00:00 2001 From: Burak Velioglu Date: Fri, 19 Nov 2021 12:03:17 +0300 Subject: [PATCH 3/4] Change lock type --- src/backend/distributed/commands/dependencies.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/dependencies.c b/src/backend/distributed/commands/dependencies.c index 42dd387a2..a79d6abb5 100644 --- a/src/backend/distributed/commands/dependencies.c +++ b/src/backend/distributed/commands/dependencies.c @@ -95,7 +95,7 @@ EnsureDependenciesExistOnAllNodes(const ObjectAddress *target) foreach_ptr(dependency, dependenciesWithCommands) { LockDatabaseObject(dependency->classId, dependency->objectId, - dependency->objectSubId, RowExclusiveLock); + dependency->objectSubId, ExclusiveLock); } /* From 12e05ad196b89aabb2a307b94eed642ae43a18a5 Mon Sep 17 00:00:00 2001 From: Burak Velioglu Date: Mon, 22 Nov 2021 11:43:32 +0300 Subject: [PATCH 4/4] Sorted addresses before getting lock --- .../distributed/commands/dependencies.c | 57 ++++++++++++++++++- 1 file changed, 56 insertions(+), 1 deletion(-) diff --git a/src/backend/distributed/commands/dependencies.c b/src/backend/distributed/commands/dependencies.c index a79d6abb5..861be7890 100644 --- a/src/backend/distributed/commands/dependencies.c +++ b/src/backend/distributed/commands/dependencies.c @@ -28,6 +28,7 @@ typedef bool (*AddressPredicate)(const ObjectAddress *); +static int ObjectAddressComparator(const void *a, const void *b); static List * GetDependencyCreateDDLCommands(const ObjectAddress *dependency); static List * FilterObjectAddressListByPredicate(List *objectAddressList, AddressPredicate predicate); @@ -91,8 +92,13 @@ EnsureDependenciesExistOnAllNodes(const ObjectAddress *target) /* * Lock dependent objects explicitly to make sure same DDL command won't be sent * multiple times from parallel sessions. + * + * Sort dependencies that will be created on workers to not to have any deadlock + * issue if different sessions are creating different objects. */ - foreach_ptr(dependency, dependenciesWithCommands) + List *addressSortedDependencies = SortList(dependenciesWithCommands, + ObjectAddressComparator); + foreach_ptr(dependency, addressSortedDependencies) { LockDatabaseObject(dependency->classId, dependency->objectId, dependency->objectSubId, ExclusiveLock); @@ -136,6 +142,55 @@ EnsureDependenciesExistOnAllNodes(const ObjectAddress *target) } +/* + * Copied from PG object_address_comparator function to compare ObjectAddresses. + */ +static int +ObjectAddressComparator(const void *a, const void *b) +{ + const ObjectAddress *obja = (const ObjectAddress *) a; + const ObjectAddress *objb = (const ObjectAddress *) b; + + /* + * Primary sort key is OID descending. + */ + if (obja->objectId > objb->objectId) + { + return -1; + } + if (obja->objectId < objb->objectId) + { + return 1; + } + + /* + * Next sort on catalog ID, in case identical OIDs appear in different + * catalogs. Sort direction is pretty arbitrary here. + */ + if (obja->classId < objb->classId) + { + return -1; + } + if (obja->classId > objb->classId) + { + return 1; + } + + /* + * Last, sort on object subId. + */ + if ((unsigned int) obja->objectSubId < (unsigned int) objb->objectSubId) + { + return -1; + } + if ((unsigned int) obja->objectSubId > (unsigned int) objb->objectSubId) + { + return 1; + } + return 0; +} + + /* * GetDistributableDependenciesForObject finds all the dependencies that Citus * can distribute and returns those dependencies regardless of their existency