bugfix to ensure resync cannot fail due to table size too large#4196
Conversation
❌ 2 Tests Failed:
View the top 2 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
🔄 Flaky Test DetectedAnalysis: The e2e test ✅ Automatically retrying the workflow |
🔄 Flaky Test DetectedAnalysis: The test ✅ Automatically retrying the workflow |
pfcoperez
left a comment
There was a problem hiding this comment.
LGTM. Would it make sense to put together a small test case where the ClickHouse target is set to use a ridiculously small max_table_size_to_drop value by default and where we exercise this change?
| // <_tmp_replace_*>, atomic swap with the existing table, and then drop the existing table. | ||
| // Setting max_table_size_to_drop = 0 ensure the implicit drop can't fail, since it can be | ||
| // non-empty if a resync is triggered on top of another resync that is mid-snapshot. | ||
| qryCtx = clickhouse.Context(ctx, clickhouse.WithSettings(clickhouse.Settings{string(chinternal.SettingMaxTableSizeToDrop): 0})) |
There was a problem hiding this comment.
I was researching on this option and found that overriding max_table_size_to_drop setting at query time is available only after ClickHouse v23.12 --> https://clickhouse.com/docs/whats-new/changelog/2023#2312
I understand that someone using such an ancient version is a low-probability corner case. Anyway, even if that happens, the setting will be just ignored and they would not get any worse than the current situation, right?
There was a problem hiding this comment.
Actually, I was wrong in my assumption, I did run a little experiment:
SELECT COUNT(*) FROM table SETTINGS i_dont_exist=0;Setting i_dont_exist is neither a builtin setting nor started with the prefix 'custom_' registered for user-defined settings.
So I think this would indeed fail on older versions. Maybe we can add a version check to the condition?
There was a problem hiding this comment.
We have a whole framework for version-aware CH settings under https://github.com/PeerDB-io/peerdb/blob/main/flow/internal/clickhouse/settings.go. I think this logic should move to generateCreateTableSQLForNormalizedTable and be appended to SQL like the other cases we use settings
There was a problem hiding this comment.
@pfcoperez good call on checking ch version before adding it to the context.
@ilidemi i was worried that SETTINGS max_table_size_to_drop = 0 get applied to the CREATE TABLE as a table-level storage setting, but looks like clickhouse handles this automagically here
🔄 Flaky Test DetectedAnalysis: The failures are caused by a PostgreSQL catalog connection being externally terminated (SQLSTATE 57P01 admin_shutdown) during the test, which is a transient CI infrastructure issue, not a code regression. ✅ Automatically retrying the workflow |
|
@pfcoperez re: testing I ran this in clickhouse on a non-empty table with max_table_size_to_drop = 1 which repro-ed the error, and with max_table_size_to_drop = 0 it succeeded. For testing, this feels more like a clickhouse functionality test than peerdb test, but i realized we we don't have a unit test coverage for |
🔄 Flaky Test DetectedAnalysis: TestCancelTableAddition_NoRemovalAssumed failed due to a PostgreSQL catalog connection being forcibly terminated (SQLSTATE 57P01 / admin_shutdown), an infrastructure/network error rather than a code assertion failure, likely caused by a race condition in the parallel e2e test environment using Toxiproxy. ✅ Automatically retrying the workflow |
Scenario this change fixes:
a. create a new table with name tmp_replace*
b. exchange new table with old table
c. drop old table
max_table_size_to_drop, 3c fails and the tmp_replace* table is orphaned.The fix ensures drop would not fail because table is too large. Arguably cleaner if we make this a client-level setting, but safer to assess it on the per-query level since we are authorizing dropping 1TB+ table.
Fixes: DBI-698