Fix the notify about commit logic in Column Shards #27104

kirillvasilenko · 2025-10-18T15:49:41Z

There is some renaming. It does not fixes the logic but makes it much easier to understand and harder to make a mistake.

The actual fix is in:

TReadMetadata, I added clear separation between "maybe conflicting writes" (writes that we may be conflicting but we have not checked for sure yet) and "conflicting writes" (writes that we have checked and found out they are conflicting). Previously, there was no way to get "maybe conflicting writes" but they are crucial for the notify about commit logic.
TOperationsManager::AddEventForLock(), there were a tricky bug. We check if there is a lock for conflicting tx and "tell" it to break the current tx, when it commits. If the lock was present, we did not check if it is committed already. And that was a mistake, because the lock may be present and its tx may be already committed. Now we check whether the conflicting tx is committed or not always.
TReadMetadata::DoOnBeforeStartReading(). Here we must pass "maybe conflicting writes" to evWriter and "ask" all the maybe conflicting txs to notify the given tx when they commit. We do not now which are really conflicting so we have to "ask" all of them.

github-actions · 2025-10-18T15:52:37Z

🟢 2025-10-18 18:10:02 UTC The validation of the Pull Request description is successful.

github-actions · 2025-10-18T15:53:35Z

⚪ 2025-10-18 15:53:34 UTC Pre-commit check linux-x86_64-relwithdebinfo for 7e9cffc has started.
⚪ 2025-10-18 15:53:48 UTC Artifacts will be uploaded here
⚪ 2025-10-18 15:57:36 UTC ya make is running...
🔴 2025-10-18 16:03:29 UTC Build failed, see the logs. Also see fail summary

github-actions · 2025-10-18T15:53:42Z

⚪ 2025-10-18 15:53:41 UTC Pre-commit check linux-x86_64-release-asan for 7e9cffc has started.
⚪ 2025-10-18 15:53:56 UTC Artifacts will be uploaded here
⚪ 2025-10-18 15:57:47 UTC ya make is running...
🔴 2025-10-18 16:02:49 UTC Build failed, see the logs. Also see fail summary

github-actions · 2025-10-18T17:53:10Z

⚪ 2025-10-18 17:53:09 UTC Pre-commit check linux-x86_64-relwithdebinfo for 67996ce has started.
⚪ 2025-10-18 17:53:23 UTC Artifacts will be uploaded here
⚪ 2025-10-18 17:56:58 UTC ya make is running...
🟡 2025-10-18 19:21:56 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
39067	36303	0	1	2743	20

⚪ 2025-10-18 19:22:04 UTC ya make is running... (failed tests rerun, try 2)
🟢 2025-10-18 19:39:45 UTC Tests successful.

Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
568 (only retried tests)	549	0	0	0	19

🟢 2025-10-18 19:39:48 UTC Build successful.
🟢 2025-10-18 19:40:02 UTC ydbd size 2.3 GiB changed* by -2.2 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `b09f809`	merge: `67996ce`	diff	diff %
ydbd size	2 426 358 584 Bytes	2 426 356 312 Bytes	-2.2 KiB	-0.000%
ydbd stripped size	516 361 128 Bytes	516 360 936 Bytes	-192 Bytes	-0.000%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2025-10-18T17:53:33Z

⚪ 2025-10-18 17:53:32 UTC Pre-commit check linux-x86_64-release-asan for 67996ce has started.
⚪ 2025-10-18 17:53:47 UTC Artifacts will be uploaded here
⚪ 2025-10-18 17:57:37 UTC ya make is running...
🟡 2025-10-18 20:06:21 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
15887	15354	0	219	294	20

🟢 2025-10-18 20:06:26 UTC Build successful.
🟢 2025-10-18 20:06:52 UTC ydbd size 3.8 GiB changed* by -8.6 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `b09f809`	merge: `67996ce`	diff	diff %
ydbd size	4 037 461 824 Bytes	4 037 453 048 Bytes	-8.6 KiB	-0.000%
ydbd stripped size	1 500 208 640 Bytes	1 500 202 624 Bytes	-5.9 KiB	-0.000%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

Copilot

Pull Request Overview

Fixes the “notify about commit” logic in Column Shards by clarifying lock-vs-tx semantics and ensuring commits of conflicting locks correctly trigger breaking and notifications.

Distinguishes “maybe conflicting writes” from confirmed “conflicting writes” in TReadMetadata and wires them into the read lifecycle.
Corrects AddEventForLock to handle cases where a conflicting lock may already be committed.
Renames txId-centric interfaces to lockId across interactions, readers, and event writers for clarity.

Reviewed Changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
ydb/core/tx/columnshard/transactions/locks/write.h	Switches interactions from txId to lockId; conflicts now map “selfLockId commits → others break”.
ydb/core/tx/columnshard/transactions/locks/read_start.h	Renames interaction method params to lockId; notification wiring for maybe-conflicting locks.
ydb/core/tx/columnshard/transactions/locks/read_start.cpp	Applies lockId naming and interval registration on add/remove.
ydb/core/tx/columnshard/transactions/locks/read_finished.h	Signature updated to lockId naming; behavior unchanged.
ydb/core/tx/columnshard/transactions/locks/interaction.h	Renames API to GetAffectedLockIds and aligns interval add/remove with lockId naming.
ydb/core/tx/columnshard/transactions/locks/abstract.h	Updates ITxEvent/ITxEventWriter interfaces from txId to lockId; event container now keyed by LockId.
ydb/core/tx/columnshard/operations/manager.h	Renames BrokeOnCommit → BreakOnCommit; adjusts commit notification API.
ydb/core/tx/columnshard/operations/manager.cpp	Fixes commit-handling logic; breaks conflicting locks and delivers commit notifications consistently.
ydb/core/tx/columnshard/engines/reader/simple_reader/iterator/source.cpp	Adapts to new read metadata methods (SetBreakLockOnReadFinished, SetWriteConflicting).
ydb/core/tx/columnshard/engines/reader/plain_reader/iterator/source.cpp	Same as above for plain reader.
ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h	Introduces maybe vs confirmed conflicting writes; renames and reorganizes counters/APIs.
ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.cpp	Wires maybe-conflicting lock IDs into pre-read, and confirmed conflicts into post-read logic.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

ydb/core/tx/columnshard/operations/manager.cpp

ydb/core/tx/columnshard/operations/manager.h

ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h

…ata ydb-platform#26463

github-actions · 2025-10-19T18:20:31Z

⚪ 2025-10-19 18:20:30 UTC Pre-commit check linux-x86_64-relwithdebinfo for d789420 has started.
⚪ 2025-10-19 18:20:44 UTC Artifacts will be uploaded here
⚪ 2025-10-19 18:24:19 UTC ya make is running...
🟡 2025-10-19 19:55:11 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
39067	36250	0	8	2780	29

⚪ 2025-10-19 19:55:19 UTC ya make is running... (failed tests rerun, try 2)
🟡 2025-10-19 20:13:28 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
1791 (only retried tests)	1733	0	1	37	20

⚪ 2025-10-19 20:13:32 UTC ya make is running... (failed tests rerun, try 3)
🔴 2025-10-19 20:34:40 UTC Some tests failed, follow the links below.

Ya make output | Test bloat | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
855 (only retried tests)	837	0	1	0	17

🟢 2025-10-19 20:34:43 UTC Build successful.
🟢 2025-10-19 20:35:01 UTC ydbd size 2.3 GiB changed* by -2.2 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `b09f809`	merge: `d789420`	diff	diff %
ydbd size	2 426 358 584 Bytes	2 426 356 312 Bytes	-2.2 KiB	-0.000%
ydbd stripped size	516 361 128 Bytes	516 360 936 Bytes	-192 Bytes	-0.000%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2025-10-19T18:20:44Z

⚪ 2025-10-19 18:20:43 UTC Pre-commit check linux-x86_64-release-asan for d789420 has started.
⚪ 2025-10-19 18:20:57 UTC Artifacts will be uploaded here
⚪ 2025-10-19 18:24:41 UTC ya make is running...

github-actions · 2025-10-20T12:26:20Z

⚪ 2025-10-20 12:26:19 UTC Pre-commit check linux-x86_64-release-asan for 17bcbaa has started.
⚪ 2025-10-20 12:26:34 UTC Artifacts will be uploaded here
⚪ 2025-10-20 12:30:38 UTC ya make is running...
🟡 2025-10-20 14:39:36 UTC Some tests failed, follow the links below. This fail is not in blocking policy yet

Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
15888	15443	0	194	235	16

🟢 2025-10-20 14:39:41 UTC Build successful.
🟢 2025-10-20 14:40:06 UTC ydbd size 3.8 GiB changed* by -4.6 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `8d15116`	merge: `17bcbaa`	diff	diff %
ydbd size	4 037 369 472 Bytes	4 037 364 800 Bytes	-4.6 KiB	-0.000%
ydbd stripped size	1 500 179 616 Bytes	1 500 177 696 Bytes	-1.9 KiB	-0.000%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

github-actions · 2025-10-20T12:31:38Z

