Introduce IDReservation table to reserve paper IDs

The change in 9c290ec introduced a bug: `batch/savepapers.php --dry-run` would actually insert into the Paper table! It should not, and it seems risky to have `--dry-run` go back and delete the Paper. So instead, reserve a paper ID using a separate IDReservation table.
kohler · Dec 2, 2024 · 45fb333 · 45fb333
1 parent 2cf7729
commit 45fb333
Show file tree

Hide file tree

Showing 6 changed files with 146 additions and 41 deletions.
diff --git a/src/conference.php b/src/conference.php
@@ -353,7 +353,7 @@ function __load_settings() {
 
     function load_settings() {
         $this->__load_settings();
-        if ($this->sversion < 304) {
+        if ($this->sversion < 306) {
             $old_nerrors = Dbl::$nerrors;
             while ((new UpdateSchema($this))->run()) {
                 usleep(50000);
@@ -5737,7 +5737,8 @@ private function clean_tokens() {
             }
             Dbl::free($result);
         }
-        $this->ql("delete from Capability where timeExpires>0 and timeExpires<".Conf::$now);
+        $this->ql("delete from Capability where timeExpires>0 and timeExpires<" . Conf::$now);
+        $this->ql("delete from IDReservation where timestamp<" . (Conf::$now - 60));
         $this->ql("insert into Settings set name='__capability_gc', value=? on duplicate key update value=?", Conf::$now, Conf::$now);
         $this->settings["__capability_gc"] = Conf::$now;
     }

diff --git a/src/contact.php b/src/contact.php
@@ -5935,7 +5935,7 @@ function assign_review($pid, $reviewer, $type, $extra = []) {
             if ($extra["token"] ?? null) {
                 $fields["reviewToken"] = $this->unassigned_review_token();
             }
-            $reviewId = $this->conf->id_randomizer()->insert("PaperReview", "reviewId", $fields, 5);
+            $reviewId = $this->conf->id_randomizer()->insert(DatabaseIDRandomizer::REVIEWID, $fields, 5);
             $result = Dbl_Result::make_empty();
         } else if ($type === 0) {
             $rflags = 0;

diff --git a/src/databaseidrandomizer.php b/src/databaseidrandomizer.php
@@ -6,54 +6,65 @@ class DatabaseIDRandomizer {
     /** @var Conf
      * @readonly */
     private $conf;
-    /** @var array<string,list<int>> */
+    /** @var array<int,list<int>> */
     private $ids = [];
     /** @var int */
     private $batch = 10;
+    /** @var array<int,list<int>> */
+    private $reservations = [];
+
+    const PAPERID = 0;
+    const REVIEWID = 1;
+    static private $tinfo = [
+        ["Paper", "paperId"], ["PaperReview", "reviewId"]
+    ];
 
     function __construct(Conf $conf) {
         $this->conf = $conf;
+        register_shutdown_function([$this, "cleanup"]);
     }
 
-    /** @param string $table
-     * @param string $id_col
+    /** @param 0|1 $type
      * @param int $factor
      * @return int */
-    function available_id($table, $id_col, $factor = 3) {
-        $key = "{$table}.{$id_col}";
-        while (empty($this->ids[$key])) {
+    function available_id($type, $factor = 3) {
+        list($table, $id_col) = self::$tinfo[$type];
+        while (empty($this->ids[$type])) {
             // choose a batch of IDs
             $this->batch = min(100, $this->batch * 2);
             $n = max(100, $factor * $this->conf->fetch_ivalue("select count(*) from {$table}"));
             $ids = [];
             while (count($ids) < $this->batch) {
                 $ids[] = mt_rand(1, $n);
             }
-            $ids = array_values(array_unique($ids));
+            $sorted_ids = $ids = array_values(array_unique($ids));
 
             // remove IDs that already exist
-            $result = $this->conf->qe("select {$id_col} from {$table} where {$id_col}?a", $ids);
+            sort($sorted_ids);
+            $result = $this->conf->qe("select {$id_col} from {$table} where {$id_col}?a union select id from IDReservation where type=? and id?a", $sorted_ids, $type, $sorted_ids);
             while (($row = $result->fetch_row())) {
-                array_splice($ids, array_search((int) $row[0], $ids, true), 1);
+                if (($p = array_search((int) $row[0], $ids, true)) !== false) {
+                    array_splice($ids, $p, 1);
+                }
             }
             $result->close();
 
-            $this->ids[$key] = $ids;
+            $this->ids[$type] = $ids;
         }
-        return array_pop($this->ids[$key]);
+        return array_pop($this->ids[$type]);
     }
 
-    /** @param string $table
-     * @param string $id_col
+    /** @param 0|1 $type
      * @param array<string,mixed> $fields
      * @param int $factor
      * @return int */
-    function insert($table, $id_col, $fields, $factor = 3) {
+    function insert($type, $fields, $factor = 3) {
+        list($table, $id_col) = self::$tinfo[$type];
         $random = $this->conf->setting("random_pids");
         $id = $fields[$id_col] ?? null;
         while (true) {
             if ($id === null && $random) {
-                $id = $this->available_id($table, $id_col, $factor);
+                $id = $this->available_id($type, $factor);
             }
             if ($id === null) {
                 unset($fields[$id_col]);
@@ -62,10 +73,56 @@ function insert($table, $id_col, $fields, $factor = 3) {
                 $fields[$id_col] = $id;
                 $result = $this->conf->qe("insert into {$table} (" . join(",", array_keys($fields)) . ") values ?v on duplicate key update {$id_col}={$id_col}", [array_values($fields)]);
             }
-            if ($result->affected_rows > 0) {
-                return $result->insert_id;
+            $id = $result->affected_rows > 0 ? $result->insert_id : null;
+            $result->close();
+            if ($id !== null) {
+                return $id;
             }
-            $id = null;
+        }
+    }
+
+    /** @param 0|1 $type
+     * @param int $factor
+     * @return int */
+    function reserve($type, $factor = 3) {
+        list($table, $id_col) = self::$tinfo[$type];
+        $random = $this->conf->setting("random_pids");
+        while (true) {
+            if ($random) {
+                $wantid = $this->available_id($type, $factor);
+                $result = $this->conf->qe("insert into IDReservation set type=?, id=?, timestamp=? on duplicate key update id=id",
+                    $type, $wantid, Conf::$now);
+                $id = $result->affected_rows > 0 ? $wantid : null;
+                $result->close();
+            } else {
+                $mresult = Dbl::multi_qe($this->conf->dblink,
+                    "insert into IDReservation (type,id,timestamp)
+                    select ?, greatest((select coalesce(max({$id_col}),0) from {$table}), coalesce(max(id),0))+1, ?
+                    from IDReservation where type=?;
+                    select id from IDReservation where uid=last_insert_id()",
+                    $type, Conf::$now, $type);
+                $id = null;
+                if (($result = $mresult->next())) {
+                    $ok = $result->affected_rows > 0;
+                    $result->close();
+                    if (($result = $mresult->next())) {
+                        if ($ok && ($row = $result->fetch_row())) {
+                            $id = (int) $row[0];
+                        }
+                        $result->close();
+                    }
+                }
+            }
+            if ($id !== null) {
+                $this->reservations[$type][] = $id;
+                return $id;
+            }
+        }
+    }
+
+    function cleanup() {
+        foreach ($this->reservations as $type => $ids) {
+            $this->conf->qe("delete from IDReservation where type=? and id?a", $type, $ids);
         }
     }
 }
diff --git a/src/paperstatus.php b/src/paperstatus.php
@@ -44,14 +44,16 @@ class PaperStatus extends MessageSet {
     private $_author_change_cids;
     /** @var ?list<Contact> */
     private $_created_contacts;
+    /** @var list<int> */
+    private $_dids;
     /** @var bool */
     private $_paper_submitted;
     /** @var bool */
     private $_documents_changed;
     /** @var bool */
     private $_noncontacts_changed;
-    /** @var list<int> */
-    private $_update_pid_dids;
+    /** @var bool */
+    private $_document_pids_needed;
     /** @var list<DocumentInfo> */
     private $_joindocs;
     /** @var list<array{string,float|false}> */
@@ -250,8 +252,9 @@ function upload_document($docj, PaperOption $o) {
         if ($doc->documentType <= 0) {
             $this->_joindocs[] = $doc;
         }
+        $this->_dids[] = $doc->paperStorageId;
         if ($doc->paperId === 0 || $doc->paperId === -1) {
-            $this->_update_pid_dids[] = $doc->paperStorageId;
+            $this->_document_pids_needed = true;
         } else {
             assert($doc->paperId === $this->prow->paperId);
         }
@@ -1023,15 +1026,17 @@ private function _reset(PaperInfo $prow, $pid) {
         $this->_conflict_values = [];
         $this->_conflict_ins = $this->_created_contacts = null;
         $this->_author_change_cids = null;
-        $this->_paper_submitted = $this->_documents_changed = false;
+        $this->_paper_submitted = false;
+        $this->_documents_changed = $this->_document_pids_needed = false;
         $this->_noncontacts_changed = $prow->is_new();
-        $this->_update_pid_dids = $this->_joindocs = $this->_tags_changed = [];
+        $this->_dids = $this->_joindocs = $this->_tags_changed = [];
         $this->_save_status = 0;
         if ($prow->is_new()) {
-            $pid = $this->conf->id_randomizer()->insert("Paper", "paperId", [
-                "paperId" => $pid, "title" => "", "abstract" => "", "authorInformation" => ""
-            ]);
+            $pid = $this->conf->id_randomizer()->reserve(DatabaseIDRandomizer::PAPERID);
             $this->prow->set_prop("paperId", $pid);
+            $this->prow->set_prop("title", "");
+            $this->prow->set_prop("abstract", "");
+            $this->prow->set_prop("authorInformation", "");
             foreach (Tagger::split_unpack($prow->all_tags_text()) as $tv) {
                 $this->_tags_changed[] = $tv;
             }
@@ -1221,8 +1226,9 @@ private function _normalize_and_check($pj) {
         if ($this->prow->is_new()
             && $this->_new_paper_is_empty()) {
             $this->error_at(null, $this->_("<0>Empty {submission}. Please fill out the fields and try again"));
-            $this->conf->qe("delete from Paper where paperId=?", $this->prow->paperId);
-            $this->conf->qe("update PaperStorage set paperId=-1 where paperId=?", $this->prow->paperId);
+            if (!empty($this->_dids)) {
+                $this->conf->qe("update PaperStorage set paperId=-1 where paperId=?", $this->prow->paperId);
+            }
             return false;
         }
 
@@ -1281,16 +1287,25 @@ private function _sql_prop() {
     /** @return bool */
     private function _execute_update() {
         list($qf, $qv) = $this->_sql_prop();
-        $qv[] = $this->prow->paperId;
-        $result = $this->conf->qe_apply("update Paper set " . join(", ", $qf) . " where paperId=?", $qv);
+        if ($this->prow->is_new()) {
+            $result = $this->conf->qe_apply("insert into Paper set " . join(", ", $qf) . " on duplicate key update title=title", $qv);
+        } else {
+            $qv[] = $this->prow->paperId;
+            $result = $this->conf->qe_apply("update Paper set " . join(", ", $qf) . " where paperId=?", $qv);
+        }
         if ($result->is_error()) {
             $action = $this->prow->is_new() ? "create" : "update";
             $this->error_at(null, $this->_("<0>Could not {$action} {submission}"));
             return false;
-        } else if ($result->affected_rows === 0
-                   && !$this->conf->fetch_ivalue("select exists(select * from Paper where paperId=?) from dual", $this->prow->paperId)) {
-            $this->error_at(null, $this->_("<0>{Submission} #{} has been deleted", $this->prow->paperId));
-            return false;
+        }
+        if ($result->affected_rows === 0) {
+            if ($this->prow->is_new()) {
+                $this->error_at(null, $this->_("<0>Edit conflict"));
+                return false;
+            } else if (!$this->conf->fetch_ivalue("select exists(select * from Paper where paperId=?) from dual", $this->prow->paperId)) {
+                $this->error_at(null, $this->_("<0>{Submission} #{} has been deleted", $this->prow->paperId));
+                return false;
+            }
         }
         $this->paperId = $this->prow->paperId;
         if ($this->prow->is_new()) {
@@ -1495,8 +1510,8 @@ function execute_save() {
         $this->_execute_options();
         $this->_execute_conflicts();
 
-        if (!empty($this->_update_pid_dids)) {
-            $this->conf->qe("update PaperStorage set paperId=? where paperStorageId?a", $this->paperId, $this->_update_pid_dids);
+        if ($this->_document_pids_needed) {
+            $this->conf->qe("update PaperStorage set paperId=? where paperStorageId?a", $this->paperId, $this->_dids);
         }
 
         // maybe update `papersub` settings
@@ -1539,7 +1554,7 @@ function execute_save() {
         // save new title and clear out memory
         $this->title = $this->prow->title();
         $this->prow = null;
-        $this->_update_pid_dids = $this->_joindocs = null;
+        $this->_dids = $this->_joindocs = null;
         return true;
     }
 

diff --git a/src/schema.sql b/src/schema.sql
@@ -162,6 +162,22 @@ CREATE TABLE `Formula` (
 
 
 
+--
+-- Table structure for table `IDReservation`
+--
+
+DROP TABLE IF EXISTS `IDReservation`;
+CREATE TABLE `IDReservation` (
+  `type` int(11) NOT NULL,
+  `id` int(11) NOT NULL,
+  `timestamp` bigint(11) NOT NULL,
+  `uid` int(11) NOT NULL AUTO_INCREMENT,
+  PRIMARY KEY (`type`,`id`),
+  UNIQUE KEY `uid` (`uid`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4;
+
+
+
 --
 -- Table structure for table `Invitation`
 --
@@ -627,7 +643,7 @@ CREATE TABLE `TopicInterest` (
 -- Initial settings
 -- (each setting must be on its own line for createdb.php/createdb.sh)
 insert into Settings (name, value, data) values
-  ('allowPaperOption', 304, null),   -- schema version
+  ('allowPaperOption', 306, null),   -- schema version
   ('setupPhase', 1, null),           -- initial user is chair
   ('no_papersub', 1, null),          -- no submissions yet
   ('sub_pcconf', 1, null),           -- collect PC conflicts, not collaborators

diff --git a/src/updateschema.php b/src/updateschema.php
@@ -3149,6 +3149,22 @@ function run() {
             && $conf->ql_ok("update PaperReviewHistory set rflags=rflags&~? where rflags>=? and reviewAuthorNotified=0", self::RF_AUSEEN_LIVE_v299, self::RF_AUSEEN_LIVE_v299)) {
             $conf->update_schema_version(304);
         }
+        if ($conf->sversion === 304
+            && $conf->ql_ok("CREATE TABLE `IDReservation` (
+  `type` int(11) NOT NULL,
+  `id` int(11) NOT NULL,
+  `timestamp` bigint(11) NOT NULL,
+  PRIMARY KEY (`type`,`id`)
+) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4")) {
+            $conf->update_schema_version(305);
+        }
+        if ($conf->sversion === 305
+            && $conf->ql_ok("delete from IDReservation")
+            && $conf->ql_ok("alter table IDReservation add `uid` int(11) NOT NULL")
+            && $conf->ql_ok("alter table IDReservation add unique key `uid` (`uid`)")
+            && $conf->ql_ok("alter table IDReservation change `uid` `uid` int(11) NOT NULL AUTO_INCREMENT")) {
+            $conf->update_schema_version(306);
+        }
 
         $conf->ql_ok("delete from Settings where name='__schema_lock'");
         Conf::$main = $old_conf_g;