sequences: Fixing concurrent "NEXT VALUE FOR" (#143)

This fixes a bug where concurrent transactions using "NEXT VALUE FOR"
would make the sequence invisible to the other concurrent transaction.

The properties of a sequence (such as the `INCREMENT BY`, etc) are held
in the same record as the next value. Since the next value of a sequence
needs to be atomic (and separate from the transaction isolation) a
`ROLLBACK` on a transaction that contains an `ALTER SEQUENCE` will not
undo any changes.

Ideally, the properties of a `SEQUENCE` can be stored in a separate
location on disk. However, for now it's a documented limitation.

docs/alter-sequence.rst

@@ -44,6 +44,16 @@ Examples
    -- msg: ALTER SEQUENCE 1
    -- COL1: 1 COL2: 2
 
+Caveats
+-------
+
+The properties of a sequence (such as the ``INCREMENT BY``, etc) are held in the
+same record as the next value. Since the next value of a sequence needs to be
+atomic (and separate from the transaction isolation) a ``ROLLBACK`` on a
+transaction that contains an ``ALTER SEQUENCE`` will not undo any changes.
+
+This was noted in :doc:`file-format` under *Notes for Future Improvements*.
+
 See Also
 --------
 

docs/file-format.rst

@@ -207,13 +207,35 @@ A Schema Object (has the ``S`` prefix) contains a schema definition. The
 serialization does not need to be explained in detail here. You can check the
 code for ``Schema.bytes()`` and ``new_schema_from_bytes()`` respectively.
 
+Sequence Object
+---------------
+
+A Sequence Object (has the ``Q`` prefix) contains the definition and next value
+for a sequence. Since a sequence's next value needs to be atomic, this creates
+some special rules that only apply to updating or incrementing a sequence. See
+*Notes for Future Improvements* below or *Caveats* in :doc:`alter-sequence`.
+
 Table Object
 ------------
 
 A Table Object (has the ``T`` prefix) contains a table definition. The
 serialization does not need to be explained in detail here. You can check the
 code for ``Table.bytes()`` and ``new_table_from_bytes()`` respectively.
 
+Notes for Future Improvements
+-----------------------------
+
+Sequences
+^^^^^^^^^
+
+The properties of a sequence (such as the ``INCREMENT BY``, etc) are held in the
+same record as the next value. Since the next value of a sequence needs to be
+atomic (and separate from the transaction isolation) a ``ROLLBACK`` on a
+transaction that contains an ``ALTER SEQUENCE`` will not undo any changes.
+
+Ideally, the properties of a ``SEQUENCE`` can be stored in a separate location
+on disk.
+
 Notes
 -----
 

tests/next-value-for.sql

@@ -0,0 +1,128 @@
+CREATE SEQUENCE seq1;
+CREATE TABLE bar (baz INTEGER);
+INSERT INTO bar (baz) VALUES (NEXT VALUE FOR seq1);
+INSERT INTO bar (baz) VALUES (NEXT VALUE FOR seq1);
+SELECT * FROM bar;
+UPDATE bar SET baz = NEXT VALUE FOR seq1;
+SELECT * FROM bar;
+-- msg: CREATE SEQUENCE 1
+-- msg: CREATE TABLE 1
+-- msg: INSERT 1
+-- msg: INSERT 1
+-- BAZ: 1
+-- BAZ: 2
+-- msg: UPDATE 2
+-- BAZ: 3
+-- BAZ: 4
+
+/* connection 1 */
+CREATE SEQUENCE seq1;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 2 */
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 3 */
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+-- 1: msg: CREATE SEQUENCE 1
+-- 1: COL1: 1 COL2: 2
+-- 2: COL1: 3 COL2: 4
+-- 3: COL1: 5 COL2: 6
+
+/* connection 1 */
+CREATE SEQUENCE seq1;
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+COMMIT;
+/* connection 2 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+COMMIT;
+/* connection 3 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+COMMIT;
+-- 1: msg: CREATE SEQUENCE 1
+-- 1: msg: START TRANSACTION
+-- 1: COL1: 1 COL2: 2
+-- 1: msg: COMMIT
+-- 2: msg: START TRANSACTION
+-- 2: COL1: 3 COL2: 4
+-- 2: msg: COMMIT
+-- 3: msg: START TRANSACTION
+-- 3: COL1: 5 COL2: 6
+-- 3: msg: COMMIT
+
+/* connection 1 */
+CREATE SEQUENCE seq1;
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+ROLLBACK;
+/* connection 2 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+ROLLBACK;
+/* connection 3 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+ROLLBACK;
+-- 1: msg: CREATE SEQUENCE 1
+-- 1: msg: START TRANSACTION
+-- 1: COL1: 1 COL2: 2
+-- 1: msg: ROLLBACK
+-- 2: msg: START TRANSACTION
+-- 2: COL1: 3 COL2: 4
+-- 2: msg: ROLLBACK
+-- 3: msg: START TRANSACTION
+-- 3: COL1: 5 COL2: 6
+-- 3: msg: ROLLBACK
+
+/* connection 1 */
+CREATE SEQUENCE seq1;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+ROLLBACK;
+/* connection 2 */
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+START TRANSACTION;
+/* connection 1 */
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 2 */
+COMMIT;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+-- 1: msg: CREATE SEQUENCE 1
+-- 1: COL1: 1 COL2: 2
+-- 1: msg: START TRANSACTION
+-- 1: COL1: 3 COL2: 4
+-- 1: msg: ROLLBACK
+-- 2: COL1: 5 COL2: 6
+-- 2: msg: START TRANSACTION
+-- 1: COL1: 7 COL2: 8
+-- 2: msg: COMMIT
+-- 2: COL1: 9 COL2: 10
+
+/* connection 1 */
+CREATE SEQUENCE seq1;
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 2 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 3 */
+START TRANSACTION;
+VALUES NEXT VALUE FOR seq1, NEXT VALUE FOR seq1;
+/* connection 1 */
+COMMIT;
+/* connection 2 */
+COMMIT;
+/* connection 3 */
+COMMIT;
+-- 1: msg: CREATE SEQUENCE 1
+-- 1: msg: START TRANSACTION
+-- 1: COL1: 1 COL2: 2
+-- 2: msg: START TRANSACTION
+-- 2: COL1: 3 COL2: 4
+-- 3: msg: START TRANSACTION
+-- 3: COL1: 5 COL2: 6
+-- 1: msg: COMMIT
+-- 2: msg: COMMIT
+-- 3: msg: COMMIT

vsql/storage.v

@@ -210,8 +210,13 @@ fn (mut f Storage) sequence_next_value(name Identifier) !i64 {
 
 	next_sequence := sequence.next()!
 	key := 'Q${canonical_name}'.bytes()
+
+	// Important: All other objects have to use the current transaction ID when
+	// modifying an entity so that the change is not visible to other transaction.
+	// However, a sequence's number needs to be atomic and modifys the value in
+	// place.
 	old_obj := new_page_object(key, sequence.tid, 0, sequence.bytes())
-	new_obj := new_page_object(key, f.transaction_id, 0, next_sequence.bytes())
+	new_obj := new_page_object(key, sequence.tid, 0, next_sequence.bytes())
 	for page_number in f.btree.update(old_obj, new_obj, f.transaction_id)! {
 		f.transaction_pages[page_number] = true
 	}
@@ -232,8 +237,18 @@ fn (mut f Storage) update_sequence(old_sequence Sequence, new_sequence Sequence)
 	}
 
 	key := 'Q${canonical_name}'.bytes()
+
+	// Important: All other objects have to use the current transaction ID when
+	// modifying an entity so that the change is not visible to other transaction.
+	// However, a sequence's number needs to be atomic and modifys the value in
+	// place.
+	//
+	// Unfortunately, this also means that we can't guarantee the properties
+	// (INCREMENET BY, etc) AND the next value (which needs to be atomic outside
+	// of any transaction). So a ROLLBACK on a sequence will not undo such
+	// property modifications.
 	old_obj := new_page_object(key, old_sequence.tid, 0, old_sequence.bytes())
-	new_obj := new_page_object(key, f.transaction_id, 0, new_sequence.bytes())
+	new_obj := new_page_object(key, old_sequence.tid, 0, new_sequence.bytes())
 	for page_number in f.btree.update(old_obj, new_obj, f.transaction_id)! {
 		f.transaction_pages[page_number] = true
 	}