feat(storage): clear up dropped column #13952
Conversation
zwang28
force-pushed
the
wangzheng/clear_column
branch
from
86095b3 to
4337885
Compare
zwang28
force-pushed
the
wangzheng/clear_column
branch
from
4337885 to
5994337
Compare
There was a problem hiding this comment.
I think the optimization may be unnecessary, because if there is no column to drop, the rewriting attempt is lightweight. See try_drop_invalid_columns. To be verified via compaction benchmark.
Good point. Look forward to the benchmark. If the optimization is not needed, the changes can be even simpler.
Why entries of the same group in existing SSTs may have different schemas?
Indeed. Given that fast compaction is enabled by default in bottom level, that basically means the optimization can never applies to bottom level unless we have maintained more metadata. |
Say a SST with older schema and a SST with newer schema of the same table are compacted into one SST. |
Here is the microbenchmark. The task is to build output SSTs from two input nonoverlapping levels. Each level has 2 SSTs. Each SST has 256k KVs from 2 tables. Each KV's value part is column aware row encoded, with N columns of int64 type. The setup is the same as other functions in the Conclusions:
|
This comment was marked as outdated.
This comment was marked as outdated.
I think this scenario is related to fast compaction. Without fast compaction, entries in the output SST should share the same schema version.
1, 2 and 3 indicate that the optimization can eliminate most overhead of 4 indicates that the overhead of This means the optimization is a must-have and IIUC, the only blocker to enable this optimization is fast compact. To be more specific, with fast compact enabled, it may result in dropped columns never cleaned. This is very similar to range tombstone and stale keys can never be cleaned with fast compact so I am thinking whether we should follow what we do in https://github.com/risingwavelabs/risingwave/pull/13690/files#diff-04cd80a5bd6a9c040d2153a5e6fd701b126ee0dcce878314a598772a8a7c757dR390 to disable fast compaction when there are too many entries to clean, regardless of whether and where to maintain the (table, schema version id) mapping. |
|
Let me summary the PR. This PR sticks to a solution that doesn't introduce any new fields in storage SST or meta store, i.e. table version. With that being said, the key point is how to eliminate unnecessary dropping column check (which can be heavy if number of column is large), for most KVs during compaction. The idea is to logically group KVs, and skip remaining KVs for dropping column check if there is already a KV in the same group has been checked and resulted in no column to drop. With regard to implementation,
According to the latest benchmark result below, the performance degradation is acceptable to me. (pls refer to old impl's result for meaning of each bench) The performance is affected by several factors:
The PR is actually simple, most LOC is for bench. I suggest to adopt this approach first. If there is compaction performance issue in the future, we can easily change to the solution that storage maintains table version, which is more complex (part of its impl). |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #13952 +/- ##
==========================================
- Coverage 68.06% 67.99% -0.08%
==========================================
Files 1535 1535
Lines 264826 265072 +246
==========================================
- Hits 180266 180235 -31
- Misses 84560 84837 +277
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -268,3 +287,89 @@ impl ValueRowDeserializer for ColumnAwareSerde { | |||
| self.deserializer.deserialize(encoded_bytes) | |||
| } | |||
| } | |||
|
|
|||
| /// Deserializes row `encoded_bytes`, drops columns not in `valid_column_ids`, serializes and returns. | |||
There was a problem hiding this comment.
IIUC, we don't do "real" deserialization here. We're only working with the serialized bytes and drop the unnecessary ones then rearrange them.
| && let Some(object_id) = object_id | ||
| && let Some(block_id) = block_id | ||
| && skip_schema_check.get(&object_id).map(|prev_block_id|*prev_block_id != block_id).unwrap_or(true) | ||
| && let Some(schema) = schemas.get(&check_table_id) { |
There was a problem hiding this comment.
Is it possible that the schema can be stale? For instance, if a new column is added and written to the storage while the schema is still the old one, it may incorrectly identify the new column as invalid.
There was a problem hiding this comment.
No.
Schema given in compaction task is at least as new as the one from compaction task input SSTs, guaranteed by meta when issuing compaction task, i.e. pick compaction task first, then fulfill its schema field with the latest.
|
This PR has been open for 60 days with no activity. Could you please update the status? Feel free to ping a reviewer if you are waiting for review. |
# Conflicts: # src/common/src/config.rs # src/meta/node/src/lib.rs # src/meta/src/hummock/manager/mod.rs # src/meta/src/manager/catalog/mod.rs # src/meta/src/manager/env.rs # src/storage/benches/bench_compactor.rs # src/storage/src/hummock/compactor/compaction_utils.rs # src/storage/src/hummock/compactor/compactor_runner.rs # src/storage/src/hummock/iterator/forward_merge.rs # src/storage/src/hummock/iterator/merge_inner.rs # src/storage/src/hummock/iterator/skip_watermark.rs
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 9425213 | Triggered | Generic Password | 666e7b8 | ci/scripts/regress-test.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
Our GitHub checks need improvements? Share your feedbacks!
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR makes compaction clear up columns dropped by
alter table drop column.There are alternative ways to implement step 2, with different tradeoffs. This PR prefers a way that requires no change to any storage format. It finds old columns by comparing the column ids between the one in value and the one from meta node, without knowing the schema version.
Optional Optimization
(object id, table id). If the first value of one group is deserialized and results in no column to drop, then the rewriting attempts for other values in this group are skipped.This optimization assumes that all values in the same group should share the same schema. But unfortunately it can be broken in some cases:- The existing SSTs before this PR. This is trivial to address, e.g. run a full compaction without this optimization once.- Fast compaction. I don't have any idea how to address this yet.- On second thought I think the optimization may be unnecessary, because if there is no column to drop, the rewriting attempt is lightweight. Seetry_drop_invalid_columns. To be verified via compaction benchmark.close #12224
Checklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.