Remove the ever growing pile of memory in locking_tx_datastore #38
Conversation
| set_id: table_id.0, | ||
| data_key: row_id.0, | ||
| }) | ||
| if let Some(pv) = table.delete(&row_id) { |
There was a problem hiding this comment.
This let Some check became necessary after rebasing onto the restart patch.
| /// Operations in a transaction are either Inserts or Deletes. | ||
| /// Inserts report the byte objects they inserted, to be persisted | ||
| /// later in an object store. | ||
| pub enum TxOp { |
There was a problem hiding this comment.
Happy to rename these new types to whatever.
crates/core/src/db/commit_log.rs
Outdated
| @@ -51,37 +57,38 @@ impl CommitLog { | |||
| } | |||
| } | |||
|
|
|||
| fn generate_commit<D>(&self, transaction: Arc<Transaction>, datastore: &D) -> Option<Vec<u8>> | |||
| fn generate_commit<D>(&self, tx_data: &TxData, datastore: &D) -> Option<Vec<u8>> | |||
| where | |||
| D: MutTxDatastore<RowId = RowId>, | |||
| { | |||
| // TODO(george) Don't clone the data, just the Arc. | |||
There was a problem hiding this comment.
I'm not 100% sure what this comment was referring to; is it now stale?
There was a problem hiding this comment.
It looks like it was your comment, so I'm not sure tbh.
There was a problem hiding this comment.
Looks stale... no visible cloning anymore?
crates/core/src/db/commit_log.rs
Outdated
| @@ -51,37 +57,38 @@ impl CommitLog { | |||
| } | |||
| } | |||
|
|
|||
| fn generate_commit<D>(&self, transaction: Arc<Transaction>, datastore: &D) -> Option<Vec<u8>> | |||
| fn generate_commit<D>(&self, tx_data: &TxData, datastore: &D) -> Option<Vec<u8>> | |||
| where | |||
| D: MutTxDatastore<RowId = RowId>, | |||
| { | |||
| // TODO(george) Don't clone the data, just the Arc. | |||
There was a problem hiding this comment.
Looks stale... no visible cloning anymore?
crates/core/src/db/commit_log.rs
Outdated
| { | ||
| let mut guard = self.odb.lock().unwrap(); | ||
| for data in datas.into_iter() { | ||
| guard.add((*data).clone()); | ||
| for record in tx_data.records.iter() { |
There was a problem hiding this comment.
| for record in tx_data.records.iter() { | |
| for record in &tx_data.records { |
There was a problem hiding this comment.
This is more idiomatic rust basically.
| /// Whether the operation was an insert or a delete. | ||
| pub(crate) op: TxOp, | ||
| /// The value of the modified row. | ||
| pub(crate) pv: ProductValue, |
There was a problem hiding this comment.
Why are we storing both the product_value and the bytes for the produce_value in the record? Shouldn't we just be doing one or the other?
There was a problem hiding this comment.
Also I'd probably change pv -> product_value
There was a problem hiding this comment.
The bytes make it out to the object store. The ProductValue goes out to the sql vm. Doing just one or the other would I think require this also handing out the schema for the row at the point of time of the transaction, to either encode or decode accordingly.
Changing pv now.
* Remove the ever growing pile of memory in locking_tx_datastore * Clippy lint * cargo fmt * Cleanups * Fix lint * Rename TxRecord::pv to product_value --------- Co-authored-by: Boppy <no-reply@boppygames.gg> Co-authored-by: Tyler Cloutier <cloutiertyler@aol.com>
* Remove the ever growing pile of memory in locking_tx_datastore * Clippy lint * cargo fmt * Cleanups * Fix lint * Rename TxRecord::pv to product_value --------- Co-authored-by: Boppy <no-reply@boppygames.gg> Co-authored-by: Tyler Cloutier <cloutiertyler@aol.com>
Description of Changes
API
If the API is breaking, please state below what will break