⚪ 2025-10-20 12:31:37 UTC Pre-commit check linux-x86_64-relwithdebinfo for 17bcbaa has started.
⚪ 2025-10-20 12:32:00 UTC Artifacts will be uploaded here
⚪ 2025-10-20 12:36:23 UTC ya make is running...
🟡 2025-10-20 14:22:16 UTC Some tests failed, follow the links below. Going to retry failed tests...

Ya make output | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
39069	36274	0	8	2756	31

⚪ 2025-10-20 14:22:26 UTC ya make is running... (failed tests rerun, try 2)
🟢 2025-10-20 14:48:35 UTC Tests successful.

Ya make output | Test bloat | Test bloat

TESTS	PASSED	ERRORS	FAILED	SKIPPED	MUTED^?
822 (only retried tests)	797	0	0	0	25

🟢 2025-10-20 14:48:38 UTC Build successful.
🟢 2025-10-20 14:48:55 UTC ydbd size 2.3 GiB changed* by -2.3 KiB, which is <= 0 Bytes vs main: OK

ydbd size dash	main: `8d15116`	merge: `17bcbaa`	diff	diff %
ydbd size	2 426 304 400 Bytes	2 426 302 072 Bytes	-2.3 KiB	-0.000%
ydbd stripped size	516 341 288 Bytes	516 341 032 Bytes	-256 Bytes	-0.000%

^{*please be aware that the difference is based on comparing your commit and the last completed build from the post-commit, check comparation}

dorooleg · 2025-10-20T15:45:41Z

ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h


 private:
-    std::shared_ptr<TAtomicCounter> BrokenWithCommitted = std::make_shared<TAtomicCounter>();
+    mutable TAtomicCounter BreakLockOnReadFinished = TAtomicCounter();


mutable? Из-за избавления от shared_ptr?

Так точно. Там метод который это поле меняет const, и на этот const там полагаются вызывающие методы и т.д. В общем, я не стал все это править, просто локально упростил. shared_ptr убрал т.к. это внутренняя переменная, никуда мы ее не шарим, а сам класс не копируемый.

dorooleg · 2025-10-20T15:46:36Z

ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h

    }
-    THashSet<ui64> GetConflictableLockIds() const {
+
+    void SetBreakLockOnReadFinished() const {


А почему не Add или Inc?

Ну оно по смыслу не Add. Мы именно просим «сломай свой лок когда закончится чтение».

То что оно там add внутри - это детали реализации, не вижу смысла «показывать» их наружу.

Странно выглядит, что на самом деле это не Set

fix the notify about commit logic ydb-platform#26463

ef6c0ed

Kirill Vasilenko added 2 commits October 18, 2025 17:49

fix the notify about commit logic ydb-platform#26463

c0f4ff5

fix the notify about commit logic ydb-platform#26463

e837acf

kirillvasilenko requested a review from Copilot October 19, 2025 08:10

Copilot AI reviewed Oct 19, 2025

View reviewed changes

ydb/core/tx/columnshard/operations/manager.cpp Show resolved Hide resolved

ydb/core/tx/columnshard/operations/manager.h Show resolved Hide resolved

ydb/core/tx/columnshard/engines/reader/common_reader/constructor/read_metadata.h Show resolved Hide resolved

explicitly delete copy constructor/assignment operator for TReadMetad…

7ed0651

…ata ydb-platform#26463

kirillvasilenko requested a review from dorooleg October 20, 2025 06:55

dorooleg added the rebase-and-check Rebase PR with the current base branch and check label Oct 20, 2025

github-actions bot removed the rebase-and-check Rebase PR with the current base branch and check label Oct 20, 2025

kirillvasilenko marked this pull request as ready for review October 20, 2025 14:41

kirillvasilenko requested a review from a team as a code owner October 20, 2025 14:41

dorooleg reviewed Oct 20, 2025

View reviewed changes

dorooleg approved these changes Oct 20, 2025

View reviewed changes

kirillvasilenko merged commit fa78237 into ydb-platform:main Oct 21, 2025
21 of 23 checks passed

Fix the notify about commit logic in Column Shards #27104

Fix the notify about commit logic in Column Shards #27104

Uh oh!

Conversation

kirillvasilenko commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dorooleg Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

kirillvasilenko Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorooleg Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

kirillvasilenko Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

kirillvasilenko Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dorooleg Oct 20, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kirillvasilenko commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 18, 2025 •

edited

Loading

github-actions bot commented Oct 19, 2025 •

edited

Loading

github-actions bot commented Oct 19, 2025 •

edited

Loading

github-actions bot commented Oct 20, 2025 •

edited

Loading

github-actions bot commented Oct 20, 2025 •

edited

Loading

kirillvasilenko Oct 20, 2025 •

edited

Loading

kirillvasilenko Oct 20, 2025 •

edited

Loading