Skip to content

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Sep 1, 2025

Description of Changes

Aternative to and closes #3210.
This version relies on pending_schema_changes.
The first commit adds clear_table to the datastore that's efficient and can be exposed to the module ABI in a follow up.
The second commit fixes drop_table.

API and ABI breaking changes

None

Expected complexity level and risk

3?

Testing

test_drop_table_is_transactional is amended to check TxData.

Copy link
Contributor

@Shubham8287 Shubham8287 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks Good, thanks!
Considering there will be a follow-up PR to fix this for commitlog replay.
This PR will make commitlog replay logic to empty dropped tables but we still need a patch to actually drop them from memory.

@Shubham8287 Shubham8287 mentioned this pull request Sep 2, 2025
1 task
Centril and others added 2 commits September 2, 2025 08:37
Co-authored-by: Shubham Mishra <[email protected]>
Signed-off-by: Mazdak Farrokhzad <[email protected]>
Copy link
Contributor

@gefjon gefjon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding off on approving until we get the replay parts of this merged and tested, but I like this.

Comment on lines +718 to +727
committed_state.get_table(table_id).and_then(|table| {
table_stats.insert(
table_id,
TableStats {
row_count: table.row_count,
bytes_occupied_overestimate: table.bytes_occupied_overestimate(),
num_indices: table.num_indices(),
},
)
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reminds me: do we need to clean up any metrics when a table gets deleted? If there are any metrics that have a table ID in the with_label_values, we'll want to call remove_label_values on them at some point, probably within commit.

@bfops bfops added the release-any To be landed in any release window label Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-any To be landed in any release window
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants