Skip to content

Add error_message for deduplicated parts in system.part_log#80264

Merged
yariks5s merged 2 commits intoClickHouse:masterfrom
AVMusorin:show-error-message-deduplicated-part
Dec 2, 2025
Merged

Add error_message for deduplicated parts in system.part_log#80264
yariks5s merged 2 commits intoClickHouse:masterfrom
AVMusorin:show-error-message-deduplicated-part

Conversation

@AVMusorin
Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add error message that the part was deduplicated

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@yariks5s yariks5s self-assigned this May 15, 2025
@yariks5s yariks5s added the can be tested Allows running workflows for external contributors label May 15, 2025
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented May 15, 2025

Workflow [PR], commit [cb61057]

@clickhouse-gh clickhouse-gh bot added the pr-improvement Pull request with some product improvements label May 15, 2025
@yariks5s yariks5s enabled auto-merge May 19, 2025 16:43
@yariks5s yariks5s disabled auto-merge May 19, 2025 16:44
@yariks5s
Copy link
Copy Markdown
Member

I propose to check this new logic with an integration test because I have concerns about this check, and I'm sure that this check you've added will eventually become broken/flaky in case we have an extensive testing process, and current test will be at the end of the testing queue, it already happens sometimes:
Code: 158. DB::Exception: Limit for rows (controlled by 'max_rows_to_read' setting) exceeded, max rows: 20.00 million, current rows: 134.60 million. (TOO_MANY_ROWS) (version 25.4.1.36938 (official build)) (from [::1]:60320) (comment: 02124_insert_deduplication_token_replica.sql)

This is because we may have allocated a lot of logging entries in the table, and then we're unable to execute the query due to the CI restrictions. Checking this simple logic with an integration test can solve this problem.

@azat
Copy link
Copy Markdown
Member

azat commented May 22, 2025

I propose to check this new logic with an integration test because I have concerns about this check, and I'm sure that this check you've added will eventually become broken/flaky in case we have an extensive testing process, and current test will be at the end of the testing queue, it already happens sometimes:

It should be OK I think, the mentioned failure was caused by a different issue (besides we already have similar tests)
But we need to add filter by database here.

Also we need to add some changes for SMT likely

@AVMusorin AVMusorin force-pushed the show-error-message-deduplicated-part branch 2 times, most recently from d9befc4 to cb61057 Compare June 2, 2025 10:10
@AVMusorin AVMusorin requested a review from yariks5s June 10, 2025 10:02
@AVMusorin
Copy link
Copy Markdown
Contributor Author

@yariks5s could we please continue this PR?

@yariks5s
Copy link
Copy Markdown
Member

For me, it looks okay, maybe @azat has something to point out

@azat
Copy link
Copy Markdown
Member

azat commented Jul 16, 2025

Looks good, but the private should be adjusted as well

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Aug 19, 2025

Dear @yariks5s, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Oct 21, 2025

Dear @yariks5s, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

@AVMusorin
Copy link
Copy Markdown
Contributor Author

@yariks5s don't you mind to continue?

@AVMusorin AVMusorin force-pushed the show-error-message-deduplicated-part branch from cb61057 to 87f98ad Compare November 28, 2025 17:01
@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Nov 28, 2025

Workflow [PR], commit [8fb6932]

Summary:

job_name test_name status info comment
Stateless tests (arm_asan, targeted) failure
02117_custom_separated_with_names_and_types FAIL cidb
Stateless tests (amd_msan, parallel) failure
00816_long_concurrent_alter_column FAIL cidb, issue
BuzzHouse (amd_debug) failure
Logical error: 'Inconsistent AST formatting: the query: FAIL cidb, issue

@AVMusorin
Copy link
Copy Markdown
Contributor Author

@yariks5s Hey! just a friendly reminder about this PR

@yariks5s
Copy link
Copy Markdown
Member

yariks5s commented Dec 2, 2025

@AVMusorin could you please update with master? I've fixed the problem on our side

AVMusorin and others added 2 commits December 2, 2025 11:46
…plica

Fixes:

```
Code: 158. DB::Exception: Limit for rows (controlled by 'max_rows_to_read' setting) exceeded, max rows: 20.00 million, current rows: 134.60 million. (TOO_MANY_ROWS) (version 25.4.1.36938 (official build)) (from [::1]:60320) (comment: 02124_insert_deduplication_token_replica.sql)
```

Co-authored-by: Azat Khuzhin <a3at.mail@gmail.com>
@AVMusorin AVMusorin force-pushed the show-error-message-deduplicated-part branch from 87f98ad to 8fb6932 Compare December 2, 2025 12:20
@AVMusorin
Copy link
Copy Markdown
Contributor Author

02117_custom_separated_with_names_and_types FAIL

should be fixed in #91328

@yariks5s
Copy link
Copy Markdown
Member

yariks5s commented Dec 2, 2025

02117_custom_separated_with_names_and_types FAIL
should be fixed in #91328

Stateless tests (amd_msan, parallel) -- broken test

@yariks5s yariks5s added this pull request to the merge queue Dec 2, 2025
Merged via the queue into ClickHouse:master with commit b15c6d2 Dec 2, 2025
126 of 130 checks passed
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Dec 2, 2025
@AVMusorin AVMusorin deleted the show-error-message-deduplicated-part branch December 2, 2025 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-improvement Pull request with some product improvements pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants