Skip to content

Commit aa9e31d

Browse files
jakedttstirrat15
jakedt
and
tstirrat15
authored
fix: postgres revision compare for semi-disjoint overlapping transactions (#2958)
Co-authored-by: Tanner Stirrat <tstirrat@gmail.com>
1 parent 4111b69 commit aa9e31d

File tree

3 files changed

+160
-5
lines changed

3 files changed

+160
-5
lines changed

‎CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
1010

1111
### Fixed
1212
- Regression introduced in 1.49.2: missing spans in ReadSchema calls (https://github.com/authzed/spicedb/pull/2947)
13+
- Long standing bug in the way postgres revisions were being compared. Sometimes revisions that were actually overlapping were erroneously being ordered. (https://github.com/authzed/spicedb/pull/2958)
1314

1415
## [1.49.2] - 2026-03-02
1516
### Added

‎internal/datastore/postgres/snapshot.go‎

Lines changed: 40 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -171,8 +171,8 @@ func (cr comparisonResult) String() string {
171171
// 0:4:2 -> (1,3 visible)
172172
// 0:4:2,3 -> (1 visible)
173173
func (s pgSnapshot) compare(rhs pgSnapshot) comparisonResult {
174-
rhsHasMoreInfo := rhs.anyTXVisible(s.xmax, s.xipList)
175-
lhsHasMoreInfo := s.anyTXVisible(rhs.xmax, rhs.xipList)
174+
rhsHasMoreInfo := s.otherHasMoreInfo(rhs)
175+
lhsHasMoreInfo := rhs.otherHasMoreInfo(s)
176176

177177
switch {
178178
case rhsHasMoreInfo && lhsHasMoreInfo:
@@ -186,11 +186,46 @@ func (s pgSnapshot) compare(rhs pgSnapshot) comparisonResult {
186186
}
187187
}
188188

189-
func (s pgSnapshot) anyTXVisible(first uint64, others []uint64) bool {
190-
if s.txVisible(first) {
189+
// otherHasMoreInfo returns true if other knows the disposition of any
190+
// transaction that s does not. s doesn't know about its own xipList entries
191+
// (in-progress) or any txid >= s.xmax (unseen). If other can see any of
192+
// those as committed, then other has strictly more information.
193+
func (s pgSnapshot) otherHasMoreInfo(other pgSnapshot) bool {
194+
// If any of the in-progress transactions in this snapshot are visible (i.e. commited
195+
// or rolled back) to the other snapshot, the other snapshot has more information.
196+
if slices.ContainsFunc(s.xipList, other.txVisible) {
191197
return true
192198
}
193-
return slices.ContainsFunc(others, s.txVisible)
199+
200+
// Check if other has visibility into any transaction s hasn't seen yet.
201+
// Transactions in [s.xmax, other.xmax) that are NOT in other's xipList
202+
// are committed from other's perspective but completely unknown to s.
203+
// If the range contains more txids than other has in-progress in that
204+
// range, at least one must be settled.
205+
206+
// The following logic is functionally equivalent to iterating from
207+
// `s.xmax` to `other.xmax` and asking whether each of those txids is present
208+
// in `other.xipList`. if any are *not* present, that means that `other`
209+
// knows that one of those txids has been settled, and therefore `other`
210+
// has more information.
211+
212+
// Doing this naively (i.e. the iteration above) is O(n) on the size of `other.xipList`;
213+
// the implementation below is equivalent but makes use of `slices.BinarySearch`
214+
// to make it O(log(n)).
215+
216+
// This condition is only possible if the maximum transaction that the other snapshot
217+
// is aware of is further along than the current snapshot.
218+
if other.xmax > s.xmax {
219+
rangeSize := other.xmax - s.xmax
220+
lo, _ := slices.BinarySearch(other.xipList, s.xmax)
221+
hi, _ := slices.BinarySearch(other.xipList, other.xmax)
222+
xipInRange := uint64(hi - lo) //nolint:gosec // we already know that other.xmax is greater than s.xmax, and we know that xipList is sorted in ascending order.
223+
if xipInRange < rangeSize {
224+
return true
225+
}
226+
}
227+
228+
return false
194229
}
195230

196231
// markComplete will create a new snapshot where the specified transaction will be marked as

‎internal/datastore/postgres/snapshot_test.go‎

Lines changed: 119 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,126 @@ func TestCompare(t *testing.T) {
101101
compareWith pgSnapshot
102102
result comparisonResult
103103
}{
104+
// === Identity / equality ===
105+
{snap(0, 0), snap(0, 0), equal},
106+
{snap(1, 1), snap(1, 1), equal},
107+
{snap(5, 5), snap(5, 5), equal},
108+
{snap(100, 100), snap(100, 100), equal},
109+
{snap(0, 4, 2), snap(0, 4, 2), equal},
110+
{snap(10, 20, 12, 15, 18), snap(10, 20, 12, 15, 18), equal},
111+
112+
// Same visible set expressed differently: all xipList entries fill [xmin,xmax)
113+
{snap(1, 1), snap(1, 5, 1, 2, 3, 4), equal},
114+
{snap(1, 5, 1, 2, 3, 4), snap(1, 1), equal},
115+
{snap(5, 5), snap(5, 8, 5, 6, 7), equal},
116+
{snap(5, 8, 5, 6, 7), snap(5, 5), equal},
117+
118+
// === Strict ordering: one has more info, no conflict ===
119+
// RHS committed one more tx (tx 3)
104120
{snap(0, 4, 2), snap(0, 4, 2, 3), gt},
121+
{snap(0, 4, 2, 3), snap(0, 4, 2), lt},
122+
123+
// Higher xmin means more committed
124+
{snap(5, 10, 7), snap(3, 10, 3, 5, 7), gt},
125+
{snap(3, 10, 3, 5, 7), snap(5, 10, 7), lt},
126+
127+
// Higher xmax with no new in-progress means strictly more info
128+
{snap(1, 1), snap(1, 3), lt},
129+
{snap(1, 3), snap(1, 1), gt},
130+
{snap(5, 5), snap(5, 10), lt},
131+
{snap(5, 10), snap(5, 5), gt},
132+
133+
// One snapshot is a strict superset of the other's knowledge
134+
{snap(10, 10), snap(10, 15, 12), lt},
135+
{snap(10, 15, 12), snap(10, 10), gt},
136+
{snap(10, 10), snap(10, 15), lt},
137+
{snap(10, 15), snap(10, 10), gt},
138+
139+
// xmin advanced past the other's in-progress
140+
{snap(10, 10), snap(8, 10, 8, 9), gt},
141+
{snap(8, 10, 8, 9), snap(10, 10), lt},
142+
143+
// Both see same xmax but one has fewer in-progress
144+
{snap(5, 10, 5, 7), snap(5, 10, 5), lt},
145+
{snap(5, 10, 5), snap(5, 10, 5, 7), gt},
146+
{snap(5, 10, 5, 6, 7, 8, 9), snap(5, 10, 5, 6, 7), lt},
147+
{snap(5, 10, 5, 6, 7), snap(5, 10, 5, 6, 7, 8, 9), gt},
148+
149+
// One sees everything the other does plus a higher xmax range
150+
{snap(0, 5), snap(0, 10), lt},
151+
{snap(0, 10), snap(0, 5), gt},
152+
153+
// === Concurrent: each knows something the other doesn't ===
154+
// Original bug case: LHS knows tx 100 done, RHS knows tx 102 done
155+
{snap(101, 101), snap(100, 104, 100, 101), concurrent},
156+
{snap(100, 104, 100, 101), snap(101, 101), concurrent},
157+
158+
// LHS knows tx 5 committed, RHS knows tx 7 committed
159+
{snap(6, 8, 6, 7), snap(5, 9, 5, 8), concurrent},
160+
{snap(5, 9, 5, 8), snap(6, 8, 6, 7), concurrent},
161+
162+
// Disjoint xmax ranges with each having committed txs the other hasn't seen
163+
{snap(3, 5, 4), snap(2, 7, 2, 3, 6), concurrent},
164+
{snap(2, 7, 2, 3, 6), snap(3, 5, 4), concurrent},
165+
166+
// Each has one committed tx the other considers in-progress
167+
{snap(10, 14, 10, 12), snap(10, 14, 11, 13), concurrent},
168+
{snap(10, 14, 11, 13), snap(10, 14, 10, 12), concurrent},
169+
170+
// LHS advanced xmin past RHS's in-progress, but RHS has higher xmax with committed txs
171+
{snap(20, 20), snap(18, 25, 18, 19), concurrent},
172+
{snap(18, 25, 18, 19), snap(20, 20), concurrent},
173+
174+
// Both have different in-progress sets that overlap in complex ways
175+
{snap(5, 12, 5, 8, 10), snap(5, 12, 6, 9, 11), concurrent},
176+
{snap(5, 12, 6, 9, 11), snap(5, 12, 5, 8, 10), concurrent},
177+
178+
// Wide gap: LHS committed low txs, RHS committed high txs
179+
{snap(50, 50), snap(40, 60, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49), concurrent},
180+
{snap(40, 60, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49), snap(50, 50), concurrent},
181+
182+
// === Large xid gaps: must not iterate the range ===
183+
{snap(0, 4, 2), snap(1<<63, 1<<63), lt},
184+
{snap(1<<63, 1<<63), snap(0, 4, 2), gt},
185+
{snap(1, 1), snap(2, 1<<63), lt},
186+
{snap(2, 1<<63), snap(1, 1), gt},
187+
{snap(1<<62, 1<<62), snap(1<<63, 1<<63), lt},
188+
{snap(1<<63, 1<<63), snap(1<<62, 1<<62), gt},
189+
190+
// Large gap concurrent: each has info the other doesn't
191+
{snap(1<<62, 1<<62), snap(1<<61, 1<<63, 1<<61), concurrent},
192+
{snap(1<<61, 1<<63, 1<<61), snap(1<<62, 1<<62), concurrent},
193+
194+
// === Edge cases ===
195+
// xmin=0, xmax=0: empty snapshot
196+
{snap(0, 0), snap(0, 1), lt},
197+
{snap(0, 1), snap(0, 0), gt},
198+
199+
// Single tx difference
200+
{snap(0, 1), snap(0, 2), lt},
201+
{snap(0, 2), snap(0, 1), gt},
202+
{snap(0, 2, 1), snap(0, 2), lt},
203+
{snap(0, 2), snap(0, 2, 1), gt},
204+
205+
// Adjacent xmax values
206+
{snap(5, 6), snap(5, 7), lt},
207+
{snap(5, 7), snap(5, 6), gt},
208+
209+
// All txs in-progress in range = same knowledge as lower xmax
210+
{snap(3, 3), snap(3, 6, 3, 4, 5), equal},
211+
{snap(3, 6, 3, 4, 5), snap(3, 3), equal},
212+
213+
// One committed tx among many in-progress
214+
{snap(0, 10, 0, 1, 2, 3, 4, 5, 6, 7, 8), snap(0, 10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9), gt},
215+
{snap(0, 10, 0, 1, 2, 3, 4, 5, 6, 7, 8, 9), snap(0, 10, 0, 1, 2, 3, 4, 5, 6, 7, 8), lt},
216+
217+
// markComplete scenario: snapshot after marking own tx complete
218+
{snap(100, 100).markComplete(100), snap(100, 100), gt},
219+
{snap(100, 100), snap(100, 100).markComplete(100), lt},
220+
221+
// markComplete advances past other's in-progress: strictly greater
222+
{snap(100, 102, 100).markComplete(100), snap(100, 102, 101), gt},
223+
{snap(100, 102, 101), snap(100, 102, 100).markComplete(100), lt},
105224
}
106225

107226
for _, tc := range testCases {

0 commit comments

Comments
 (0)