unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
ClickHouse/ClickHouse
mid8/10
backwards-compatibilityconfiguration-managementerror-handlingtesting-strategy

Fix LOGICAL_ERROR in DatabaseReplicatedDDLWorker with max_replication_lag_to_enqueue=0

Project: ClickHouse/ClickHouse | Language: C++ | Concepts: backwards-compatibility, configuration-management, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/ClickHouse/ClickHouse/pull/106006

The problem

Setting max_replication_lag_to_enqueue = 0 for Replicated databases in ClickHouse caused the server to abort with a LOGICAL_ERROR in debug and sanitizer builds. This happened because a value of 0 made the post-recovery unsynced check always true, incorrectly marking a fully caught-up replica as unsynced, which then triggered an assertion failure.

Why it's non-trivial

A naive fix might only reject 0 in some user-facing code paths (like SQL CREATE DATABASE), but miss other sources where the setting can be supplied, such as server config files, metadata replay during database attach, or upgrades from persisted metadata. It's also easy to miss that old metadata or backups created by previous versions could still contain the problematic value, leading to startup or restore failures after an upgrade.

The approach

1. Change the setting type to enforce non-zero at parse time

The type of max_replication_lag_to_enqueue is switched from UInt64 to NonZeroUInt64 everywhere it is declared. This ensures that 0 is rejected as soon as the setting is parsed, regardless of the source (SQL, config, metadata, etc.).

-    DECLARE(UInt64, max_replication_lag_to_enqueue, 50, "Replica will throw exception on attempt to execute query if its replication lag greater", 0) \
+    DECLARE(NonZeroUInt64, max_replication_lag_to_enqueue, 50, "Replica will throw exception on attempt to execute query if its replication lag greater. Must be greater than 0", 0) \

What this does:
This macro is used to declare all Replicated database settings. Changing the type to NonZeroUInt64 means the parser will emit an error if 0 is supplied.

2. Update all code references to use the new type

All code that references this setting is updated to use the new type, ensuring consistency and type safety.

-    extern const DatabaseReplicatedSettingsUInt64 max_replication_lag_to_enqueue;
+    extern const DatabaseReplicatedSettingsNonZeroUInt64 max_replication_lag_to_enqueue;

What this does:
This updates the type in all relevant places, so the compiler enforces the invariant everywhere and any code that tries to use the old type will fail to compile.

3. Update documentation and test generators

The documentation table for supported settings is updated to clarify that 0 is not allowed, and the test property generator is changed so it never emits 0 for this setting.

-| `max_replication_lag_to_enqueue`                                             | 50                             | Replica will throw exception on attempt to execute query if its replication lag greater                                                                                                                                                                                                                                               |
+| `max_replication_lag_to_enqueue`                                             | 50                             | Replica will throw exception on attempt to execute query if its replication lag greater. Must be greater than `0`; `0` is rejected with `BAD_ARGUMENTS` (minimum is `1`)                                                                                                                                                              |

-            "max_replication_lag_to_enqueue": threshold_generator(0.2, 0.2, 0, 200),
+            "max_replication_lag_to_enqueue": threshold_generator(0.2, 0.2, 1, 200),

What this does:
Ensures documentation and test data generation are aligned with the new invariant.

4. Add a regression test to cover all entry points

A new stateless regression test is added to check that:

  • CREATE DATABASE ... SETTINGS max_replication_lag_to_enqueue = 0 is rejected.
  • ATTACH DATABASE ... SETTINGS max_replication_lag_to_enqueue = 0 is rejected (this covers metadata replay).
  • The smallest valid value (1) is accepted and works as expected.

Key snippet from the test script:

if ${CLICKHOUSE_CLIENT} -q "
    CREATE DATABASE ${db_zero}
    ENGINE = Replicated('/test/${CLICKHOUSE_DATABASE}/rdb_zero_lag_zero', 's1', 'r1')
    SETTINGS max_replication_lag_to_enqueue = 0
" 2>&1 | grep -q -F "A setting's value has to be greater than 0"
then
    echo "rejected"
else
    echo "NOT rejected (bug)"
fi

What this does:
Confirms that the parser now rejects 0 with a clear error message, and that the fix is enforced in all relevant code paths.

From the review thread

@PedroTadim: @groeneai the test 04298_98823_replicated_zero_lag_assertion failed, check

@groeneai: @PedroTadim thanks for catching this. The previous test ran the verbatim 3-statement reproducer with --ignore-error, which depended on a stale-read race between log queue entry creation and max_log_ptr updates. It tripped the chassert locally but completed cleanly in under a second on CI's Bugfix validation run (so the Bugfix validation correctly flagged "test passed on master HEAD, doesn't catch the regression").

Pushed 15799cd2 to switch to a deterministic check: read unsynced_after_recovery from system.clusters right after the replica activates. With the fix this is 0; on master HEAD without the fix it is 1 because max_log_ptr + 0 <= new_max_log_ptr is always true. A follow-up CREATE TABLE + INSERT + SELECT still exercises initAndCheckTask, which on master HEAD now reliably aborts the server with Logical error: 'our_log_ptr < max_log_ptr'.

Takeaways

  • Enforce invariants at the type or parse level, not just at runtime. Using a type like NonZeroUInt64 ensures that invalid values are rejected early, regardless of how the value is supplied.
  • Test all configuration entry points, including upgrades and metadata replay. Bugs can be triggered not only by direct user input but also by persisted settings from previous versions or server config files.
  • Update documentation and test generators alongside code changes. Keeping docs and test data in sync with code invariants prevents accidental reintroduction of invalid values and clarifies behavior for users.