# HG changeset patch # User Alain Mazy # Date 1701249858 -3600 # Node ID 3cdea26ece7399ed2390c22cf96cc8eea8f90755 # Parent a7f0f27fe33c8b6c179a3a321888246ea75d0b93# Parent d700c8f9fc24f0f0306e484cce8de0fb9d71effa merge default -> pg-transactions diff -r d700c8f9fc24 -r 3cdea26ece73 Framework/PostgreSQL/PostgreSQLDatabase.cpp --- 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)); } } diff -r d700c8f9fc24 -r 3cdea26ece73 Framework/PostgreSQL/PostgreSQLDatabase.h --- 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(); + } + }; } diff -r d700c8f9fc24 -r 3cdea26ece73 Framework/PostgreSQL/PostgreSQLParameters.cpp --- 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); + } + } diff -r d700c8f9fc24 -r 3cdea26ece73 Framework/PostgreSQL/PostgreSQLParameters.h --- 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; }; } diff -r d700c8f9fc24 -r 3cdea26ece73 Framework/PostgreSQL/PostgreSQLTransaction.cpp --- 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); diff -r d700c8f9fc24 -r 3cdea26ece73 PostgreSQL/NEWS --- 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) diff -r d700c8f9fc24 -r 3cdea26ece73 PostgreSQL/Plugins/PostgreSQLDefinitions.h --- 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; diff -r d700c8f9fc24 -r 3cdea26ece73 PostgreSQL/Plugins/PostgreSQLIndex.cpp --- 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(manager.GetDatabase()); + PostgreSQLDatabase::TransientAdvisoryLock lock(db, POSTGRESQL_LOCK_CREATE_INSTANCE, 100, 1); + + statement.Execute(args); + } if (statement.IsDone() || statement.GetResultFieldsCount() != 8) diff -r d700c8f9fc24 -r 3cdea26ece73 PostgreSQL/Plugins/PrepareIndex.sql --- 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(