From 3f08055594da0e19e092954f7d0050567aafccd7 Mon Sep 17 00:00:00 2001 From: Seasoft Date: Sat, 4 Jan 2025 16:04:08 +0900 Subject: [PATCH] =?UTF-8?q?mysql=205.7=20=E3=81=A7deadlock=E3=81=8C?= =?UTF-8?q?=E7=99=BA=E7=94=9F=20#630?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - トランザクション分離レベルを意識して、無駄な SQL 実行を避ける。https://github.com/EC-CUBE/ec-cube2/issues/630#issuecomment-2561370770 - 幾らかコストが低い exists() を使う。https://github.com/EC-CUBE/ec-cube2/issues/630#issuecomment-2562061376 - SC_DB_DBFactory のテストが無さそうなので、今回追加したメソッドの他、ごく基本的な部分のみ実装する。 --- data/class/SC_Query.php | 8 +++- data/class/db/SC_DB_DBFactory.php | 27 +++++++++++ .../db/dbfactory/SC_DB_DBFactory_MYSQL.php | 13 ++++- phpunit.xml.dist | 1 - tests/class/db/SC_DB_DBFactoryTest.php | 48 +++++++++++++++++++ .../dbfactory/SC_DB_DBFactory_MYSQLTest.php | 30 ++++++++++++ .../dbfactory/SC_DB_DBFactory_PGSQLTest.php | 30 ++++++++++++ 7 files changed, 153 insertions(+), 4 deletions(-) create mode 100644 tests/class/db/SC_DB_DBFactoryTest.php create mode 100644 tests/class/db/dbfactory/SC_DB_DBFactory_MYSQLTest.php create mode 100644 tests/class/db/dbfactory/SC_DB_DBFactory_PGSQLTest.php diff --git a/data/class/SC_Query.php b/data/class/SC_Query.php index a90b2b02af..bbfe661884 100644 --- a/data/class/SC_Query.php +++ b/data/class/SC_Query.php @@ -832,8 +832,12 @@ public function getCol($col, $table = '', $where = '', $arrWhereVal = []) */ public function delete($table, $where = '', $arrWhereVal = []) { - // 空deleteは実行しない - if ($this->count($table, $where, $arrWhereVal) == 0) { + // デッドロックを生じ得る条件の場合、空 DELETE を避ける + if ( + $this->inTransaction() + && $this->dbFactory->isSkipDeleteIfNotExists() + && !$this->exists($table, $where, $arrWhereVal) + ) { return; } diff --git a/data/class/db/SC_DB_DBFactory.php b/data/class/db/SC_DB_DBFactory.php index a9f09977cb..f89f41aa54 100644 --- a/data/class/db/SC_DB_DBFactory.php +++ b/data/class/db/SC_DB_DBFactory.php @@ -30,6 +30,11 @@ */ class SC_DB_DBFactory { + public const ISOLATION_LEVEL_READ_UNCOMMITTED = 10; + public const ISOLATION_LEVEL_READ_COMMITTED = 20; + public const ISOLATION_LEVEL_REPEATABLE_READ = 30; + public const ISOLATION_LEVEL_SERIALIZABLE = 40; + /** * DB_TYPE に応じた DBFactory インスタンスを生成する. * @@ -321,4 +326,26 @@ public function alldtlSQL($where_products_class = '', $product_ids = []) return $sql; } + + /** + * トランザクション分離レベルを取得する。 + * + * @return int + */ + public function getTransactionIsolationLevel() + { + // TODO: 一般的な DBMS のデフォルトを返している。実際のレベルを返すのが望ましい。しかし、毎回 `SHOW transaction_isolation` などを実行するのは避けたい。インストーラーで実行環境のレベルを退避して設定ファイルに記録したり、設定ファイルで別のレベルを設定できるよう改善できそう。 + return static::ISOLATION_LEVEL_READ_COMMITTED; + } + + /** + * 削除時に対象行が存在しない場合に削除をスキップする必要があるかを返す。 + * + * @return bool + */ + + public function isSkipDeleteIfNotExists() + { + return false; + } } diff --git a/data/class/db/dbfactory/SC_DB_DBFactory_MYSQL.php b/data/class/db/dbfactory/SC_DB_DBFactory_MYSQL.php index 054c10d5e4..0d1cf0e091 100644 --- a/data/class/db/dbfactory/SC_DB_DBFactory_MYSQL.php +++ b/data/class/db/dbfactory/SC_DB_DBFactory_MYSQL.php @@ -389,4 +389,15 @@ public function initObjQuery(SC_Query &$objQuery) } $objQuery->exec("SET SESSION sql_mode = 'ANSI'"); } -} + + public function getTransactionIsolationLevel() + { + // TODO: デフォルトを返している。実際のレベルを返すのが望ましい。しかし、毎回 `SELECT @@session.transaction_isolation` などを実行するのは避けたい。 + return static::ISOLATION_LEVEL_REPEATABLE_READ; + } + + public function isSkipDeleteIfNotExists() + { + return $this->getTransactionIsolationLevel() >= static::ISOLATION_LEVEL_REPEATABLE_READ; + } + } diff --git a/phpunit.xml.dist b/phpunit.xml.dist index 8f15ec18ab..c7ed80921b 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -31,7 +31,6 @@ ./data/class/pages/ ./data/class/graph/ ./data/class/batch/ - ./data/class/db/dbfactory/ diff --git a/tests/class/db/SC_DB_DBFactoryTest.php b/tests/class/db/SC_DB_DBFactoryTest.php new file mode 100644 index 0000000000..05065d98b5 --- /dev/null +++ b/tests/class/db/SC_DB_DBFactoryTest.php @@ -0,0 +1,48 @@ +dbFactoryPgsql = SC_DB_DBFactory_Ex::getInstance('pgsql'); + $this->dbFactoryMysql = SC_DB_DBFactory_Ex::getInstance('mysql'); + $this->dbFactoryMysqli = SC_DB_DBFactory_Ex::getInstance('mysqli'); + } + + public function testGetInstance_pgsql() + { + $this->assertInstanceOf('SC_DB_DBFactory', $this->dbFactoryPgsql); + $this->assertInstanceOf('SC_DB_DBFactory_Ex', $this->dbFactoryPgsql); + $this->assertInstanceOf('SC_DB_DBFactory_PGSQL', $this->dbFactoryPgsql); + $this->assertInstanceOf('SC_DB_DBFactory_PGSQL_Ex', $this->dbFactoryPgsql); + } + + public function testGetInstance_mysql() + { + $this->assertInstanceOf('SC_DB_DBFactory', $this->dbFactoryMysql); + $this->assertInstanceOf('SC_DB_DBFactory_Ex', $this->dbFactoryMysql); + $this->assertInstanceOf('SC_DB_DBFactory_MYSQL', $this->dbFactoryMysql); + $this->assertInstanceOf('SC_DB_DBFactory_MYSQL_Ex', $this->dbFactoryMysql); + } + + public function testGetInstance_mysqli() + { + $this->assertSame(get_class($this->dbFactoryMysql), get_class($this->dbFactoryMysqli)); + } +} diff --git a/tests/class/db/dbfactory/SC_DB_DBFactory_MYSQLTest.php b/tests/class/db/dbfactory/SC_DB_DBFactory_MYSQLTest.php new file mode 100644 index 0000000000..06cbbb15c3 --- /dev/null +++ b/tests/class/db/dbfactory/SC_DB_DBFactory_MYSQLTest.php @@ -0,0 +1,30 @@ +dbFactory = new SC_DB_DBFactory_MYSQL_Ex; + } + + public function testGetInstance() + { + $this->assertInstanceOf('SC_DB_DBFactory_MYSQL_Ex', $this->dbFactory); + } + + public function testGetTransactionIsolationLevel() + { + $this->assertEquals(SC_DB_DBFactory_Ex::ISOLATION_LEVEL_REPEATABLE_READ, $this->dbFactory->getTransactionIsolationLevel()); + } + + public function testIsSkipDeleteIfNotExists() + { + $this->assertEquals(true, $this->dbFactory->isSkipDeleteIfNotExists()); + } +} diff --git a/tests/class/db/dbfactory/SC_DB_DBFactory_PGSQLTest.php b/tests/class/db/dbfactory/SC_DB_DBFactory_PGSQLTest.php new file mode 100644 index 0000000000..c9df88953d --- /dev/null +++ b/tests/class/db/dbfactory/SC_DB_DBFactory_PGSQLTest.php @@ -0,0 +1,30 @@ +dbFactory = new SC_DB_DBFactory_PGSQL_Ex; + } + + public function testGetInstance() + { + $this->assertInstanceOf('SC_DB_DBFactory_PGSQL_Ex', $this->dbFactory); + } + + public function testGetTransactionIsolationLevel() + { + $this->assertEquals(SC_DB_DBFactory_Ex::ISOLATION_LEVEL_READ_COMMITTED, $this->dbFactory->getTransactionIsolationLevel()); + } + + public function testIsSkipDeleteIfNotExists() + { + $this->assertEquals(false, $this->dbFactory->isSkipDeleteIfNotExists()); + } +}