ydb/core/tx/datashard/datashard_ut_incremental_backup.cpp (2)

219-236: FindIncrementalBackupDir helper is reasonable

The helper uses DescribePath with ReturnChildren=true and picks the first child ending with _incremental, which fits the test usage where only one incremental backup is expected per collection. Returning "" and asserting at call sites gives clear failures.

If you ever introduce scenarios with multiple incremental backups per collection, consider selecting the latest child (lexicographically max timestamp) instead of the first match.

---

511-630: SimpleBackupRestoreWithIndex provides strong end-to-end coverage

This test nicely validates:

• Pre-backup index usability via VIEW idx.
• Full backup & restore of a table with a global index.
• Post-restore index behavior (query via view) and the index implementation table row count.

The additional version diagnostics using Ls are harmless and will help debugging if something regresses. The fixed sleeps (5s) are a bit heavy but acceptable in this suite.

ydb/core/tx/datashard/datashard_ut_common_kqp.h (1)

6-6: KqpSimpleExecSuccess helper and test context wiring

The new KqpSimpleExecSuccess wrapper plus #include <ydb/library/ut/ut.h> cleanly centralize “assert SUCCESS + format result” behavior for KQP tests. The TTestContext testCtx parameter is currently not passed into CTX_UNIT_ASSERT_VALUES_EQUAL_C, so this relies on TTestContext RAII semantics rather than an explicit context argument; if CTX_* macros actually expect a context parameter, please thread testCtx through, otherwise consider marking it explicitly unused to avoid warnings.

Also applies to: 186-192

ydb/core/tx/datashard/ut_common/datashard_ut_common.cpp (1)

2098-2104: ExecSQL now explicit about expected status and uses CTX_ assertions*

Extending ExecSQL with an explicit Ydb::StatusIds::StatusCode code and switching to CTX_UNIT_ASSERT_VALUES_EQUAL_C makes the expectation clearer and integrates with the new test‑context machinery. As with the KQP helper, TTestContext testCtx is not passed to the macro, so this depends on RAII semantics in ut.h; please confirm that this is intentional and that all ExecSQL call sites have been updated (or consider adding a convenience overload that defaults code to SUCCESS).

Also applies to: 2111-2113

ydb/core/protos/flat_scheme_op.proto (1)

349-351: Proto extensions for index skipping and per‑index CDC look consistent

Adding optional bool OmitIndexes = 47 [default = false]; to TTableDescription and map<string, TCreateCdcStream> IndexImplTableCdcStreams = 9; to TCopyTableConfig is wire‑compatible and matches the incremental backup design (skip impl copies and track CDC streams per index impl table). It would be helpful to keep the semantics of OmitIndexes consistent with the similarly named flags in backup collection configs and to clearly document when IndexImplTableCdcStreams is expected to be filled (e.g., only for incremental backups with CDC).

Also applies to: 1288-1291

ydb/core/tx/schemeshard/schemeshard_cdc_stream_common.h (1)

13-14: Make dependencies for new CDC state helpers explicit

SyncIndexEntityVersion / SyncChildIndexes look fine, but this header now depends on NIceDb::TNiceDb and TPathElement::TPtr without declaring them locally. To reduce reliance on transitive includes, consider either:

• Adding a lightweight forward declaration for namespace NIceDb { class TNiceDb; } and struct TPathElement;, or
• Including the minimal headers that define these types (e.g. schemeshard_path_element.h / the TNiceDb declaration header).

This keeps the CDC state helpers robust against include-order changes.

Also applies to: 35-54

INCREMENTAL_BACKUP_INDEX_PROGRESS.md (1)

73-97: Clarify what is implemented vs. exploratory in the progress log

This document is very detailed and useful, but sections like “Next Steps (To Be Implemented)” and “Phase 2 Analysis In Progress” now coexist with actual implementation work in this PR (e.g., index version synchronization on restore). It would help future readers if you:

• Explicitly mark which parts of the “Proposed Solution: Version Synchronization” are implemented in code vs. still speculative, and
• Optionally add a short “Current status” summary at the top, reflecting the behavior after this PR.

Also applies to: 98-102

ydb/core/tx/datashard/ut_common/datashard_ut_common.h (1)

15-16: ExecSQL test-context extension looks good

