Draft: Create a SequenceTracker for Postgres sequences. - #2986
Open
nicktobey wants to merge 1 commit into
Open
Conversation
Contributor
|
|
SummaryCoverage spans sequence naming and validation, generated identifiers, value updates, persistence across transactions, restarts and branches, boundary and cycling behavior, and concurrent writes. The results show broad happy-path and edge-case coverage, including malformed inputs and adversarial concurrent allocation scenarios. Not safe to merge yet — PR-attributable high-severity failures show that concurrent sequence use can both lose writes and issue duplicate values, creating a direct integrity risk for core database operations. Sequence renaming is also unsupported, but that finding is outside this PR and is a flag for later rather than a merge driver. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Sequence rename is not supported
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| return 0, errors.Errorf(`relation "%s" does not exist`, sequenceString) | ||
| } | ||
|
|
||
| next, err := ait.Next(ctx, sequenceString, nil) |
There was a problem hiding this comment.
Concurrent sequence allocations lose inserts
What failed: The expected result was 160 successful inserts with unique generated IDs and a sequence that remains available to subsequent sessions. In the recorded retry, 110 rows committed and at least one concurrent client failed with relation-not-found for myseq; later inserts continued at 111 through 113, but sequence metadata access intermittently returned the same relation error.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Concurrent database clients can lose inserts when generated-ID allocation intermittently fails, leaving core write workflows incomplete. Subsequent sessions may also encounter relation errors while reading sequence metadata.
- Steps to Reproduce:
- Create a sequence and a table whose generated ID uses nextval('myseq').
- Run concurrent clients that insert multiple rows using the generated default.
- Read the row count and distinct generated IDs from a fresh session, then issue additional nextval-backed inserts.
- Stub / mock content: The test used a locally configured Doltgres instance, a temporary database, sequence, and audit table; no application mocks, route interceptions, or test bypasses were applied.
- Code Analysis: server/functions/nextval.go:35-67, which was added by the PR, obtains the shared sequence collection, allocates through the SequenceTracker, and then calls collection.SetVal to mirror the allocation. In core/sequences/collection.go:438-457, SetVal mutates the cached Sequence fields, while getSequence at lines 534-555 reads and writes the shared accessedMap and writeCache at lines 557-584 iterates and clears that map without synchronization. The Sequence.mu declared at line 192 is not used by these paths. The smallest practical fix is to serialize access to the collection cache and the cached sequence state across getSequence, SetVal, and writeCache, or otherwise ensure the tracker-to-collection mirror is performed through a synchronized update path.
- Why this is likely a bug: The failure occurs in a direct local SQL workload rather than a browser-only check, and the source has a concrete unsynchronized shared-state path matching the intermittent relation errors and incomplete concurrent writes. The PR introduced the tracker-to-collection update path and the associated shared cache changes, so a targeted synchronization of those paths is the practical remediation.
Relevant code
server/functions/nextval.go:58-67
next, err := ait.Next(ctx, sequenceString, nil)
if err != nil {
return 0, err
}
err = collection.SetVal(ctx, sequenceId, next, true, true)core/sequences/collection.go:438-455
seq, err := pgs.getSequence(ctx, name)
...
seq.Current = newValue
seq.IsAtEnd = false
seq.HasBeenCalled = hasBeenCalled
if autoAdvance {
_, err := seq.nextValForSequence()core/sequences/collection.go:534-555
if seq, ok := pgs.accessedMap[name]; ok {
return seq, nil
}
...
pgs.accessedMap[seq.Id] = seq
return seq, nilcore/sequences/collection.go:557-584
for _, seq := range pgs.accessedMap {
...
}
pgs.underlyingMap = flushed
clear(pgs.accessedMap)Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Concurrent sequence allocations lose inserts**
**What failed:** The expected result was 160 successful inserts with unique generated IDs and a sequence that remains available to subsequent sessions. In the recorded retry, 110 rows committed and at least one concurrent client failed with relation-not-found for myseq; later inserts continued at 111 through 113, but sequence metadata access intermittently returned the same relation error.
- **Impact:** Concurrent database clients can lose inserts when generated-ID allocation intermittently fails, leaving core write workflows incomplete. Subsequent sessions may also encounter relation errors while reading sequence metadata.
- **Steps to reproduce:**
1. Create a sequence and a table whose generated ID uses nextval('myseq').
2. Run concurrent clients that insert multiple rows using the generated default.
3. Read the row count and distinct generated IDs from a fresh session, then issue additional nextval-backed inserts.
- **Stub / mock content:** The test used a locally configured Doltgres instance, a temporary database, sequence, and audit table; no application mocks, route interceptions, or test bypasses were applied.
- **Code analysis:** server/functions/nextval.go:35-67, which was added by the PR, obtains the shared sequence collection, allocates through the SequenceTracker, and then calls collection.SetVal to mirror the allocation. In core/sequences/collection.go:438-457, SetVal mutates the cached Sequence fields, while getSequence at lines 534-555 reads and writes the shared accessedMap and writeCache at lines 557-584 iterates and clears that map without synchronization. The Sequence.mu declared at line 192 is not used by these paths. The smallest practical fix is to serialize access to the collection cache and the cached sequence state across getSequence, SetVal, and writeCache, or otherwise ensure the tracker-to-collection mirror is performed through a synchronized update path.
- **Why this is likely a bug:** The failure occurs in a direct local SQL workload rather than a browser-only check, and the source has a concrete unsynchronized shared-state path matching the intermittent relation errors and incomplete concurrent writes. The PR introduced the tracker-to-collection update path and the associated shared cache changes, so a targeted synchronization of those paths is the practical remediation.
**Relevant code:**
`server/functions/nextval.go:58-67`
~~~go
next, err := ait.Next(ctx, sequenceString, nil)
if err != nil {
return 0, err
}
err = collection.SetVal(ctx, sequenceId, next, true, true)
~~~
`core/sequences/collection.go:438-455`
~~~go
seq, err := pgs.getSequence(ctx, name)
...
seq.Current = newValue
seq.IsAtEnd = false
seq.HasBeenCalled = hasBeenCalled
if autoAdvance {
_, err := seq.nextValForSequence()
~~~
`core/sequences/collection.go:534-555`
~~~go
if seq, ok := pgs.accessedMap[name]; ok {
return seq, nil
}
...
pgs.accessedMap[seq.Id] = seq
return seq, nil
~~~
`core/sequences/collection.go:557-584`
~~~go
for _, seq := range pgs.accessedMap {
...
}
pgs.underlyingMap = flushed
clear(pgs.accessedMap)
~~~
| // SequenceTrackerKey is the key to identify the SequenceTracker in the globalstate.GlobalState | ||
| var SequenceTrackerKey dsess.TrackerKey[*SequenceTracker] = struct{}{} | ||
|
|
||
| func (sequence SequenceState) Merge(otherSequenceState SequenceState) (merged SequenceState, ok bool) { |
There was a problem hiding this comment.
Concurrent sequence allocations reuse values
What failed: Both concurrent transactions returned 1 and committed, while subsequent allocations returned 3, 5, and 7. The expected behavior was distinct committed values, with the second transaction receiving the next value and the merged state preserving both advances.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: Concurrent committed allocations can return the same sequence value, allowing users or downstream records to receive duplicate identifiers or ordering values. The lost advance also causes later allocations to diverge from the number of committed transactions, and cannot be corrected for values already issued.
- Steps to Reproduce:
- Create an integer sequence starting at 1 with increment 2 and cache 1.
- Start two independent transactions against the same sequence and call nextval from both before either transaction commits.
- Commit the transactions in reverse order, then call nextval repeatedly after the commits.
- Compare the values returned by both transactions with subsequent allocations.
- Stub / mock content: No stubs, mocks, or bypasses were applied for this test in the recorded run.
- Code Analysis: The PR introduces SequenceState.Merge at core/sequences/collection.go:75-96. For compatible states it chooses one state's Current at lines 90-94; when concurrent transaction states represent the same base progress, the equal-state comparison in GreaterThan at lines 116-139 does not encode that two allocations occurred, so merging can retain only one advance. The new nextval path in server/functions/nextval.go:58-64 obtains a tracker value and then mirrors it into the collection, making this merge/allocation contract part of the production sequence path. The smallest practical fix is to make tracker transaction merges account for each committed allocation, or serialize allocation at the tracker boundary so concurrent committed calls cannot merge to the same returned value; preserve the furthest resulting state rather than collapsing equal concurrent states.
- Why this is likely a bug: The duplicate value was observed in a local transaction run, and the production code now merges transaction-local sequence states by selecting a single Current without representing both committed allocations. The later 3, 5, 7 progression confirms the sequence advanced only once for the two calls, which is a silent uniqueness failure rather than an environment-only error.
Relevant code
core/sequences/collection.go:75-96
func (sequence SequenceState) Merge(otherSequenceState SequenceState) (merged SequenceState, ok bool) {
newSequenceState := sequence
if sequence.Increment >= 0 && otherSequenceState.Increment >= 0 {
newSequenceState.Increment = utils.Min(sequence.Increment, otherSequenceState.Increment)
newSequenceState.Start = utils.Min(sequence.Start, otherSequenceState.Start)
}
if sequence.GreaterThan(otherSequenceState) {
newSequenceState.Current = sequence.Current
} else {
newSequenceState.Current = otherSequenceState.Current
}
return newSequenceState, true
}core/sequences/collection.go:116-139
func (sequence SequenceState) GreaterThan(other SequenceState) bool {
if sequence.Increment > 0 {
hasWrapped := sequence.Current < sequence.Start
otherHasWrapped := other.Current < sequence.Start
if hasWrapped == otherHasWrapped {
return sequence.Current > other.Current
}
return hasWrapped
}
...
}server/functions/nextval.go:35-67
func nextval(ctx *sql.Context, ait *sequences.SequenceTracker, relationName string) (int64, error) {
...
next, err := ait.Next(ctx, sequenceString, nil)
if err != nil {
return 0, err
}
err = collection.SetVal(ctx, sequenceId, next, true, true)
...
return next, err
}Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — Concurrent sequence allocations reuse values**
**What failed:** Both concurrent transactions returned 1 and committed, while subsequent allocations returned 3, 5, and 7. The expected behavior was distinct committed values, with the second transaction receiving the next value and the merged state preserving both advances.
- **Impact:** Concurrent committed allocations can return the same sequence value, allowing users or downstream records to receive duplicate identifiers or ordering values. The lost advance also causes later allocations to diverge from the number of committed transactions, and cannot be corrected for values already issued.
- **Steps to reproduce:**
1. Create an integer sequence starting at 1 with increment 2 and cache 1.
2. Start two independent transactions against the same sequence and call nextval from both before either transaction commits.
3. Commit the transactions in reverse order, then call nextval repeatedly after the commits.
4. Compare the values returned by both transactions with subsequent allocations.
- **Stub / mock content:** No stubs, mocks, or bypasses were applied for this test in the recorded run.
- **Code analysis:** The PR introduces SequenceState.Merge at core/sequences/collection.go:75-96. For compatible states it chooses one state's Current at lines 90-94; when concurrent transaction states represent the same base progress, the equal-state comparison in GreaterThan at lines 116-139 does not encode that two allocations occurred, so merging can retain only one advance. The new nextval path in server/functions/nextval.go:58-64 obtains a tracker value and then mirrors it into the collection, making this merge/allocation contract part of the production sequence path. The smallest practical fix is to make tracker transaction merges account for each committed allocation, or serialize allocation at the tracker boundary so concurrent committed calls cannot merge to the same returned value; preserve the furthest resulting state rather than collapsing equal concurrent states.
- **Why this is likely a bug:** The duplicate value was observed in a local transaction run, and the production code now merges transaction-local sequence states by selecting a single Current without representing both committed allocations. The later 3, 5, 7 progression confirms the sequence advanced only once for the two calls, which is a silent uniqueness failure rather than an environment-only error.
**Relevant code:**
`core/sequences/collection.go:75-96`
~~~go
func (sequence SequenceState) Merge(otherSequenceState SequenceState) (merged SequenceState, ok bool) {
newSequenceState := sequence
if sequence.Increment >= 0 && otherSequenceState.Increment >= 0 {
newSequenceState.Increment = utils.Min(sequence.Increment, otherSequenceState.Increment)
newSequenceState.Start = utils.Min(sequence.Start, otherSequenceState.Start)
}
if sequence.GreaterThan(otherSequenceState) {
newSequenceState.Current = sequence.Current
} else {
newSequenceState.Current = otherSequenceState.Current
}
return newSequenceState, true
}
~~~
`core/sequences/collection.go:116-139`
~~~go
func (sequence SequenceState) GreaterThan(other SequenceState) bool {
if sequence.Increment > 0 {
hasWrapped := sequence.Current < sequence.Start
otherHasWrapped := other.Current < sequence.Start
if hasWrapped == otherHasWrapped {
return sequence.Current > other.Current
}
return hasWrapped
}
...
}
~~~
`server/functions/nextval.go:35-67`
~~~go
func nextval(ctx *sql.Context, ait *sequences.SequenceTracker, relationName string) (int64, error) {
...
next, err := ait.Next(ctx, sequenceString, nil)
if err != nil {
return 0, err
}
err = collection.SetVal(ctx, sequenceId, next, true, true)
...
return next, err
}
~~~
zachmu
approved these changes
Jul 29, 2026
zachmu
left a comment
zachmu
left a comment
Member
There was a problem hiding this comment.
Not sure how complete this is but nothing too objectionable
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestConcurrentSequences(t *testing.T) { |
Member
There was a problem hiding this comment.
I believe there is a skipped test that does this too, might want to consolidate
| Expected: []sql.Row{{1}}, | ||
| }, | ||
| { | ||
| Query: "/* client b */ select nextval('myseq')", |
Member
There was a problem hiding this comment.
Probably should have each client commit as well, then assert the current values
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.