changeset 136:3266785d5627

cleaning up PostgreSQL locks with constants
author Sebastien Jodogne <s.jodogne@gmail.com>
date Thu, 09 May 2019 09:52:11 +0200
parents e26690365c25
children 52b3859ee0b7
files PostgreSQL/Plugins/PostgreSQLDefinitions.h PostgreSQL/Plugins/PostgreSQLIndex.cpp PostgreSQL/Plugins/PostgreSQLStorageArea.cpp
diffstat 3 files changed, 222 insertions(+), 179 deletions(-) [+]
line wrap: on
line diff
--- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+++ b/PostgreSQL/Plugins/PostgreSQLDefinitions.h	Thu May 09 09:52:11 2019 +0200
@@ -0,0 +1,49 @@
+/**
+ * Orthanc - A Lightweight, RESTful DICOM Store
+ * Copyright (C) 2012-2016 Sebastien Jodogne, Medical Physics
+ * Department, University Hospital of Liege, Belgium
+ * Copyright (C) 2017-2019 Osimis S.A., Belgium
+ *
+ * This program is free software: you can redistribute it and/or
+ * modify it under the terms of the GNU Affero General Public License
+ * as published by the Free Software Foundation, either version 3 of
+ * the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Affero General Public License for more details.
+ * 
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ **/
+
+
+#pragma once
+
+#include <stdint.h>
+
+
+/**
+ * This advisory lock is used if the "Lock" option is set to "true",
+ * in order to prevent the execution of two PostgreSQL index plugins
+ * on the same database.
+ **/
+static const int32_t POSTGRESQL_LOCK_INDEX = 42;
+
+
+/**
+ * This advisory lock is used if the "Lock" option is set to "true",
+ * in order to prevent the execution of two PostgreSQL storage area
+ * plugins on the same database.
+ **/
+static const int32_t POSTGRESQL_LOCK_STORAGE = 43;
+
+
+/**
+ * Transient advisory lock to protect the setup of the database,
+ * because concurrent statements like "CREATE TABLE" are not protected
+ * by transactions.
+ * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
+ **/
+static const int32_t POSTGRESQL_LOCK_DATABASE_SETUP = 44;
--- a/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Wed May 08 21:09:18 2019 +0200
+++ b/PostgreSQL/Plugins/PostgreSQLIndex.cpp	Thu May 09 09:52:11 2019 +0200
@@ -24,6 +24,7 @@
 #include "../../Framework/Plugins/GlobalProperties.h"
 #include "../../Framework/PostgreSQL/PostgreSQLDatabase.h"
 #include "../../Framework/PostgreSQL/PostgreSQLTransaction.h"
+#include "PostgreSQLDefinitions.h"
 
 #include <EmbeddedResources.h>  // Auto-generated file
 
