Skip to content

Conversation

@mkysel
Copy link
Collaborator

@mkysel mkysel commented Oct 24, 2025

Unify vector selector queries by updating DBBasedCursorUpdater.read to use queries.SelectVectorClock and map OriginatorNodeID to OriginatorSequenceID

The cursor updater switches to the vector clock query and aligns sequence mapping to originator values, and the SQL layer removes the latest cursor query while changing sequence selection to read from the latest table via COALESCE.

  • Update DBBasedCursorUpdater.read to call queries.SelectVectorClock and map OriginatorNodeID to OriginatorSequenceID in cursor_updater.go
  • Remove GetLatestCursor named query and generated accessors in envelopes.sql.go
  • Modify GetLatestSequenceId to COALESCE((SELECT originator_sequence_id FROM gateway_envelopes_latest WHERE originator_node_id = @originator_node_id), 0)::BIGINT in envelopes.sql

📍Where to Start

Start with DBBasedCursorUpdater.read in cursor_updater.go to see the switch to queries.SelectVectorClock and the updated node-to-sequence mapping.


📊 Macroscope summarized 4994e4d. 1 file reviewed, 2 issues evaluated, 2 issues filtered, 0 comments posted

🗂️ Filtered Issues

pkg/api/metadata/cursor_updater.go — 0 comments posted, 2 evaluated, 2 filtered
  • line 93: Possible nil dereference if cu.store is nil. read() calls queries.New(cu.store).SelectVectorClock(cu.ctx) (line 93). If the *sql.DB passed to NewCursorUpdater was nil, queries.New stores a nil DBTX, and SelectVectorClock will invoke q.db.QueryContext(...) on a nil receiver, which will panic at runtime. There is no guard in NewCursorUpdater or read() to prevent this. Add a nil check on store at construction or before invoking the query and return an error if nil. [ Low confidence ]
  • line 100: Behavior/contract parity risk: The diff changes the query used from GetLatestCursor to SelectVectorClock, and the mapped field from row.MaxSequenceID to row.OriginatorSequenceID (line 100). If these two queries/fields are not semantically equivalent (e.g., GetLatestCursor returned a per-node maximum and SelectVectorClock returns something else or has different grouping/ordering), the computed cursor may change, potentially causing missed or extra subscriber notifications or an incorrect cursor state. Verify that SelectVectorClock returns exactly one record per OriginatorNodeID with the correct latest sequence and that OriginatorSequenceID is the intended value. If not, adapt the mapping or query to preserve the previous externally visible behavior. [ Low confidence ]

@mkysel mkysel requested a review from a team as a code owner October 24, 2025 17:10
@graphite-app
Copy link

graphite-app bot commented Oct 24, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • Queue - adds this PR to the back of the merge queue
  • Hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@mkysel mkysel merged commit a6df098 into main Oct 24, 2025
11 checks passed
@mkysel mkysel deleted the mkysel/unify-vector-selectors branch October 24, 2025 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants