Conversation
|
This is an automated comment for commit 4b9a299 with description of existing statuses. It's updated for the latest CI running ✅ Click here to open a full report in a separate page Successful checks
|
65d6960 to
b41b770
Compare
|
Hi, thx for your work, it's really useful, When will this feature be released？ |
fe15caa to
c9cfcc3
Compare
809a43d to
1f7c7c5
Compare
There was a problem hiding this comment.
Looks great, as usual. Sorry I haven't looked at it earlier – I should start treating my backlog as a queue, not a stack.
Do you think it should be categorized as "Not for changelog" ? This PR has quite a few user-facing changes, such as new MV settings, IMO it should be classified as "improvement" and be reflected in the changelog.
| /// Drop the old table (outside the try-catch so we don't try to drop the other table if this fails). | ||
| InterpreterDropQuery::executeDropQuery(ASTDropQuery::Kind::Drop, view->getContext(), refresh_context, stale_table, /*sync*/ true, /*ignore_sync_setting*/ true); | ||
| if (table_to_drop.has_value()) | ||
| view->dropTempTable(table_to_drop.value(), refresh_context); |
There was a problem hiding this comment.
dropTempTable doesn't throw due to the internal try-catch block, yet the previous executeDropQuery may throw. Is this the intention? Also, I think the comment "(outside the try-catch..." is outdated.
There was a problem hiding this comment.
Removing outdated comment, leaving the inner try-catch, the idea is that it's better to consider the refresh successful in this case to avoid retries.
| executeRefreshUnlocked(append); | ||
| refreshed = true; | ||
| } | ||
| catch (...) |
There was a problem hiding this comment.
Wdyt about logging exponential backoff information (possibly, with the delay duration) in addition to "Refresh failed"?
|
Wait, this makes another incompatible change: Guess I'll add a compatibility setting for it. |
|
The integration test failure is probably unrelated to this PR. It's probably the thing @nikitamikhaylov explained to me where integration test flaky check reuses test fixture across tests, so if you touch an integration test that doesn't support fixture reuse you have to modify the test to support it. I can't easily do it here because I couldn't run the test locally: (return code 1 and empty std...out? err? idk what So I'll undo the |
… to add support for fixture reuse in test_replicated_database first
|
The bugs from #68154 have not been addressed |
|
I know, just this was very difficult to get past CI, so I merged it at first opportunity, fixes should be easier to merge. |
| {"use_hive_partitioning", false, false, "Allows to use hive partitioning for File, URL, S3, AzureBlobStorage and HDFS engines."}, | ||
| {"allow_experimental_kafka_offsets_storage_in_keeper", false, false, "Allow the usage of experimental Kafka storage engine that stores the committed offsets in ClickHouse Keeper"}, | ||
| {"allow_archive_path_syntax", true, true, "Added new setting to allow disabling archive path syntax."}, | ||
| {"allow_materialized_view_with_bad_select", true, true, "Support (but not enable yet) stricter validation in CREATE MATERIALIZED VIEW"}, |
There was a problem hiding this comment.
Thanks a lot for the clear descriptions. It helps a lot, seriously.
|
Hi! @al13n321 Are we expecting such behaviour? https://fiddle.clickhouse.com/b58974e0-089b-4c1d-81ca-5695da3f040c |
|
Yes, this PR added this sanity check, and it's not disabled by allow_materialized_view_with_bad_select. Previously such MV could be created, but wouldn't insert any rows into the target table: create table src (x Int8) engine Memory;
create materialized view dst (y Int8) engine Memory as select x from src;
insert into src values (10);
select * from dst;
SELECT *
FROM dst
Query id: 20708ba0-186c-4a82-b3da-15daf3d2301a
Ok.
0 rows in set. Elapsed: 0.001 sec.The correct query is |
|
Wow, thank you! |
Refreshable materialized views improvements
Refreshable materialized views improvements
24.8.14 Stable Backport of ClickHouse#58934: Refreshable materialized views improvements
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Refreshable materialized view improvements: append mode (
... REFRESH EVERY 1 MINUTE APPEND ...) to add rows to existing table instead of overwriting the whole table, retries (disabled by default, configured in SETTINGS section of the query),SYSTEM WAIT VIEW <name>query that waits for the currently running refresh, some fixes.Documentation entry for user-facing changes
A few things:
... REFRESH EVERY 1 MINUTE APPEND ...will add rows to existing table instead of overwriting the whole table. Can be used to write logs.SYSTEM WAIT VIEW <name>query that waits for the currently running refresh, rethrowing exception if it failed.weak_ptr<IStorage>because that's not allowed, and wasn't needed anyway.(Still no support for DatabaseReplicated.)
Closes #59095