Skip to content

Revert "Revert "rework pushing to views #77309""#79963

Merged
CheSema merged 10 commits intomasterfrom
revert-79898-revert-pushing-to-views
May 16, 2025
Merged

Revert "Revert "rework pushing to views #77309""#79963
CheSema merged 10 commits intomasterfrom
revert-79898-revert-pushing-to-views

Conversation

@CheSema
Copy link
Copy Markdown
Member

@CheSema CheSema commented May 8, 2025

Reverts #79898
Follow up for #77309

I will try to fix discovered errors here.

I hope I had found the reason of the crashes.
Thanks for TSan report here:
https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=77352&sha=80154a15c7a423a5ce7798a4b741273b5d38ef14&name_0=PR&name_1=Stateless%20tests%20%28tsan%2C%20s3%20storage%2C%201%2F3%29
When that race takes place double free is possible on string reallocation at exception message extension.

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

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

This reverts #79898 and restores changes from #77309

Documentation entry for user-facing changes

  • Documentation is written (mandatory for new features)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented May 8, 2025

Workflow [PR], commit [ac468ea]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 8, 2025
@CheSema CheSema marked this pull request as draft May 8, 2025 10:19
@clickhouse-gh clickhouse-gh bot added the submodule changed At least one submodule changed in this PR. label May 8, 2025
@CheSema
Copy link
Copy Markdown
Member Author

CheSema commented May 13, 2025

I do not know how to fix flaky check for
test_storage_s3_queue/test_3.py::test_upgrade_2[3-3]
test_storage_s3_queue/test_3.py::test_upgrade[3-3]
The instance with old version is failed with
DB::Exception: Unknown mark file extension: '4'
after it starts on the new data.

So I wont do it here.

@CheSema
Copy link
Copy Markdown
Member Author

CheSema commented May 13, 2025

test_storage_iceberg/test.py::test_types[s3-2]
data race in OPENSSL_sk_num #80160
#80160

@CheSema CheSema marked this pull request as ready for review May 15, 2025 16:41
@Michicosun Michicosun self-assigned this May 15, 2025
@CheSema
Copy link
Copy Markdown
Member Author

CheSema commented May 16, 2025

Integration tests (tsan, 3/6) , Integration tests (tsan, 6/6) -- tsan in SentryWriter, suspected PR is #80138, fix is comming

test_storage_s3_queue/test_3.py::test_upgrade_2[1-3]
test_storage_s3_queue/test_3.py::test_upgrade[1-3]
I do not fix this.

@CheSema CheSema added this pull request to the merge queue May 16, 2025
Merged via the queue into master with commit 8d68102 May 16, 2025
115 of 124 checks passed
{
view.runtime_stats->setStatus(QueryViewsLogElement::ViewStatus::QUERY_FINISH);

LOG_TRACE(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @CheSema,
I see that some users find this log useful for various debugging purposes. Do I understand correctly that it was removed prior to the view_duration_ms in system.query_views_log, which shows exactly the same value?

select_contexts[root_view] = init_context;
insert_contexts[root_view] = init_context;
input_headers[root_view] = init_header;
thread_groups[root_view] = CurrentThread::getGroup();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will likely not work for background processes, i.e. kafka pushes data to MV, and likely the reason for #86398

Copy link
Copy Markdown

@cleaton cleaton Aug 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to match our experience - our tables mentioned in that issue is connected to Kafka.

We tried reproducing locally manually inserting data to our views and the events did show up in the system table.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely will be fixed by #86422 as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo submodule changed At least one submodule changed in this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants