Mercurial > hg > orthanc-databases

changeset 427:3cdea26ece73 pg-transactions

Find changesets by keywords (author, files, the commit message), revision number or hash, or revset expression.
merge default -> pg-transactions
author Alain Mazy <am@osimis.io>
date Wed, 29 Nov 2023 10:24:18 +0100
parents a7f0f27fe33c (diff) d700c8f9fc24 (current diff)
children 4d0bacbd0fba
files PostgreSQL/NEWS PostgreSQL/Plugins/PostgreSQLIndex.cpp
diffstat 9 files changed, 110 insertions(+), 11 deletions(-) [+]
line wrap: on
line diff
--- a/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Tue Nov 21 20:31:31 2023 +0100
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.cpp	Wed Nov 29 10:24:18 2023 +0100
@@ -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	Tue Nov 21 20:31:31 2023 +0100
+++ b/Framework/PostgreSQL/PostgreSQLDatabase.h	Wed Nov 29 10:24:18 2023 +0100
@@ -36,6 +36,7 @@
   private:
     friend class PostgreSQLStatement;
     friend class PostgreSQLLargeObject;
+    friend class PostgreSQLTransaction;
 
     class Factory;
 
@@ -91,7 +92,9 @@
 
     public:
       TransientAdvisoryLock(PostgreSQLDatabase&  database,
-                            int32_t lock);
+                            int32_t lock,
+                            unsigned int retries = 10,
+                            unsigned int retryInterval = 500);
 
       ~TransientAdvisoryLock();
     };
@@ -99,5 +102,17 @@
     static IDatabaseFactory* CreateDatabaseFactory(const PostgreSQLParameters& parameters);
 
     static PostgreSQLDatabase* CreateDatabaseConnection(const PostgreSQLParameters& parameters);
+
+  protected:
+    const std::string& GetReadWriteTransactionStatement() const
+    {
+      return parameters_.GetReadWriteTransactionStatement();
+    }
+
+    const std::string& GetReadOnlyTransactionStatement() const
+    {
+      return parameters_.GetReadOnlyTransactionStatement();
+    }
+
   };
 }
--- a/Framework/PostgreSQL/PostgreSQLParameters.cpp	Tue Nov 21 20:31:31 2023 +0100
+++ b/Framework/PostgreSQL/PostgreSQLParameters.cpp	Wed Nov 29 10:24:18 2023 +0100
@@ -42,6 +42,8 @@
     lock_ = true;
     maxConnectionRetries_ = 10;
     connectionRetryInterval_ = 5;
+    readWriteTransactionStatement_ = "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ WRITE";
+    readOnlyTransactionStatement_ = "SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY";
   }
 
 
@@ -96,6 +98,17 @@
 
     maxConnectionRetries_ = configuration.GetUnsignedIntegerValue("MaximumConnectionRetries", 10);
     connectionRetryInterval_ = configuration.GetUnsignedIntegerValue("ConnectionRetryInterval", 5);
+
+    if (configuration.LookupStringValue(s, "ReadWriteTransactionStatement"))
+    {
+      SetReadWriteTransactionStatement(s);
+    }
+
+    if (configuration.LookupStringValue(s, "ReadOnlyTransactionStatement"))
+    {
+      SetReadOnlyTransactionStatement(s);
+    }
+
   }
 
 
--- a/Framework/PostgreSQL/PostgreSQLParameters.h	Tue Nov 21 20:31:31 2023 +0100
+++ b/Framework/PostgreSQL/PostgreSQLParameters.h	Wed Nov 29 10:24:18 2023 +0100
@@ -43,6 +43,8 @@
     bool         lock_;
     unsigned int maxConnectionRetries_;
     unsigned int connectionRetryInterval_;
+    std::string  readWriteTransactionStatement_;
+    std::string  readOnlyTransactionStatement_;
 
     void Reset();
 
@@ -125,6 +127,26 @@
       return connectionRetryInterval_;
     }
 
+    void SetReadWriteTransactionStatement(const std::string& statement)
+    {
+      readWriteTransactionStatement_ = statement;
+    }
+
+    void SetReadOnlyTransactionStatement(const std::string& statement)
+    {
+      readOnlyTransactionStatement_ = statement;
+    }
+
+    const std::string& GetReadWriteTransactionStatement() const
+    {
+      return readWriteTransactionStatement_;
+    }
+
+    const std::string& GetReadOnlyTransactionStatement() const
+    {
+      return readOnlyTransactionStatement_;
+    }
+
     void Format(std::string& target) const;
   };
 }
--- a/Framework/PostgreSQL/PostgreSQLTransaction.cpp	Tue Nov 21 20:31:31 2023 +0100
+++ b/Framework/PostgreSQL/PostgreSQLTransaction.cpp	Wed Nov 29 10:24:18 2023 +0100
@@ -70,12 +70,22 @@
     switch (type)
     {
       case TransactionType_ReadWrite:
-        database_.ExecuteMultiLines("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ WRITE");
-        break;
+      {
+        const std::string& statement = database_.GetReadWriteTransactionStatement();
+        if (!statement.empty()) // if not defined, will use the default DB transaction isolation level
+        {
+          database_.ExecuteMultiLines(statement);
+        }
+       }; break;
 
       case TransactionType_ReadOnly:
-        database_.ExecuteMultiLines("SET TRANSACTION ISOLATION LEVEL SERIALIZABLE READ ONLY");
-        break;
+      {
+        const std::string& statement = database_.GetReadOnlyTransactionStatement();
+        if (!statement.empty()) // if not defined, will use the default DB transaction isolation level
+        {
+          database_.ExecuteMultiLines(statement);
+        }
+       }; break;
 
       default:
         throw Orthanc::OrthancException(Orthanc::ErrorCode_ParameterOutOfRange);
--- a/PostgreSQL/NEWS	Tue Nov 21 20:31:31 2023 +0100
+++ b/PostgreSQL/NEWS	Wed Nov 29 10:24:18 2023 +0100
@@ -1,6 +1,17 @@
 Release 5.1 (2023-06-27)
 ========================
 
+* 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.
 
 Release 5.0 (2023-04-15)
--- a/PostgreSQL/Plugins/PostgreSQLDefinitions.h	Tue Nov 21 20:31:31 2023 +0100
+++ b/PostgreSQL/Plugins/PostgreSQLDefinitions.h	Wed Nov 29 10:24:18 2023 +0100
@@ -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	Tue Nov 21 20:31:31 2023 +0100
+++ b/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Wed Nov 29 10:24:18 2023 +0100
@@ -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	Tue Nov 21 20:31:31 2023 +0100
+++ b/PostgreSQL/Plugins/PrepareIndex.sql	Wed Nov 29 10:24:18 2023 +0100
@@ -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(