changeset 418:a7f0f27fe33c pg-transactions

wip: advisory lock around CreateInstance: not ok see WO-139
author Alain Mazy <am@osimis.io>
date Tue, 27 Jun 2023 15:17:39 +0200
parents 15bfd9a76f8d
children 3cdea26ece73
files Framework/PostgreSQL/PostgreSQLDatabase.cpp Framework/PostgreSQL/PostgreSQLDatabase.h PostgreSQL/NEWS PostgreSQL/Plugins/PostgreSQLDefinitions.h PostgreSQL/Plugins/PostgreSQLIndex.cpp PostgreSQL/Plugins/PrepareIndex.sql
diffstat 6 files changed, 48 insertions(+), 14 deletions(-) [+]
line wrap: on
line diff
--- a/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Fri Jun 23 14:26:58 2023 +0200
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Tue Jun 27 15:17:39 2023 +0200
@@ -318,13 +318,15 @@
 
   PostgreSQLDatabase::TransientAdvisoryLock::TransientAdvisoryLock(
     PostgreSQLDatabase&  database,
-    int32_t lock) :
+    int32_t lock,
+    unsigned int retries,
+    unsigned int retryInterval) :
     database_(database),
     lock_(lock)
   {
     bool locked = true;
     
-    for (unsigned int i = 0; i < 10; i++)
+    for (unsigned int i = 0; i < retries; i++)
     {
       if (database_.AcquireAdvisoryLock(lock_))
       {
@@ -333,7 +335,7 @@
       }
       else
       {
-        boost::this_thread::sleep(boost::posix_time::milliseconds(500));
+        boost::this_thread::sleep(boost::posix_time::milliseconds(retryInterval));
       }
     }
 
--- a/Framework/PostgreSQL/PostgreSQLDatabase.h	Fri Jun 23 14:26:58 2023 +0200
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.h	Tue Jun 27 15:17:39 2023 +0200
@@ -92,7 +92,9 @@
 
     public:
       TransientAdvisoryLock(PostgreSQLDatabase&  database,
-                            int32_t lock);
+                            int32_t lock,
+                            unsigned int retries = 10,
+                            unsigned int retryInterval = 500);
 
       ~TransientAdvisoryLock();
     };
--- a/PostgreSQL/NEWS	Fri Jun 23 14:26:58 2023 +0200
+++ b/PostgreSQL/NEWS	Tue Jun 27 15:17:39 2023 +0200
@@ -1,6 +1,17 @@
 Pending changes in the mainline
 ===============================
 
+* Experimental debug feature:
+  Introduced 2 new configurations with these default values:
+    "ReadOnlyTransactionStatement": "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY"
+    "ReadWriteTransactionStatement": "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ WRITE"
+  You can now customize the transaction isolation level.
+  If setting these values to "", no statement is run and therefore, the default DB or server
+  isolation level is used.
+* internals:
+  - Added a UNIQUE constraint on Resources.publicId to detect DB inconsistencies
+  - Added an advisory lock around CreateInstance
+
 * Optimization of LookupResources mainly used in tools/find, C-Find and QIDO-RS.
   The optimization mainly affects find at study level.
 
@@ -13,13 +24,6 @@
 * Upgraded dependencies for static builds (notably on Windows and LSB):
   - openssl 3.1.0
 
-* Experimental debug feature:
-  Introduced 2 new configurations with these default values:
-    "ReadOnlyTransactionStatement": "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY"
-    "ReadWriteTransactionStatement": "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ WRITE"
-  You can now customize the transaction isolation level.
-  If setting these values to "", no statement is run and therefore, the default DB or server
-  isolation level is used.
 
 Release 4.0 (2021-04-22)
 ========================
--- a/PostgreSQL/Plugins/PostgreSQLDefinitions.h	Fri Jun 23 14:26:58 2023 +0200
+++ b/PostgreSQL/Plugins/PostgreSQLDefinitions.h	Tue Jun 27 15:17:39 2023 +0200
@@ -48,3 +48,10 @@
  * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
  **/
 static const int32_t POSTGRESQL_LOCK_DATABASE_SETUP = 44;
+
+/**
+ * Transient advisory lock to protect the instance creation,
+ * because it is not 100% resilient to concurrency in, e.g, READ COMIITED 
+ * transaction isolation level.
+ **/
+static const int32_t POSTGRESQL_LOCK_CREATE_INSTANCE = 45;
--- a/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Fri Jun 23 14:26:58 2023 +0200
+++ b/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Tue Jun 27 15:17:39 2023 +0200
@@ -133,7 +133,16 @@
           SetGlobalIntegerProperty(manager, MISSING_SERVER_IDENTIFIER, Orthanc::GlobalProperty_DatabasePatchLevel, revision);
         }
 
-        if (revision != 1)
+        if (revision == 1)
+        {
+          LOG(WARNING) << "PostgreSQL plugin: adding UNIQUE(publicId) constraint to the 'Resources' table ";
+          t.GetDatabaseTransaction().ExecuteMultiLines("ALTER TABLE Resources ADD UNIQUE (publicId);");
+
+          revision = 2;
+          SetGlobalIntegerProperty(manager, MISSING_SERVER_IDENTIFIER, Orthanc::GlobalProperty_DatabasePatchLevel, revision);
+        }
+
+        if (revision != 2)
         {
           LOG(ERROR) << "PostgreSQL plugin is incompatible with database schema revision: " << revision;
           throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);        
@@ -406,8 +415,17 @@
     args.SetUtf8Value("study", hashStudy);
     args.SetUtf8Value("series", hashSeries);
     args.SetUtf8Value("instance", hashInstance);
-    
-    statement.Execute(args);
+
+    { // The CreateInstance procedure is not 100% safe in highly concurrent environments when the
+      // transaction isolation is set to "READ COMMITED": (e.g, with 10 clients
+      // anonymizing studies in parallel with the "ResourceModification" config set to 8, we have observed
+      // the same series being created twice).  Therefore, we protect the whole CreateInstance procedure
+      // with an advisory lock
+      PostgreSQLDatabase& db = dynamic_cast<PostgreSQLDatabase&>(manager.GetDatabase());
+      PostgreSQLDatabase::TransientAdvisoryLock lock(db, POSTGRESQL_LOCK_CREATE_INSTANCE, 100, 1);
+
+      statement.Execute(args);
+    }
 
     if (statement.IsDone() ||
         statement.GetResultFieldsCount() != 8)
--- a/PostgreSQL/Plugins/PrepareIndex.sql	Fri Jun 23 14:26:58 2023 +0200
+++ b/PostgreSQL/Plugins/PrepareIndex.sql	Tue Jun 27 15:17:39 2023 +0200
@@ -8,6 +8,7 @@
        resourceType INTEGER NOT NULL,
        publicId VARCHAR(64) NOT NULL,
        parentId BIGINT REFERENCES Resources(internalId) ON DELETE CASCADE
+       -- UNIQUE (publicId)  -- this is made unique in C++ code (new in plugin v X.Y.Z)
        );
 
 CREATE TABLE MainDicomTags(