[Backport] Column-level metadata loading and serialization - #22768
Conversation
When making changes to only a subset of columns, DuckDB currently requires loading and serializing the full column metadata for all other columns in the touched rowgroups too. For large tables, this can create a significant IO and memory overhead (as this memory is not tracked by the buffer pool). This PR makes it so that operations such as `ALTER TABLE ... ADD COLUMN | REMOVE COLUMN | ALTER TYPE` as well as `UPDATE` operations only need to load and write out the metadata for the changed columns. It further extends the `pragma_storage_info` table function with a new named parameter called `only_loaded_segments` which, when explicitly set to true, will only return the segment information for those segments that are actually loaded in memory. This helps test that the change has the desired effect (i.e. only relevant columns are loaded).
Mytherin
left a comment
There was a problem hiding this comment.
Thanks - I still think this is very large for a bug-fix patch that also touches on some critical sections of the code (storage / checkpointing). Even though most of it is gated behind a flag many of the refactors are not. I'm OK with merging it but it would help if you could do extra testing on your end to ensure this works correctly and there's no other hidden issues that pop up.
|
|
||
| if (has_per_column_metadata_blocks) { | ||
| row_group->per_column_metadata_blocks = per_column_metadata_blocks; | ||
| row_group->per_column_metadata_blocks.RemoveColumn(target_idx); |
There was a problem hiding this comment.
Should this not be removed_column? Isn't target_idx here equal to columns.size() - 1?
There was a problem hiding this comment.
Good catch. This was effectively trying to remove the extra metadata at target_idx = columns.size(), which does not exist, i.e. keeping the metadata as is. It's not a big deal though, as the metadata was fixed during the following checkpoint.
I've addressed this in 394bb32
| if (debug_verify_blocks) { | ||
| if (write_action == RowGroupWriteAction::REUSE_EXISTING_ROW_GROUP_METADATA) { | ||
| auto existing_column_count = entry->ReferenceNode()->GetColumnCount(); | ||
| reuse_column.reserve(existing_column_count); |
There was a problem hiding this comment.
Either this needs to be resize or we need to use push_back to add elements to this vector
| auto write_action = row_group_write_data.write_action; | ||
| auto debug_verify_blocks = Settings::Get<DebugVerifyBlocksSetting>(GetAttached().GetDatabase()) && | ||
| dynamic_cast<SingleFileTableDataWriter *>(&checkpoint_state.writer) != nullptr; | ||
| std::vector<bool> reuse_column; |
There was a problem hiding this comment.
nit: we prefer to just use vector, not std::vector
| vector<ColumnSegmentInfo> GetColumnSegmentInfo(const QueryContext &context); | ||
| vector<ColumnSegmentInfo> | ||
| GetColumnSegmentInfo(const QueryContext &context, | ||
| ColumnSegmentInfoScanType scan_type = ColumnSegmentInfoScanType::ALL) const; |
There was a problem hiding this comment.
This is a feature / API change that should be left out on this branch
There was a problem hiding this comment.
addressed in fb82839 (reduces testing coverage though).
Otherwiseit's unnecessarily keeping metadata around
Agreed. If you're fine with the change (i.e. LGTM) I will merge it into our staging environments today to get some extra testing cycles (first with the flag off) to make sure there are no regressions. Note that the fixes in 394bb32 should also make it back to the main branch (would you like me to create a PR, or can you fix this while merging 1.5 changes back to main?) |
|
Thanks! |
…ckdb#22768) This is a backport of #22333 to the v1.5 branch. The branch-specific adaptations are in duckdb/duckdb@85e1d5f and the effective change is gated by an experimental setting (force_column_metadata_reuse). Upstream-Commit: duckdb/duckdb@cd3b2ad
This is a backport of #22333 to the v1.5 branch. The branch-specific adaptations are in 85e1d5f and the effective change is gated by an experimental setting (force_column_metadata_reuse).