Adding ydb/library/ut/ut.h and extending ExecSQL with an optional TTestContext keeps existing callers working while enabling richer diagnostics in new tests. This is a sensible way to thread context into the helper.

If TTestContext ever becomes heavier to copy, you might switch the parameter to const TTestContext& testCtx = TTestContext() to avoid unnecessary copies, but for current unit-test usage this is purely optional.

Also applies to: 809-815

ydb/core/tx/schemeshard/schemeshard__operation_consistent_copy_tables.cpp (2)

171-213: Consider dialing back or gating verbose NOTICE logging in index copy path

The new LOG_NOTICE blocks around:

• Table-level details (srcPath, dstPath, childrenCount, omitIndexes),
• Table-index descriptions, and
• Per-child decisions (deleted / sequence / non-index / index) and impl-table copies

are excellent for debugging incremental backup issues, but they will produce a lot of NOTICE-level chatter in environments that perform many consistent copy operations.

If this code path can be hot in production, consider either:

• Lowering some of these to DEBUG level, or
• Guarding the more verbose per-child logs behind a feature flag / dynamic log-component level.

Also applies to: 221-247, 263-267

---

268-284: Keying IndexImplTableCdcStreams only by impl-table leaf name may limit future flexibility

IndexImplTableCdcStreams is keyed by srcImplTableName (the impl table’s leaf name). This works today because:

• Each index impl table under a given main table uses the same leaf name (e.g. indexImplTable), and
• You currently configure identical CDC streams for all such impl tables.

However, if you ever need per-index CDC customization (different stream names or settings per index), this map will collapse multiple indexes to the same entry and you’ll lose the ability to distinguish them. In that case you’d likely want to key either by:

• The full path under the main table (e.g. idxName/implTableName), or
• The index path id.

No change is needed now, but it’s worth keeping in mind if the CDC configuration becomes index-specific.

ydb/core/tx/schemeshard/schemeshard__operation_backup_backup_collection.cpp (1)

211-259: Index impl table CDC diagnostics and PQ-part creation are consistent with main-table flow

The added diagnostics after CDC creation (logging AlterVersions for main tables, indexes, and index impl tables) and the PQ-part creation for index impl table CDC streams mirror the existing behavior for main tables:

• StreamImpl is created up-front (main and index impl tables),
• AtTable is driven via CreateConsistentCopyTables using the stored CDC descriptors, and
• PQ parts are created afterwards using per-table partition boundaries.

This symmetry makes debugging schema-version issues around indexes much easier and seems logically correct.

One minor optional improvement would be to reuse the stored TCreateCdcStream from IndexImplTableCdcStreams when building PQ parts instead of reconstructing a fresh struct with the same fields, to keep all CDC configuration for a given table in one place.

Also applies to: 303-360

ydb/core/tx/schemeshard/schemeshard_incremental_restore_scan.cpp (5)

238-261: Index impl tables added to finalize target list look functionally correct but stringly-typed

The new loop over Self->PathsById to add indexImplTable paths in CollectTargetTablePaths correctly filters by EPathStateIncomingIncrementalRestore and by parent table prefix, so it should only pull in index impl tables that are part of this restore. However, matching via pathString.Contains("/indexImplTable") and StartsWith(tablePath + "/") is fairly brittle: it relies on the naming convention and flat strings instead of the existing structural metadata (e.g., index path type + parent-child relationships).

If you want to harden this later, consider:

• Using TPath/TPathElement to check that the path is indeed an index impl table (e.g., parent is EPathTypeTableIndex, child is table) instead of relying on the substring.
• Optionally precomputing the relevant index impl table PathIds from the Indexes map or from the restore operations, then converting to strings, rather than scanning all PathsById for every finalize.

Functionally this is fine; the suggestion is about robustness and avoiding accidental matches if naming ever changes.

---

626-648: FindTargetTablePath suffix matching is simple but can be ambiguous in deeply nested layouts

FindTargetTablePath maps an accumulated relative path to a full table path using:

if (itemPath == relativeTablePath || itemPath.EndsWith("/" + relativeTablePath))

This works for the common case where relativeTablePath is the DB-relative suffix (e.g. dir/table). However, in pathological layouts with overlapping suffixes (e.g. /Root/A/B/table and /Root/C/A/B/table) this could pick the “wrong” table depending on entry order, since both end with /A/B/table.

If such layouts are possible, consider:

• Comparing against the precomputed DB-relative part you already derive via TrySplitPathByDb instead of raw itemPath, or
• Normalizing both sides into split components and comparing vectors rather than string suffixes.

If the schema guarantees uniqueness of these suffixes, this is fine; otherwise it’s a subtle corner case worth double-checking.

---

650-696: Recursive index discovery assumes metadata tree structure; add defensive checks if structure can vary

DiscoverIndexesRecursive walks the __ydb_backup_meta/indexes subtree and:

• Treats accumulatedRelativePath as a table-relative path and calls FindTargetTablePath on it.
• On match, assumes currentPath.Base()->GetChildren() are index names.
• Otherwise, recurses into all children, extending accumulatedRelativePath.

This is correct if the metadata tree mirrors DB-relative table paths and the per-table node directly contains index nodes. If future metadata gains extra layers (e.g. other markers under the table node), CreateSingleIndexRestoreOperation might get invoked on non-index children.

If the layout isn’t strictly guaranteed, it would be safer to:

• Filter children by expected type or naming before calling CreateSingleIndexRestoreOperation, or
• Encode index metadata (e.g. a marker file node) and filter on that instead of blindly iterating all children.

Right now this is structurally sound but a bit optimistic about the metadata tree shape.

---

698-739: Index metadata discovery is tolerant, but lack of index meta is silently treated as “no indexes”

DiscoverAndCreateIndexRestoreOperations:

• Respects OmitIndexes (good).
• Resolves {bcPath}/{backupName}_incremental/__ydb_backup_meta/indexes and, if missing, logs and returns.

This means a mismatch between OmitIndexes=false and missing indexes dir is treated equivalently to “no indexes were backed up”. That’s likely fine, but if you want stricter observability you could:

• Log at LOG_W when omitIndexes == false but indexMetaPath is unresolved, or
• Optionally track a counter/flag in TIncrementalRestoreState that “indexes metadata missing” for diagnosing misconfigured backups.

Behavior-wise, this is safe; suggestion is only for diagnosability.

---

741-875: CreateSingleIndexRestoreOperation overall looks solid; only minor invariants worth calling out

The function correctly:

• Validates the target table resolves and is a table.
• Locates the index by name under the table, verifies it’s a TableIndex and EIndexTypeGlobal.
• Enforces a single-child impl table, and gracefully skips unusual structures.
• Builds a per-index ESchemeOpRestoreMultipleIncrementalBackups op, wires it into IncrementalRestoreOperationToState and TxIdToIncrementalRestore, and populates ExpectedShards/InvolvedShards from the impl table’s partitions.

Minor points to consider:

1. Assumption that every GetChildren()-listed pathId is present in PathsById and Tables

   • PathsById.at(childPathId) and Tables.at(indexImplTablePathId) will Y_ABORT if metadata is temporarily inconsistent (e.g., index in the middle of drop/create).
   • If incremental restore can overlap with DDL on the same table/index, you may want FindPtr/contains checks and a warning-based early return instead of assuming perfect consistency.

2. Handling missing table info for impl table

   • When Tables lacks indexImplTablePathId, you still create and send the restore request but never populate ExpectedShards for that op.
   • Depending on how AllShardsComplete() is implemented, this could either treat the op as “already complete” (0 expected shards) or never complete if responses arrive.
   • If such inconsistent states are possible, consider bailing out before creating the op when Tables has no entry for the impl table, or at least logging at LOG_W and not adding the op to InProgressOperations.

3. Source path construction assumes {indexes}/{relativeTablePath}/{indexName}

   • This matches the new discovery code; if metadata layout ever changes, both must be updated in lockstep.

Functionally, this looks correct for the intended invariants; the above is mostly about making failure modes clearer and avoiding hard aborts.

ydb/core/tx/schemeshard/schemeshard__operation_common_cdc_stream.cpp (3)

115-173: SyncImplTableVersion now persists and publishes versions; check for double-persist and invariants

The extended SyncImplTableVersion:

• Derives targetVersion as max(parentTable.AlterVersion, index.AlterVersion) (when index exists).
• Adjusts the impl table’s AlterVersion to targetVersion (or +1 if already ahead).
• Persists the new version via PersistTableAlterVersion and publishes to SchemeBoard.

This is appropriate when invoked from UpdateTableVersion after an actual schema operation (e.g., CDC-related alter) has already gone through datashards.

Two things to double-check:

1. Double persistence with TProposeAtTable::HandleReply

   • After UpdateTableVersion returns, HandleReply still calls PersistTableAlterVersion(db, pathId, table) unconditionally.
   • For impl tables, that’s harmless but redundant. If PersistTableAlterVersion does any heavier work (indexes, counters), you might consider guarding the second call or documenting the intentional duplication.

2. Impl table ahead of parent

   • In the currentImplVersion > targetVersion branch, you increment impl version again, so impl stays ahead of parent/index.
   • That maintains monotonicity but violates “impl version >= parent/index version” if you wanted strict alignment.
   • If strict alignment is a requirement, you may want to set table->AlterVersion = currentImplVersion and then sync index entity to that, instead of bumping further.

Semantically this looks safe, but verifying expectations around parent vs impl version ordering would be good.

---

175-248: UpdateTableVersion logic is sound but may call SyncChildIndexes twice for continuous backup streams

UpdateTableVersion now distinguishes two cases:

• Index impl table that is part of a continuous backup: calls SyncImplTableVersion, then SyncIndexEntityVersion, then SyncChildIndexes on the grandparent (main table).
• All other cases: increments the table’s AlterVersion, and if the table has a continuous backup stream (HasParentContinuousBackup(versionCtx.PathId, context)), calls SyncChildIndexes to align child indexes.

This is consistent with the intent: keep impl tables, index entities, and sibling indexes in sync when continuous backup is active.

However, in TProposeAtTable::HandleReply you also have:

if (versionCtx.IsContinuousBackupStream && !versionCtx.IsIndexImplTable) {
    NCdcStreamState::SyncChildIndexes(path, table->AlterVersion, OperationId, context, db);
}

For operations on a continuous backup stream:

• UpdateTableVersion’s “main table with continuous backup” path will call SyncChildIndexes once.
• The IsContinuousBackupStream branch above will call it a second time with the same targetVersion.

Because SyncIndexEntityVersion is monotonic (if (targetVersion > oldIndexVersion)), the second pass is effectively a no-op but still iterates all children and logs.

If you want to avoid the duplicate work, you can either:

• Drop the HasParentContinuousBackup branch for the “main table with continuous backup” inside UpdateTableVersion and rely on the explicit IsContinuousBackupStream branch, or
• Add a flag to the TTableVersionContext indicating that child indexes were already synced during UpdateTableVersion.

Functionally correct as is; this is a small efficiency/clarity nit.

---

318-368: SyncChildIndexes correctly avoids impl-table bumps; assumes parentPath has all child indexes loaded

The new SyncChildIndexes:

• Iterates parentPath->GetChildren(), filters to IsTableIndex() and not Dropped().
• Uses SyncIndexEntityVersion to bump index entity versions to targetVersion.
• Explicitly does not touch index impl table versions (with a clear NOTE), to avoid SCHEME_CHANGED errors from bumping versions without a datashard schema TX.
• Logs entry/exit and per-index processing.

This matches the desired semantics: keep index entities in step with their parent’s AlterVersion, while leaving impl tables to be synchronized by actual schema operations (SyncImplTableVersion/alter flows).

Only assumption is that parentPath->GetChildren() and PathsById[childPathId] are always consistent. If there’s any chance of transient inconsistency (e.g. partially dropped index), consider guarding the PathsById.at(childPathId) with a contains check to avoid hard aborts.

ydb/core/tx/schemeshard/schemeshard__operation_incremental_restore_finalize.cpp (3)

40-173: TConfigureParts correctly prepares per-shard ALTERs for index impl tables; minor robustness nits

TConfigureParts::ProgressState:

• Collects target index impl tables via CollectIndexImplTables(finalize, ...).
• If none are found, logs and returns true, making ConfigureParts a no-op.
• For each impl table, ensures AlterData exists (creating minimal TAlterTableInfo when necessary) and bumps AlterData->AlterVersion by 1.
• Adds all partitions’ shard indices to txState->Shards as ConfigureParts operations and persists txState.
• For each shard, finds the owning impl table, builds an ALTER TABLE body, and proposes it to the corresponding datashard, then updates ShardsInProgress.

Overall this is a solid pattern and mirrors other “ConfigureParts” flows in Schemeshard.

Minor improvements to consider:

1. Shard-to-table mapping loop

   • For every shard you linearly scan all implTablesToUpdate and each table’s partitions to find tablePathId. For many tables/shards this is O(N²).
   • If this state can be large, consider precomputing a THashMap<TShardIdx, TPathId> once before the shard loop.

2. Table existence checks

   • You correctly guard Tables.contains(tablePathId) and log if missing, but later in the shard loop you call context.SS->Tables.at(pathId) without re-check.
   • Today implTablesToUpdate only contains IDs that passed the earlier Tables.contains check, but if tables can disappear between these stages, consider either:
     • Building and using a local map of implTablePathId -> TTableInfo::TPtr while collecting, or
     • Re-guarding with contains in the shard loop to avoid Y_ABORT.

Functionally, this should work fine for typical sizes; the suggestions are about scaling and robustness.

---

175-203: CollectIndexImplTables shares assumptions with scan-side collection; looks consistent

CollectIndexImplTables:

• Filters finalize.GetTargetTablePaths() by Contains("/indexImplTable").
• Resolves the path, checks it’s a Table and present in Tables, and then inserts its PathId into implTables.

This matches how CollectTargetTablePaths in the scan code constructs target paths for index impl tables, so the two sides are consistent. If the naming convention for impl tables ever changes, both places will need to be updated together; you might eventually want a shared helper for “is index impl table path” to avoid divergence.

---

271-338: SyncIndexSchemaVersions finalizes impl-table alters and bumps index entities; consider reusing CDC helpers

SyncIndexSchemaVersions:

• Iterates finalize.TargetTablePaths, filtering to paths containing /indexImplTable.
• For each resolved impl table with non-null AlterData:
  • Calls FinishAlter, persists via PersistTableAltered, clears caches, and publishes.
  • Locates the parent index path and unconditionally ++s its AlterVersion, then persists and publishes.

The flow ensures that:

• Impl tables, for which ConfigureParts prepared AlterData->AlterVersion = old + 1, actually commit that version bump.
• Corresponding index entities also advance their AlterVersion, keeping metadata in sync.

Two points worth a closer look:

1. Unconditional index AlterVersion increment

   • This always does oldVersion + 1 regardless of the impl-table’s new version.
   • Elsewhere (NCdcStreamState::SyncIndexEntityVersion), you sync to a target version and avoid regressions.
   • If other operations may have bumped the index version between ConfigureParts and Finalize, you might end up with index AlterVersion != table AlterVersion.
   • To keep semantics consistent, consider using SyncIndexEntityVersion(indexPathId, table->AlterVersion, OperationId, context, db) here instead of manually incrementing.

2. Missing AlterData on impl table

   • If for some reason an impl table reaches finalization without AlterData, you log a warning and skip it.
   • That leaves its schema version unbumped while its index entity might still get bumped (if handled elsewhere).
   • If that’s an unexpected state, you may want to either:
     • Treat it as a hard error (or at least warn at a higher log level), or
     • Recompute a minimal AlterData similar to TConfigureParts instead of skipping.

No blockers here; the behavior is consistent with the new ConfigureParts stage, but reusing the CDC sync helpers would tighten invariants and reuse the “no downgrade” logic.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

INCREMENTAL_BACKUP_INDEX_PROGRESS.md (1)

1-260: This appears to be intermediate investigation documentation—clarify purpose and consolidate with related files.

This file thoroughly documents the investigation journey, but it reads as working notes rather than finalized documentation. It includes failed attempts (#1 and #2) and indicates Phase 2 analysis is "in progress." Several points need clarification:

1. Are the issues documented here (race conditions, version mismatches) actually fixed in this PR's implementation, or are they still pending?
2. The "Next Steps (To Be Implemented)" section on line 75 proposes version synchronization—has this been implemented in the code, or is it future work?
3. This file significantly overlaps with VERSION_SYNC_PLAN.md and INCREMENTAL_BACKUP_INDEX_FIX_SUMMARY.md (all cover similar root causes and solution attempts).

Recommendations:

• Consolidate these three investigation documents into a single IMPLEMENTATION_STATUS.md or INVESTIGATION_NOTES.md with clear sections for: Problem → Root Cause → Attempted Solutions → Final Resolution → Remaining Issues.
• Add explicit status indicators (✅ Fixed / ⏳ Pending / ❌ Blocked) for each issue.
• If this PR includes fully working implementation, move detailed failed attempts to a separate INVESTIGATION_HISTORY.md to keep the main documentation focused on what was delivered.
• Add language specifiers to code blocks (lines 5, 66, 110, 117, 123, 153) to resolve markdown linting warnings.

VERSION_SYNC_PLAN.md (1)

1-280: Clarify whether this is a research plan that was executed or a proposal for future work.

This document presents a detailed 6-phase research and fix plan, but it's unclear whether these steps were actually executed or if they're intended as a roadmap. Key concerns:

1. Phase Execution Status: Phases 1-3 outline diagnostic logging and code search tasks. Were these completed? If so, include the findings.
2. Phase 5 Options: Three implementation approaches (A: Update Index Metadata, B: Increment Main Table Version, C: Send Explicit Version Update) are presented, but there's no indication of which was selected or why.
3. Document Scope: This overlaps significantly with INCREMENTAL_BACKUP_INDEX_PROGRESS.md (both analyze schema version mismatch) and INCREMENTAL_BACKUP_INDEX_FIX_SUMMARY.md (both propose solutions).

Recommendations:

• If this plan was executed: Update the document to summarize actual findings from each phase rather than presenting hypothetical investigation steps.
• If this is future work: Move it out of the PR and into an issue/planning document; the final PR should document what was implemented, not what might be done.
• Add language specifiers to all code blocks (e.g., ```bash, ```cpp) to resolve markdown linting errors.
• Consolidate with related investigation files to avoid maintaining three parallel documents of similar content.

INCREMENTAL_BACKUP_INDEX_FIX_SUMMARY.md (2)

420-512: Consolidate detailed technical summary with other investigation files; consider moving to single implementation summary document.

Lines 420-512 provide an excellent "How It Works" walkthrough of the three-phase CDC creation flow. However, similar explanations appear in other files. Key redundancies:

• Problem description (lines 3-17): Also in INCREMENTAL_BACKUP_INDEX_PROGRESS.md and VERSION_SYNC_PLAN.md
• Root cause analysis (lines 18-29): Overlaps with other files
• Solution approaches and failures (lines 30-150): Similar coverage in INCREMENTAL_BACKUP_INDEX_PROGRESS.md

Recommendations:

• Create a single IMPLEMENTATION_COMPLETE.md or DESIGN_SUMMARY.md that consolidates:
  • Final problem statement
  • Root cause (single version, not repeated across 3 files)
  • Actual implementation approach that was chosen
  • How it works (the excellent walkthrough in lines 420-512)
  • Verification results
• Move detailed failure analysis and investigation steps to optional INVESTIGATION_HISTORY.md for maintainer reference
• This keeps active documentation focused and reduces maintenance burden.

---

485-485: Fix markdown linting issues: add language specifiers and resolve duplicate headings.

Several markdown linting warnings need resolution:

1. Missing language specifiers on code blocks: Lines with ``` should specify language (e.g., ```cpp, ```protobuf, ```bash). Affects lines with code examples throughout.
2. Duplicate heading "Testing": Appears at lines 485 and 563—consolidate or rename one.
3. Duplicate heading "Status" (implicit): Line 548 has heading, and status is scattered through document.

Fix:

• Add language identifiers to all code blocks
• Rename second "Testing" section to "Test Case Details" or "Test Scenarios"
• Consolidate "Current Status" section (line 548) with the overall "Status" context

Also applies to: 513-513, 548-548, 563-563

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

CDC_VERSION_SYNC_RESEARCH_SUMMARY.md (2)

164-164: Fix grammar: missing hyphen in "operation structure" phrase.

Line 164 mentions "More complex operatio" (cut off in static analysis hint). This appears to be a markdown formatting issue where text wrapped awkwardly. Review the full context around line 164 to ensure proper phrasing and clarity.

---

255-255: Add missing comma after year in date.

Line 255 should read: **Date:** January 20, 2025, **Status:** (or restructure) per standard date formatting in formal documents.

DATASHARD_VERSION_VALIDATION_ANALYSIS.md (3)

20-44: Excellent code analysis, but add language identifiers to code blocks for clarity.

Lines 20-44 and throughout the document contain bare code fences without language identifiers. Change:

Y_VERIFY_DEBUG_S(...)

to:

Y_VERIFY_DEBUG_S(...)

This is a minor markdown hygiene issue that improves readability in documentation systems. The static analysis tool flagged this at multiple locations (lines 155, 226, 235, 367, etc.).

---

175-210: Query version check analysis is clear but could address one edge case.

The analysis of query execution (section 3) correctly identifies that queries use SchemeBoard, not direct datashard versions. However, consider adding:

• Behavior when SchemeBoard cache is stale (e.g., new node joining cluster)
• Whether version checks are transactional or can change mid-query

These edge cases don't seem to affect the "converge later" safety argument, but explicitly addressing them would strengthen the analysis.

---

270-301: Test coverage analysis identifies real gaps, but recommendations could be more specific.

Section 5.3 recommends adding tests for "version consistency validation after CDC completes" and "concurrent queries during CDC operation". These are good, but consider specifying:

1. How to trigger concurrent queries (query thread vs. CDC thread timing)
2. What specific assertions to check (exact version numbers, not just "no errors")
3. Whether existing test framework supports this (or needs new instrumentation)

Without these specifics, the testing recommendations may be harder for developers to implement.

cdc_version_sync_design.md (3)

1-100: Well-structured design document with clear problem statement and executive summary.

The document systematically presents the race condition problem and evaluates 5 strategies. Strengths:

• Problem statement with concrete timeline showing race (lines 29-38)
• Clear explanation of why current sync fails (lines 241-281)
• Five distinct strategies with pros/cons

Consider: Is this document primarily for architects/decision-makers or for implementers? If implementers, consider adding a quick "Start here" section directing readers to the recommended strategy (Strategy E) rather than requiring them to read all 5 strategies first.

---

302-406: Add language identifiers to all code fences per markdown best practices.

Lines 312, 330, 336, 367, 376, etc. contain bare code blocks. Should be:

// Example code

