Skip to content

bugfix to ensure resync cannot fail due to table size too large #4196

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jgao54 merged 4 commits into from
Apr 20, 2026
Merged

bugfix to ensure resync cannot fail due to table size too large #4196

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f266e69
bugfix to ensure resync cannot fail due to table size too large
jgao54 Apr 18, 2026
7b23e3b
review
jgao54 Apr 20, 2026
3810857
unit test
jgao54 Apr 20, 2026
89abfbf
lint
jgao54 Apr 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion flow/connectors/clickhouse/normalize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,16 @@ func (c *ClickHouseConnector) SetupNormalizedTable(
return false, fmt.Errorf("error while generating create table sql for destination ClickHouse table: %w", err)
}

qryCtx := ctx
if config.IsResync {
// CREATE OR REPLACE TABLE (used by resync) internally would create a NEW table with name
// <_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}))

@pfcoperez pfcoperez Apr 20, 2026 •
edited
Loading

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.

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?

@pfcoperez pfcoperez Apr 20, 2026

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.

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?

@ilidemi ilidemi Apr 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@jgao54 jgao54 Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@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

}
for _, sql := range normalizedTableCreateSQL {
if err := c.execWithLogging(ctx, sql); err != nil {
if err := c.execWithLogging(qryCtx, sql); err != nil {
return false, fmt.Errorf("[clickhouse] error while creating destination ClickHouse table: %w", err)
}
}
Expand Down
Loading