Conversation
igamigo
force-pushed
the
igamigo-map-sync-table
branch
3 times, most recently
from
e0e50c0 to
fa533d9
Compare
igamigo
force-pushed
the
igamigo-map-sync-table
branch
from
fa533d9 to
b3c4852
Compare
|
@igamigo - is this PR still active? or has it been superseded by something else? |
Just rebased it off of |
|
Failing test should be fixed with (something like) #1164 |
|
I've removed the select query and test from this PR and added it in the followup (#1140) |
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left a few small comments inline.
| // First, update any existing rows with the same (account_id, slot, key) to set | ||
| // is_latest_update=false | ||
| let update_count = diesel::update(schema::account_storage_map_values::table) | ||
| .filter( | ||
| schema::account_storage_map_values::account_id | ||
| .eq(account_id.to_bytes()) | ||
| .and(schema::account_storage_map_values::slot.eq(slot_to_raw_sql(slot))) | ||
| .and(schema::account_storage_map_values::key.eq(&key.to_bytes())) | ||
| .and(schema::account_storage_map_values::is_latest_update.eq(true)), | ||
| ) | ||
| .set(schema::account_storage_map_values::is_latest_update.eq(false)) | ||
| .execute(conn)?; | ||
|
|
||
| // Insert the new row with is_latest_update=true | ||
| let insert_count = diesel::insert_into(schema::account_storage_map_values::table) | ||
| .values(&[( | ||
| schema::account_storage_map_values::account_id.eq(account_id.to_bytes()), | ||
| schema::account_storage_map_values::block_num.eq(block_num.to_raw_sql()), | ||
| schema::account_storage_map_values::slot.eq(slot_to_raw_sql(slot)), | ||
| schema::account_storage_map_values::key.eq(key.to_bytes()), | ||
| schema::account_storage_map_values::value.eq(value.to_bytes()), | ||
| schema::account_storage_map_values::is_latest_update.eq(true), | ||
| )]) | ||
| .execute(conn)?; |
There was a problem hiding this comment.
Not for this PR: I think this works fine because we are working with an in-memory database, but if we were working with something like Postgres running on a separate machine, we'd probably want to send both queries together to the server. And probably send a batch of updates to minimize the number of network requests.
There was a problem hiding this comment.
I meant to add the change here, but I had modified this to use a single transaction in the follow up: https://github.com/0xMiden/miden-node/pull/1140/files#diff-1a7b5569fe361ba6496af6cef83594571e7e29590f0ced96fbeadfaf51298146R320
There was a problem hiding this comment.
We should not require that, since - with a few exceptions - we use the async-sync wrapper function that also wraps the queries::* call in a transaction. Any nested transactions won't have an effect.
| value BLOB NOT NULL, | ||
| is_latest_update BOOLEAN NOT NULL, | ||
|
|
||
| PRIMARY KEY (account_id, block_num, slot, key), |
There was a problem hiding this comment.
This creates an index on (account_id, block_num, slot, key) - right? But I think we also need an index on (account_id, block_num) (for the select query).
There was a problem hiding this comment.
I created an index on #1140 to account for the select, but I think we might need to add slot and key in there as well because we want to keep ordering between calls to account for the LIMIT cutoff.
EDIT: Never mind this comment, we just want to order by block number so it should be fine
4 participants
Adds a SQL table for keeping account storage map values.
There is already an existing table which is very similar (
account_storage_map_updates), but for now I'm not touching old code util we can replace old flows completely. Although for the account deltas tables and queries we might want to delete endpoints and tables altogether.