Also consider: Are these pseudocode or actual C++ that will compile? If pseudocode, consider marking explicitly (e.g., // Pseudocode: comment) to set reader expectations.

---

1000-1099: Recommendation for Strategy E is clear but could explicitly address the "Why not A?" question.

Section 7 recommends Strategy E but doesn't explicitly state why Strategy A wasn't chosen. The cost/benefit analysis (section 4) favors E on performance, but the decision section could strengthen this by saying something like:

"Strategy A was rejected not due to safety concerns (both are safe) but due to unnecessary complexity. Strategy E achieves the same correctness with 50% less code and zero additional latency. Strategy A would only be reconsidered if testing reveals N² worst-case writes are a practical problem (not expected with typical tables)."

This would help readers understand the decision was optimization-based, not safety-based.

ss2.md (2)

76-92: Example use of "Step 1-3" headers: Use markdown heading instead of bold emphasis.

Lines 76-92 show headers using bold (**Step 1: ConstructParts Factory**) instead of markdown headings (### Step 1: ConstructParts Factory). This is flagged by static analysis at line 76 (MD036). While both render, using proper headings improves document structure and navigation.

---

251-281: Parts Vector restoration explanation is clear but could add real-world troubleshooting guidance.

The explanation at lines 246-281 clearly shows that Parts vector is NOT restored after crash, only active TTxState entries are restored. This is important for developers to understand.

Consider adding a troubleshooting section: "If your operation restarts after crash and parts don't resume, check that all context is stored in TTxState, not just in part-local memory."

strategy_e_implementation_research.md (4)

1-65: Comprehensive implementation research, but documents needs structure improvements for readability.

This is an excellent, detailed implementation guide for Strategy E lock-free helping coordination. The amount of content (1770 lines) is impressive and thorough. However, consider:

1. Table of Contents: Add at top for navigation (readers need to know what's covered)
2. Quick Start: Add pointer for developers who want to just implement (vs. understand every detail)
3. Prerequisites: Section on what readers should know (lock-free algorithms? YDB internals?)

These additions would make the document more usable without reducing its depth.

---

30-46: Verification section is valuable but could be more explicit about verification methodology.

Lines 30-46 show "VERIFICATION STATUS: All file locations, classes, and functions verified" but doesn't explain how verification was done. Was this:

• Manual code inspection?
• Automated grep/ast-grep search?
• Reference to specific commit hashes?

Adding this would help readers assess confidence level and reproduce verification if needed.

---

238-294: Critical "Why Current Sync Fails" section is excellent for understanding the problem.

Lines 240-294 clearly explain the race condition in existing code. The timeline format (T1-T8) makes the problem concrete and visual. This is excellent documentation that should definitely be preserved in final design doc.

However, one improvement: Add a "This bug manifests as:" section explaining what users see (SCHEME_CHANGED errors? Version mismatches in query results?). Currently the document explains the technical issue but not the user-visible symptoms.

---

1088-1150: Step-by-step integration guide is extremely helpful, but could be more specific about testing.

Phase 4 (lines 1164-1202) recommends adding diagnostic tests but leaves specifics to the reader. Consider providing:

1. Actual test code template they can fill in
2. Specific assertions (not just "verify all at same version")
3. How to simulate concurrent CDC operations in test

Without these specifics, developers may write tests that don't actually catch race conditions.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

ydb/core/tx/datashard/datashard_ut_incremental_backup.cpp (2)

571-596: Consider making version diagnostics conditional.

The diagnostics block outputs detailed schema version information to stderr unconditionally. While helpful for debugging, this creates verbose test output.

Consider wrapping this in a conditional based on a test verbosity flag or only enabling it on test failure:

-        // Add version diagnostics after backup
-        {
-            Cerr << "========== VERSION DIAGNOSTICS AFTER BACKUP ==========" << Endl;
-            
-            auto describeTable = Ls(runtime, edgeActor, "/Root/TableWithIndex");
-            if (!describeTable->ResultSet.empty() && describeTable->ResultSet[0].TableId) {
-                Cerr << "Main table SchemaVersion: " << describeTable->ResultSet[0].TableId.SchemaVersion << Endl;
+        // Add version diagnostics after backup for debugging
+        if constexpr (false) {  // Enable for debugging
+            Cerr << "========== VERSION DIAGNOSTICS AFTER BACKUP ==========" << Endl;
+            
+            auto describeTable = Ls(runtime, edgeActor, "/Root/TableWithIndex");

---

3091-3091: Consider explicit CDC stream initialization verification.

The tests use fixed sleep times (5 seconds) after full backups to allow CDC streams to initialize. While SimulateSleep is deterministic in tests, consider whether there are explicit synchronization points that could be used instead of time-based waits.

This is not critical since the current approach works, but for improved clarity and maintainability, consider:

• Adding a helper function to verify CDC stream creation status
• Using event-based waiting similar to WaitTxNotification

Example pattern:

// Instead of:
SimulateSleep(server, TDuration::Seconds(5));

// Consider:
WaitForCdcStreamReady(server, edgeActor, "/Root/TableWithIndex");

This would make the tests more self-documenting and robust.

Also applies to: 3329-3329

ydb/core/tx/schemeshard/schemeshard__operation_common_cdc_stream.cpp (2)

117-274: Consider breaking down the function for improved maintainability.

HelpSyncSiblingVersions is 159 lines long and performs 6 distinct steps. While the current implementation is clear with step-by-step comments, extracting helper functions for collection (steps 1-2) and update (steps 4-5) logic could improve testability and maintainability.

For example:

• CollectIndexFamily() - Steps 1-2
• ComputeMaxVersion() - Step 2
• UpdateIndexEntities() - Steps 4-5

---

373-373: Minor: Redundant namespace qualification.

The call NCdcStreamState::SyncIndexEntityVersion() is already within the NCdcStreamState namespace (line 9), so the namespace prefix is redundant. You can call SyncIndexEntityVersion() directly for cleaner code.

-        NCdcStreamState::SyncIndexEntityVersion(childPathId, targetVersion, operationId, context, db);
+        SyncIndexEntityVersion(childPathId, targetVersion, operationId, context, db);

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro