Fix SQL Upgrade #471
Fix SQL Upgrade #471
Conversation
WalkthroughThis pull request introduces three new environment variable exports in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as dev/run Script
participant A as Go Application
U->>S: Execute dev/run script
S->>S: Export XMTPD_CONTRACTS_NODES_ADDRESS
S->>S: Export XMTPD_CONTRACTS_IDENTITY_UPDATES_ADDRESS
S->>S: Export XMTPD_CONTRACTS_MESSAGES_ADDRESS
S->>A: Run "go run" command
A-->>U: Application started
sequenceDiagram
participant MR as Migration Runner
participant M03 as Migration 00003
participant M04 as Migration 00004
participant DB as Database
MR->>M03: Apply migration (remove block_hash)
M03->>DB: Remove block_hash column from latest_block
MR->>M04: Apply migration up (add block_hash)
M04->>DB: Add block_hash column to latest_block
Note right of MR: For rollback:
MR->>M04: Apply migration down (drop block_hash)
M04->>DB: Execute ALTER TABLE DROP COLUMN IF EXISTS block_hash
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
dev/run (1)
13-15: Document the purpose of contract addresses.These appear to be development/test contract addresses. Consider adding comments to document:
- The purpose of each contract
- Whether these are test-only addresses
- Any environment-specific variations (dev/staging/prod)
pkg/migrations/00004_add_blockchain_columns.down.sql (1)
1-8: Consider wrapping migrations in transactions.While the migration follows proper order (index → table → column), consider wrapping it in a transaction to ensure atomicity.
+BEGIN; -- Drop index first DROP INDEX IF EXISTS idx_blockchain_messages_canonical; -- Then drop the table DROP TABLE IF EXISTS blockchain_messages; -- Drop newly added column ALTER TABLE latest_block DROP COLUMN IF EXISTS block_hash; +COMMIT;pkg/migrations/00004_add_blockchain_columns.up.sql (2)
1-11: Consider wrapping migrations in transactions.Similar to the down migration, consider wrapping in a transaction for atomicity.
+BEGIN; CREATE TABLE blockchain_messages( block_number BIGINT NOT NULL, block_hash BYTEA NOT NULL, originator_node_id INT NOT NULL, originator_sequence_id BIGINT NOT NULL, is_canonical BOOLEAN NOT NULL DEFAULT TRUE, PRIMARY KEY (block_number, block_hash, originator_node_id, originator_sequence_id), FOREIGN KEY (originator_node_id, originator_sequence_id) REFERENCES gateway_envelopes(originator_node_id, originator_sequence_id) ); CREATE INDEX idx_blockchain_messages_block_canonical ON blockchain_messages(block_number, is_canonical); +COMMIT;
13-13: Clarify NULL handling strategy for block_hash.The design choice to make block_hash nullable in latest_block but NOT NULL in blockchain_messages needs clarification:
- What scenarios lead to NULL block_hash in latest_block?
- How does the application handle the disparity between nullable and NOT NULL block_hash?
- Should we add a CHECK constraint to ensure data consistency with blockchain_messages?
This aligns with the PR objective's concern about NULL column handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
dev/run(1 hunks)pkg/migrations/00003_add-latest-block.up.sql(1 hunks)pkg/migrations/00004_add_blockchain_columns.down.sql(1 hunks)pkg/migrations/00004_add_blockchain_columns.up.sql(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test (Node)
| CREATE TABLE latest_block( | ||
| contract_address TEXT NOT NULL PRIMARY KEY, | ||
| block_number BIGINT NOT NULL, | ||
| block_hash BYTEA NOT NULL | ||
| block_number BIGINT NOT NULL | ||
| ); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify data migration strategy for block_hash.
The removal of the NOT NULL block_hash column followed by its reintroduction as nullable in migration 00004 requires careful consideration:
- How is existing data handled during the transition?
- What is the strategy for handling NULL block_hash values in the application code?
Run this script to check for existing data handling:
🏁 Script executed:
#!/bin/bash
# Search for block_hash handling in the codebase
rg -A 5 "block_hash" --type goLength of output: 2054
Action Required: Review and Adjust Nullable Block Hash Handling in Latest Block Updates
The migration sequence appears to rely on a two-step change:
- Migration 00003 creates the
latest_blocktable without ablock_hashcolumn. - Migration 00004 subsequently adds
block_hashas a nullable column.
The SQL queries in pkg/db/queries/queries.sql.go show that the application inserts and updates records in latest_block using a statement that includes block_hash:
- The
INSERT ... ON CONFLICT DO UPDATEclause updates bothblock_numberandblock_hashand includes the condition
WHERE $2 > latest_block.block_number AND $3 != latest_block.block_hash.
Because SQL’s standard inequality operator does not handle NULL well (e.g., both NULL != NULL and value != NULL evaluate to unknown), this raises two concerns:
- Data Transition: How will existing rows migrated without a
block_hashvalue (i.e. implicitlyNULL) get updated when a valid block hash becomes available? The current condition may block the update if a new non‑NULL value is compared against aNULLstored value. - Application Handling: It is unclear if and how the application code ensures that
block_hashis neverNULLwhen an update is expected or if it’s intentionally designed to allowNULLvalues.
Recommendations:
- Confirm the Migration Intent: Verify that migration 00004 aligns with the intended business logic. If updates should occur even when transitioning from a
NULLvalue, consider revising the update condition. - Use Null-Safe Comparison: To reliably handle cases where either the new or stored
block_hashmight beNULL, consider replacing the condition$3 != latest_block.block_hashwith a null-safe comparison likeIS DISTINCT FROM(if supported by your SQL dialect).
For example:... AND $3 IS DISTINCT FROM latest_block.block_hash
- Data Transition Strategy: Ensure that the migration plan accounts for existing rows without a block hash and defines how they should be updated once a non‑NULL value is available.
By addressing these points, you can ensure the migration sequence and subsequent data handling in the application are robust.
Introduced by mistake in #471 Remove them to use the ones defined in `local.env` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Streamlined internal configuration by removing outdated settings to simplify environment management. - These backend adjustments help maintain consistent performance without affecting any visible features for users. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The node does come up.
But I am not sure whether the code can handle a NULL column value.
Handing this partial solution over to @fbac .
Summary by CodeRabbit
New Features
Chores