-
Notifications
You must be signed in to change notification settings - Fork 5
NONEVM-3304: Add deterministic tiebreakers to log query ORDER BY clause #505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NONEVM-3304: Add deterministic tiebreakers to log query ORDER BY clause #505
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds deterministic tiebreakers (tx_lt, msg_index) to log query ORDER BY clauses to ensure consistent query ordering across PostgreSQL instances. The tiebreakers inherit their sort direction from the last explicit sort field.
Key Changes:
- Modified
addOrderByto appendtx_ltandmsg_indextiebreakers when explicit sort fields are provided - Tiebreakers use the same direction (ASC/DESC) as the last sort field to maintain consistent ordering
- Added comprehensive tests for timestamp sorting with tiebreakers
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| pkg/logpoller/store/postgres/queryparser.go | Refactored addOrderBy to append tiebreaker fields and added helper function for direction conversion |
| pkg/logpoller/store/postgres/queryparser_test.go | Updated existing test expectation and added new test cases for timestamp sorting with ASC/DESC directions |
| integration-tests/logpoller/pg_logs_test.go | Refactored existing test and added test case for timestamp collision scenarios with tiebreaker verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Check exact SQL with ORDER BY | ||
| expectedSQL := `SELECT id, filter_id, chain_id, address, event_sig, data_header, data_payload, tx_hash, tx_lt, msg_index, tx_timestamp, block_workchain, block_shard, block_seqno, block_root_hash, block_file_hash, master_block_seqno, msg_lt, created_at FROM ton.log_poller_logs WHERE chain_id = :chain_id AND address = :address AND event_sig = :event_sig ORDER BY tx_lt DESC` | ||
| // Check exact SQL with ORDER BY - includes msg_index tiebreaker (tx_lt already primary sort) | ||
| expectedSQL := `SELECT id, filter_id, chain_id, address, event_sig, data_header, data_payload, tx_hash, tx_lt, msg_index, tx_timestamp, block_workchain, block_shard, block_seqno, block_root_hash, block_file_hash, master_block_seqno, msg_lt, created_at FROM ton.log_poller_logs WHERE chain_id = :chain_id AND address = :address AND event_sig = :event_sig ORDER BY tx_lt DESC, msg_index DESC` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these test cases could be refactored to use TableDrivenTests pattern. This would make the test cases clearer. If not, it would be great to at least extract the repeated string of the expectedSQL to a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah good point! didn't see that opportunity.
Ensures consistent query ordering by appending deterministic tiebreakers (tx_lt, msg_index) to ORDER BY clauses.
Changes
tx_ltandmsg_indexas tiebreakers when explicit sort fields are provided