@@ -71,192 +72,188 @@
 
     if (parameters_.HasLock())
     {
-      db->AdvisoryLock(42 /* some arbitrary constant */);
-    }
-
-    /**
-     * Try and acquire a transient advisory lock to protect the setup
-     * of the database, because concurrent statements like "CREATE
-     * TABLE" are not protected by transactions.
-     * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
-     **/
-    PostgreSQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
-
-    if (clearAll_)
-    {
-      db->ClearAll();
-    }
-
-    {
-      PostgreSQLTransaction t(*db);
-
-      if (!db->DoesTableExist("Resources"))
-      {
-        std::string query;
-
-        Orthanc::EmbeddedResources::GetFileResource
-          (query, Orthanc::EmbeddedResources::POSTGRESQL_PREPARE_INDEX);
-        db->Execute(query);
-
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabaseSchemaVersion, expectedVersion);
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabasePatchLevel, 1);
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasTrigramIndex, 0);
-      }
-          
-      if (!db->DoesTableExist("Resources"))
-      {
-        LOG(ERROR) << "Corrupted PostgreSQL database";
-        throw Orthanc::OrthancException(Orthanc::ErrorCode_InternalError);        
-      }
-
-      int version = 0;
-      if (!LookupGlobalIntegerProperty(version, *db, t, Orthanc::GlobalProperty_DatabaseSchemaVersion) ||
-          version != 6)
-      {
-        LOG(ERROR) << "PostgreSQL plugin is incompatible with database schema version: " << version;
-        throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);        
-      }
-
-      int revision;
-      if (!LookupGlobalIntegerProperty(revision, *db, t, Orthanc::GlobalProperty_DatabasePatchLevel))
-      {
-        revision = 1;
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabasePatchLevel, revision);
-      }
-
-      if (revision != 1)
-      {
-        LOG(ERROR) << "PostgreSQL plugin is incompatible with database schema revision: " << revision;
-        throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);        
-      }
-
-      t.Commit();
+      db->AdvisoryLock(POSTGRESQL_LOCK_INDEX);
     }
 
     {
-      PostgreSQLTransaction t(*db);
+      PostgreSQLDatabase::TransientAdvisoryLock lock(*db, POSTGRESQL_LOCK_DATABASE_SETUP);
 
-      int hasTrigram = 0;
-      if (!LookupGlobalIntegerProperty(hasTrigram, *db, t,
-                                       Orthanc::GlobalProperty_HasTrigramIndex) ||
-          hasTrigram != 1)
+      if (clearAll_)
+      {
+        db->ClearAll();
+      }
+
       {
-        /**
-         * Apply fix for performance issue (speed up wildcard search
-         * by using GIN trigrams). This implements the patch suggested
-         * in issue #47, BUT we also keep the original
-         * "DicomIdentifiersIndexValues", as it leads to better
-         * performance for "strict" searches (i.e. searches involving
-         * no wildcard).
-         * https://www.postgresql.org/docs/current/static/pgtrgm.html
-         * https://bitbucket.org/sjodogne/orthanc/issues/47/index-improvements-for-pg-plugin
-         **/
-        try
+        PostgreSQLTransaction t(*db);
+
+        if (!db->DoesTableExist("Resources"))
+        {
+          std::string query;
+
+          Orthanc::EmbeddedResources::GetFileResource
+            (query, Orthanc::EmbeddedResources::POSTGRESQL_PREPARE_INDEX);
+          db->Execute(query);
+
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabaseSchemaVersion, expectedVersion);
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabasePatchLevel, 1);
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasTrigramIndex, 0);
+        }
+          
+        if (!db->DoesTableExist("Resources"))
         {
-          // We've observed 9 minutes on DB with 100000 studies
-          LOG(WARNING) << "Trying to enable trigram matching on the PostgreSQL database "
-                       << "to speed up wildcard searches. This may take several minutes";
+          LOG(ERROR) << "Corrupted PostgreSQL database";
+          throw Orthanc::OrthancException(Orthanc::ErrorCode_InternalError);        
+        }
 
-          db->Execute(
-            "CREATE EXTENSION IF NOT EXISTS pg_trgm; "
-            "CREATE INDEX DicomIdentifiersIndexValues2 ON DicomIdentifiers USING gin(value gin_trgm_ops);");
-
-          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasTrigramIndex, 1);
-          LOG(WARNING) << "Trigram index has been created";
+        int version = 0;
+        if (!LookupGlobalIntegerProperty(version, *db, t, Orthanc::GlobalProperty_DatabaseSchemaVersion) ||
+            version != 6)
+        {
+          LOG(ERROR) << "PostgreSQL plugin is incompatible with database schema version: " << version;
+          throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);        
+        }
 
-          t.Commit();
-        }
-        catch (Orthanc::OrthancException&)
+        int revision;
+        if (!LookupGlobalIntegerProperty(revision, *db, t, Orthanc::GlobalProperty_DatabasePatchLevel))
         {
-          LOG(WARNING) << "Performance warning: Your PostgreSQL server does "
-                       << "not support trigram matching";
-          LOG(WARNING) << "-> Consider installing the \"pg_trgm\" extension on the "
-                       << "PostgreSQL server, e.g. on Debian: sudo apt install postgresql-contrib";
+          revision = 1;
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_DatabasePatchLevel, revision);
         }
-      }
-      else
-      {
+
+        if (revision != 1)
+        {
+          LOG(ERROR) << "PostgreSQL plugin is incompatible with database schema revision: " << revision;
+          throw Orthanc::OrthancException(Orthanc::ErrorCode_Database);        
+        }
+
         t.Commit();
       }
-    }
 
-    {
-      PostgreSQLTransaction t(*db);
+      {
+        PostgreSQLTransaction t(*db);
 
-      int property = 0;
-      if (!LookupGlobalIntegerProperty(property, *db, t,
-                                       Orthanc::GlobalProperty_HasCreateInstance) ||
-          property != 2)
-      {
-        LOG(INFO) << "Installing the CreateInstance extension";
+        int hasTrigram = 0;
+        if (!LookupGlobalIntegerProperty(hasTrigram, *db, t,
+                                         Orthanc::GlobalProperty_HasTrigramIndex) ||
+            hasTrigram != 1)
+        {
+          /**
+           * Apply fix for performance issue (speed up wildcard search
+           * by using GIN trigrams). This implements the patch suggested
+           * in issue #47, BUT we also keep the original
+           * "DicomIdentifiersIndexValues", as it leads to better
+           * performance for "strict" searches (i.e. searches involving
+           * no wildcard).
+           * https://www.postgresql.org/docs/current/static/pgtrgm.html
+           * https://bitbucket.org/sjodogne/orthanc/issues/47/index-improvements-for-pg-plugin
+           **/
+          try
+          {
+            // We've observed 9 minutes on DB with 100000 studies
+            LOG(WARNING) << "Trying to enable trigram matching on the PostgreSQL database "
+                         << "to speed up wildcard searches. This may take several minutes";
+
+            db->Execute(
+              "CREATE EXTENSION IF NOT EXISTS pg_trgm; "
+              "CREATE INDEX DicomIdentifiersIndexValues2 ON DicomIdentifiers USING gin(value gin_trgm_ops);");
+
+            SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasTrigramIndex, 1);
+            LOG(WARNING) << "Trigram index has been created";
 
-        if (property == 1)
+            t.Commit();
+          }
+          catch (Orthanc::OrthancException&)
+          {
+            LOG(WARNING) << "Performance warning: Your PostgreSQL server does "
+                         << "not support trigram matching";
+            LOG(WARNING) << "-> Consider installing the \"pg_trgm\" extension on the "
+                         << "PostgreSQL server, e.g. on Debian: sudo apt install postgresql-contrib";
+          }
+        }
+        else
         {
-          // Drop older, experimental versions of this extension
-          db->Execute("DROP FUNCTION CreateInstance("
-                      "IN patient TEXT, IN study TEXT, IN series TEXT, in instance TEXT)");
+          t.Commit();
         }
+      }
+
+      {
+        PostgreSQLTransaction t(*db);
+
+        int property = 0;
+        if (!LookupGlobalIntegerProperty(property, *db, t,
+                                         Orthanc::GlobalProperty_HasCreateInstance) ||
+            property != 2)
+        {
+          LOG(INFO) << "Installing the CreateInstance extension";
+
+          if (property == 1)
+          {
+            // Drop older, experimental versions of this extension
+            db->Execute("DROP FUNCTION CreateInstance("
+                        "IN patient TEXT, IN study TEXT, IN series TEXT, in instance TEXT)");
+          }
         
-        std::string query;
-        Orthanc::EmbeddedResources::GetFileResource
-          (query, Orthanc::EmbeddedResources::POSTGRESQL_CREATE_INSTANCE);
-        db->Execute(query);
+          std::string query;
+          Orthanc::EmbeddedResources::GetFileResource
+            (query, Orthanc::EmbeddedResources::POSTGRESQL_CREATE_INSTANCE);
+          db->Execute(query);
 
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasCreateInstance, 2);
-      }
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasCreateInstance, 2);
+        }
 
       
-      if (!LookupGlobalIntegerProperty(property, *db, t,
-                                       Orthanc::GlobalProperty_GetTotalSizeIsFast) ||
-          property != 1)
-      {
-        LOG(INFO) << "Installing the FastTotalSize extension";
+        if (!LookupGlobalIntegerProperty(property, *db, t,
+                                         Orthanc::GlobalProperty_GetTotalSizeIsFast) ||
+            property != 1)
+        {
+          LOG(INFO) << "Installing the FastTotalSize extension";
 
-        std::string query;
-        Orthanc::EmbeddedResources::GetFileResource
-          (query, Orthanc::EmbeddedResources::POSTGRESQL_FAST_TOTAL_SIZE);
-        db->Execute(query);
+          std::string query;
+          Orthanc::EmbeddedResources::GetFileResource
+            (query, Orthanc::EmbeddedResources::POSTGRESQL_FAST_TOTAL_SIZE);
+          db->Execute(query);
 
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_GetTotalSizeIsFast, 1);
-      }
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_GetTotalSizeIsFast, 1);
+        }
 
 
-      // Installing this extension requires the "GlobalIntegers" table
-      // created by the "FastTotalSize" extension
-      property = 0;
-      if (!LookupGlobalIntegerProperty(property, *db, t,
-                                       Orthanc::GlobalProperty_HasFastCountResources) ||
-          property != 1)
-      {
-        LOG(INFO) << "Installing the FastCountResources extension";
+        // Installing this extension requires the "GlobalIntegers" table
+        // created by the "FastTotalSize" extension
+        property = 0;
+        if (!LookupGlobalIntegerProperty(property, *db, t,
+                                         Orthanc::GlobalProperty_HasFastCountResources) ||
+            property != 1)
+        {
+          LOG(INFO) << "Installing the FastCountResources extension";
 
-        std::string query;
-        Orthanc::EmbeddedResources::GetFileResource
-          (query, Orthanc::EmbeddedResources::POSTGRESQL_FAST_COUNT_RESOURCES);
-        db->Execute(query);
+          std::string query;
+          Orthanc::EmbeddedResources::GetFileResource
+            (query, Orthanc::EmbeddedResources::POSTGRESQL_FAST_COUNT_RESOURCES);
+          db->Execute(query);
 
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasFastCountResources, 1);
-      }
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_HasFastCountResources, 1);
+        }
 
 
-      // Installing this extension requires the "GlobalIntegers" table
-      // created by the "GetLastChangeIndex" extension
-      property = 0;
-      if (!LookupGlobalIntegerProperty(property, *db, t,
-                                       Orthanc::GlobalProperty_GetLastChangeIndex) ||
-          property != 1)
-      {
-        LOG(INFO) << "Installing the GetLastChangeIndex extension";
+        // Installing this extension requires the "GlobalIntegers" table
+        // created by the "GetLastChangeIndex" extension
+        property = 0;
+        if (!LookupGlobalIntegerProperty(property, *db, t,
+                                         Orthanc::GlobalProperty_GetLastChangeIndex) ||
+            property != 1)
+        {
+          LOG(INFO) << "Installing the GetLastChangeIndex extension";
 
-        std::string query;
-        Orthanc::EmbeddedResources::GetFileResource
-          (query, Orthanc::EmbeddedResources::POSTGRESQL_GET_LAST_CHANGE_INDEX);
-        db->Execute(query);
+          std::string query;
+          Orthanc::EmbeddedResources::GetFileResource
+            (query, Orthanc::EmbeddedResources::POSTGRESQL_GET_LAST_CHANGE_INDEX);
+          db->Execute(query);
 
-        SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_GetLastChangeIndex, 1);
+          SetGlobalIntegerProperty(*db, t, Orthanc::GlobalProperty_GetLastChangeIndex, 1);
+        }
+
+        t.Commit();
       }
-
-      t.Commit();
     }
 
     return db.release();
--- a/PostgreSQL/Plugins/PostgreSQLStorageArea.cpp	Wed May 08 21:09:18 2019 +0200
+++ b/PostgreSQL/Plugins/PostgreSQLStorageArea.cpp	Thu May 09 09:52:11 2019 +0200
@@ -20,6 +20,7 @@
 
 
 #include "PostgreSQLStorageArea.h"
+#include "PostgreSQLDefinitions.h"
 
 #include "../../Framework/PostgreSQL/PostgreSQLTransaction.h"
 
@@ -37,38 +38,34 @@
 
     if (parameters_.HasLock())
     {
-      db->AdvisoryLock(43 /* some arbitrary constant */);
-    }
-
-    /**
-     * Try and acquire a transient advisory lock to protect the setup
-     * of the database, because concurrent statements like "CREATE
-     * TABLE" are not protected by transactions.
-     * https://groups.google.com/d/msg/orthanc-users/yV3LSTh_TjI/h3PRApJFBAAJ
-     **/
-    PostgreSQLDatabase::TransientAdvisoryLock lock(*db, 44 /* some arbitrary constant */);
-
-    if (clearAll_)
-    {
-      db->ClearAll();
+      db->AdvisoryLock(POSTGRESQL_LOCK_STORAGE);
     }
 
     {
-      PostgreSQLTransaction t(*db);
+      PostgreSQLDatabase::TransientAdvisoryLock lock(*db, POSTGRESQL_LOCK_DATABASE_SETUP);
 
-      if (!db->DoesTableExist("StorageArea"))
+      if (clearAll_)
       {
-        db->Execute("CREATE TABLE IF NOT EXISTS StorageArea("
-                    "uuid VARCHAR NOT NULL PRIMARY KEY,"
-                    "content OID NOT NULL,"
-                    "type INTEGER NOT NULL)");
-
-        // Automatically remove the large objects associated with the table
-        db->Execute("CREATE OR REPLACE RULE StorageAreaDelete AS ON DELETE "
-                    "TO StorageArea DO SELECT lo_unlink(old.content);");
+        db->ClearAll();
       }
 
-      t.Commit();
+      {
+        PostgreSQLTransaction t(*db);
+
+        if (!db->DoesTableExist("StorageArea"))
+        {
+          db->Execute("CREATE TABLE IF NOT EXISTS StorageArea("
+                      "uuid VARCHAR NOT NULL PRIMARY KEY,"
+                      "content OID NOT NULL,"
+                      "type INTEGER NOT NULL)");
+
+          // Automatically remove the large objects associated with the table
+          db->Execute("CREATE OR REPLACE RULE StorageAreaDelete AS ON DELETE "
+                      "TO StorageArea DO SELECT lo_unlink(old.content);");
+        }
+
+        t.Commit();
+      }
     }
 
     return db.release();