feat(api): update version to v1.4.8 and enhance snapshot token creation #2577
Conversation
WalkthroughVersion strings bumped to v1.4.8 in docs and proto; Postgres snapshot token construction moved to use a NewToken path that finalizes MVCC visibility via an internal snapshot reconstruction; related utilities removed and tests adjusted to Encode/Decode flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant NewToken as snapshot.NewToken()
participant Token as Token (t)
participant Finalize as createFinalSnapshot()
participant Parser as Snapshot Parser
Caller->>NewToken: NewToken(xid, snapshotValue)
NewToken->>Token: create token wrapper t
Token->>Finalize: t.createFinalSnapshot()
Finalize->>Parser: Parse "xmin:xmax[:xips]"
Parser-->>Finalize: xmin, xmax, xips
Finalize->>Finalize: insert xid into xips (sorted)
Finalize->>Finalize: adjust xmin/xmax as needed
Finalize-->>Token: finalized snapshot string
Token-->>NewToken: token with finalized snapshot
NewToken-->>Caller: Token (ready to Encode/Use)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
internal/storage/postgres/snapshot/token_test.go (1)
136-150: Strengthen round‑trip assertions and add an unsorted XIPs case.
- Also assert that decoded snapshot matches the original finalized snapshot.
- Add a case with unsorted XIPs (e.g.,
"300:300:280,250") to verify deterministic ordering after yourcreateFinalSnapshotchange.Example additions:
It("Case 1.1: Success - New format (with snapshot)", func() { @@ - for _, tt := range tests { - encoded := tt.original.Encode() - decoded, err := encoded.Decode() - Expect(err).ShouldNot(HaveOccurred()) - Expect(decoded.(Token).Value.Uint).Should(Equal(tt.original.(Token).Value.Uint)) - } + for _, tt := range tests { + encoded := tt.original.Encode() + decoded, err := encoded.Decode() + Expect(err).ShouldNot(HaveOccurred()) + Expect(decoded.(Token).Value.Uint).Should(Equal(tt.original.(Token).Value.Uint)) + Expect(decoded.(Token).Snapshot).Should(Equal(tt.original.(Token).Snapshot)) + } }) + +It("Case 1.2: Success - Unsorted XIPs become deterministic", func() { + t := NewToken(postgres.XID8{Uint: 43242, Status: pgtype.Present}, "300:300:280,250") + // Expect xmin lowered or unchanged, xmax bumped to 43243, and XIPs sorted with txid inserted + Expect(t.(Token).Snapshot).Should(Equal("300:43243:250,280,43242")) +})internal/storage/postgres/data_reader.go (1)
586-586: Constructor change approved; optional consistency refactoring identified at 2 locations.The approved change at line 586 correctly uses
snapshot.NewToken(). For consistency across the codebase, consider also adopting the constructor in:
internal/storage/postgres/data_reader.go:578— ErrNoRows path currently usessnapshot.Token{Value: db.XID8{Uint: 0}, Snapshot: ""}internal/storage/postgres/watch.go:281— currently usessnapshot.Token{Value: value}This ensures uniform encapsulation and makes future invariant changes easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (8)
docs/api-reference/apidocs.swagger.json(1 hunks)docs/api-reference/openapiv2/apidocs.swagger.json(1 hunks)internal/info.go(1 hunks)internal/storage/postgres/data_reader.go(1 hunks)internal/storage/postgres/snapshot/token.go(3 hunks)internal/storage/postgres/snapshot/token_test.go(2 hunks)internal/storage/postgres/utils/common.go(2 hunks)proto/base/v1/openapi.proto(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/storage/postgres/data_reader.go (1)
internal/storage/postgres/snapshot/token.go (1)
NewToken(30-37)
internal/storage/postgres/snapshot/token_test.go (1)
internal/storage/postgres/snapshot/token.go (2)
NewToken(30-37)Token(18-21)
internal/storage/postgres/snapshot/token.go (1)
pkg/database/postgres/xid8.go (1)
XID8(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test with Coverage
- GitHub Check: Analyze (go)
- GitHub Check: Scan Container for Vulnerabilities
- GitHub Check: Test with Coverage
🔇 Additional comments (5)
internal/info.go (1)
26-26: Version bump to v1.4.8 is properly coordinated across artifacts.Verified: OpenAPI specification (
proto/base/v1/openapi.proto), generated protobuf code (pkg/pb/base/v1/openapi.pb.go), and documentation files (docs/api-reference/) all reflect v1.4.8. No stray v1.4.7 references remain in versioned files.internal/storage/postgres/utils/common.go (1)
42-42: Reduce log verbosity; verify snapshot type cast impact on tests.
- Change
slog.Infotoslog.Debugand log snapshot length instead of full string.- Adding
::pg_snapshotcasts at lines 69 and 76 will change generated SQL and break existing test expectations (common_test.go:51, :66). Verify these test expectations should be updated alongside the code changes.- slog.Info("SnapshotQuery called", slog.Uint64("xid", value), slog.String("snapshot", snapshotValue)) + slog.Debug("SnapshotQuery called", slog.Uint64("xid", value), slog.Int("snapshot_len", len(snapshotValue)))Also applies to: 69-71, 76-77
docs/api-reference/openapiv2/apidocs.swagger.json (1)
6-6: Synced OpenAPI version to v1.4.8 — OK.Run the same verification to ensure no drift across docs and proto.
(Use the script provided in the other comment.)
proto/base/v1/openapi.proto (1)
12-12: Proto OpenAPI info.version updated to v1.4.8 — OK.Please ensure generated Swagger artifacts are re-generated from proto to keep them in sync.
You can validate with the repo-wide script in the docs comment; after re-generation, confirm both JSONs still read v1.4.8.
docs/api-reference/apidocs.swagger.json (1)
6-6: Version bump to v1.4.8 is consistent and valid.Verification confirms:
- Both Swagger files and proto definitions advertise v1.4.8
- Internal version constant synced (internal/info.go)
- JSON files are valid
- No stale v1.4.7 references remain
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/storage/postgres/snapshot/token.go (1)
207-212: Consider using strings.Builder for XIP string construction.While functionally correct, the current implementation creates an intermediate
xipStrsslice. The previous review suggested usingstrings.Builderfor better efficiency, which would eliminate the intermediate allocation.Apply this diff for improved performance:
// Build the result snapshot string efficiently using strings.Builder - var xipStrs []string + var sb strings.Builder for _, xip := range xipList { - xipStrs = append(xipStrs, fmt.Sprintf("%d", xip)) + if sb.Len() > 0 { + sb.WriteByte(',') + } + sb.WriteString(strconv.FormatUint(xip, 10)) } - return fmt.Sprintf("%d:%d:%s", newXmin, newXmax, strings.Join(xipStrs, ",")) + return fmt.Sprintf("%d:%d:%s", newXmin, newXmax, sb.String())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/storage/postgres/data_reader.go(1 hunks)internal/storage/postgres/snapshot/token.go(4 hunks)internal/storage/postgres/watch.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
internal/storage/postgres/watch.go (1)
internal/storage/postgres/snapshot/token.go (1)
NewToken(31-38)
internal/storage/postgres/data_reader.go (1)
internal/storage/postgres/snapshot/token.go (1)
NewToken(31-38)
internal/storage/postgres/snapshot/token.go (1)
pkg/database/postgres/xid8.go (1)
XID8(10-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Analyze (go)
- GitHub Check: Test with Coverage
- GitHub Check: Scan Container for Vulnerabilities
- GitHub Check: Test with Coverage
🔇 Additional comments (4)
internal/storage/postgres/watch.go (1)
281-281: Token construction correctly updated to use NewToken.This change addresses the previous review feedback by using the
NewTokenconstructor instead of directToken{...}initialization, ensuring thatcreateFinalSnapshot()is invoked for proper MVCC visibility.internal/storage/postgres/snapshot/token.go (2)
29-38: LGTM! NewToken correctly centralizes snapshot finalization.The updated documentation clearly explains the MVCC visibility behavior, and the internal call to
createFinalSnapshot()ensures consistent token processing across all construction paths.
130-206: Snapshot reconstruction logic correctly implements PostgreSQL MVCC semantics.The method properly:
- Sorts XIPs for deterministic encoding (line 171), addressing the previous review feedback
- Maintains the invariant that xmin ≤ min(active XIDs) and xmax > max(active XIDs)
- Handles edge cases defensively by returning the original snapshot on parse errors
internal/storage/postgres/data_reader.go (1)
578-578: HeadSnapshot correctly migrated to NewToken constructor.Both the empty snapshot case (line 578) and the normal path (line 586) now use
snapshot.NewToken(), addressing the previous review feedback. This ensures consistent snapshot finalization across all code paths.Also applies to: 586-586
Summary by CodeRabbit
Documentation
Improvements
Tests
Chores