# HG changeset patch # User Alain Mazy # Date 1687871859 -7200 # Node ID a7f0f27fe33c8b6c179a3a321888246ea75d0b93 # Parent 15bfd9a76f8df57548ca081b9e898caff3eb7dba wip: advisory lock around CreateInstance: not ok see WO-139 diff -r 15bfd9a76f8d -r a7f0f27fe33c Framework/PostgreSQL/PostgreSQLDatabase.cpp --- 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)); } } diff -r 15bfd9a76f8d -r a7f0f27fe33c Framework/PostgreSQL/PostgreSQLDatabase.h --- 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(); }; diff -r 15bfd9a76f8d -r a7f0f27fe33c PostgreSQL/NEWS --- 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) ======================== diff -r 15bfd9a76f8d -r a7f0f27fe33c PostgreSQL/Plugins/PostgreSQLDefinitions.h --- 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; diff -r 15bfd9a76f8d -r a7f0f27fe33c PostgreSQL/Plugins/PostgreSQLIndex.cpp --- 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(manager.GetDatabase()); + PostgreSQLDatabase::TransientAdvisoryLock lock(db, POSTGRESQL_LOCK_CREATE_INSTANCE, 100, 1); + + statement.Execute(args); + } if (statement.IsDone() || statement.GetResultFieldsCount() != 8) diff -r 15bfd9a76f8d -r a7f0f27fe33c PostgreSQL/Plugins/PrepareIndex.sql --- 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(