diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f635f7b..927535d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,7 +5,7 @@ name: ci on: push: - branches: [main, dy/ch-migrate-0.6] + branches: [main, dy/ch-migrate-0.7] pull_request: workflow_call: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 70d3664..efff80a 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -8,13 +8,13 @@ name: release # PyPI publisher for project ch-migrate-cli: owner DRYCodeWorks, repository # ch-migrate, workflow release.yml, no environment. # -# This copy of the workflow is on the 0.6.x release branch, dy/ch-migrate-0.6. -# GitHub runs the workflow file from the tagged commit, so 0.6.x tags are +# This copy of the workflow is on the 0.7.x release branch, dy/ch-migrate-0.7. +# GitHub runs the workflow file from the tagged commit, so 0.7.x tags are # checked against this branch while tags on main keep main's own check. # -# To release 0.6.x: set __version__ in ch_migrate/__init__.py, commit, push -# dy/ch-migrate-0.6, then `git tag vX.Y.Z && git push origin vX.Y.Z`. Tags on -# commits that are not on dy/ch-migrate-0.6 are refused before anything builds. +# To release 0.7.x: set __version__ in ch_migrate/__init__.py, commit, push +# dy/ch-migrate-0.7, then `git tag vX.Y.Z && git push origin vX.Y.Z`. Tags on +# commits that are not on dy/ch-migrate-0.7 are refused before anything builds. on: push: @@ -24,7 +24,7 @@ permissions: contents: read env: - RELEASE_BRANCH: dy/ch-migrate-0.6 + RELEASE_BRANCH: dy/ch-migrate-0.7 jobs: on-release-branch: diff --git a/CHANGELOG.md b/CHANGELOG.md index ee91b0b..5d55207 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,38 @@ Changes are recorded in [Keep a Changelog](https://keepachangelog.com/en/1.1.0/) ## [Unreleased] +## [0.7.0] + +0.7 waits for the work a migration starts. `up` doesn't record a migration as applied until the mutations and `ON CLUSTER` DDL its statements started have finished, and it shows their progress while it waits. A normal `pip install -U` reaches it from 0.6, so read this before upgrading: + +1. **`up` now blocks until that work finishes, with no default timeout.** If a CI job's time limit stops it mid-wait, ClickHouse keeps working, and the next `up` waits for that work before it runs the migration again. `--timeout SECONDS` sets a limit of your own. +2. **A rerun runs the stopped migration again from its first statement, but only on its own when every statement passes the idempotency gate without a waiver.** An interrupted migration with a waived statement, which includes every `UPDATE`, `DELETE` and `INSERT ... SELECT`, stops the next run until you check what the earlier one changed and name it with `--rerun-interrupted REV`. CI jobs that retry `up` after a timeout need that step. +3. **The gate now reads more of your migrations.** SQL in `text("...")` and SQL sent through `op.get_bind().execute(...)` or `exec_driver_sql(...)` is checked, and SQL built at run time needs a waiver. `lint` and `up` can report migrations that passed before; waive them or keep them behind `lint.gate_baseline`. +4. **Two grants, only where `bootstrap` couldn't grant `SELECT ON system.*`**, such as on ClickHouse Cloud. As the admin user, replacing `` with your project name: + ```sql + GRANT CURRENT GRANTS(SELECT ON system.parts) TO _migration_role; + GRANT CURRENT GRANTS(SELECT ON system.replicas) TO _migration_role; + ``` + `system.parts` gives the progress bar its bytes and time estimate; without it the bar counts parts. `system.replicas` is read only for self-hosted replicated tables, so Cloud needs just the first. +5. **No `upgrade-env` this time.** The environment 0.6 wrote is current. `up` creates one small table, `_ch_migrate_in_progress`, in the project database. + +To stay on 0.6, pin `ch-migrate-cli==0.6.0`. + +### Changed + +- `up` waits before it records a migration as applied. After each statement that can write, it polls `system.mutations` on the migration session until the project database has no unfinished mutation on any replica that has to apply it, instead of holding a request open with `mutations_sync`. Mutations another writer started count too, and ones already queued are waited for before the first migration runs. The version table's own mutations stay with its 0.6 bookkeeping. +- `ON CLUSTER` statements wait inside ClickHouse: `up` sends them with `distributed_ddl_output_mode = 'throw'` and a limit of 270 seconds, or what's left of `--timeout` if that's less, so a host that fails or doesn't finish in time fails the migration with ClickHouse's message. +- A failed mutation fails the migration with ClickHouse's `latest_fail_reason` and prints the scoped `KILL MUTATION` statement for an operator; `ch-migrate` never kills a mutation itself. A failed mutation from before the run blocks it the same way. After a kill, fix the SQL and run `up` again, naming the revision with `--rerun-interrupted` if it's waived. +- `up` runs Alembic in its own process, so killing `up` can't leave a child process advancing the version table. `down` and a direct `alembic upgrade` go through the same environment and wait too; `--timeout` exists only on `ch-migrate up`. +- The idempotency gate checks literal SQL in `text("...")` and in `execute`/`exec_driver_sql` on any connection, requires a waiver for SQL that only exists at run time, and runs from `env.py` for a direct `alembic upgrade`. + +### Added + +- `up --timeout SECONDS`: one waiting budget for the whole run. When it runs out, `up` exits 1, names what is still pending and leaves the work running; the next `up` waits on it again, then runs the migration. +- Progress while `up` waits: in a terminal, a live bar per mutation weighted by bytes from `system.parts`, with parts and time left, and one row per replica. Piped output gets plain lines when a wait starts, at each 10% or every 30 seconds, and when it finishes. Without access to `system.parts` the bar counts parts only. +- `up --rerun-interrupted REV` and `down --rerun-interrupted REV` (`-x rerun_interrupted=REV` with `alembic`): run an interrupted migration again after checking what its earlier run changed. Each migration is noted in `_ch_migrate_in_progress` while it runs, created with the version table's engine. +- `bootstrap` grants `SELECT` on `system.replicas` and `system.parts`. + ## [0.6.0] 0.6 runs migrations on ClickHouse's official clickhouse-connect dialect in one session-safe connection, and stops a migration from running twice. A normal `pip install -U` reaches it from 0.5.x, so read this before upgrading: diff --git a/README.md b/README.md index e541b2a..3c9f2f7 100644 --- a/README.md +++ b/README.md @@ -23,8 +23,15 @@ This README describes the source checkout, which may be ahead of PyPI. To try an ## Upgrade from 0.x -0.6 requires Python 3.10+; 0.5.x is the last line supporting Python 3.9. -A loose requirement such as `ch-migrate-cli` or `~=0.5` upgrades to 0.6; pin +**From 0.6:** 0.7 needs no `upgrade-env`. `up` now waits for the mutations and +`ON CLUSTER` DDL its statements start (see [Migrations that wait](#migrations-that-wait)), +so give CI jobs time to finish or pass `--timeout`. Where `bootstrap` couldn't grant +`SELECT ON system.*`, such as on ClickHouse Cloud, add the grants listed in the +[CHANGELOG](CHANGELOG.md). A loose requirement such as `ch-migrate-cli` upgrades to +0.7; pin `ch-migrate-cli==0.6.0` to stay on 0.6. + +**From 0.5:** 0.6 and later require Python 3.10+; 0.5.x is the last line supporting Python 3.9. +A loose requirement such as `ch-migrate-cli` or `~=0.5` upgrades past 0.5; pin `ch-migrate-cli==0.5.1` until each project has run the command below. After upgrading the package, run this in each migration project: @@ -227,13 +234,111 @@ information or the single-server/Cloud default for version-table DDL. Review that DDL before using it for a self-hosted Replicated database. Cloud qualification is separate from the local suite; the local replicated harness has one shard. +### Migrations that wait + +`up` doesn't record a migration as applied until the work its statements started +has finished. After each statement that can write, it polls `system.mutations` on +the same checked ClickHouse session, so earlier `SET` statements stay in effect. +`op.execute`, raw `op.get_bind()` execution and `run_sql` all pass through that +check. Upgrades run in the CLI process, so killing it can't leave a child Alembic +runner advancing revisions. `down` waits the same way. + +```bash +ch-migrate up dev # No default waiting timeout +ch-migrate up dev --timeout 120 # One waiting budget across the invocation +``` + +What `up` waits for: + +- **Every unfinished mutation in the project database,** including one another + writer started. Before the first migration runs, `up` waits for any already + queued, and a failed one blocks the run. The version table's own mutations are + left to [its bookkeeping](#the-version-table). +- **Each replica that has to apply it.** Zero parts left isn't completion on its + own: the mutation must report `is_done`. A ReplicatedMergeTree mutation must be + finished on every replica, and a replica that hasn't received it yet is still + pending. Configure `cluster` for self-hosted replicated tables, or use a + Replicated database's automatic cluster; without either, `up` refuses a table + with more than one replica. SharedMergeTree on ClickHouse Cloud reports one + shared mutation. +- **`ON CLUSTER` DDL on every host.** `up` sends these statements with + `distributed_ddl_output_mode = 'throw'`, so ClickHouse answers once every host + has run the statement, and a host that fails or doesn't finish in time fails the + migration with ClickHouse's message. The limit is 270 seconds, below the + client's read timeout, or what's left of `--timeout` if that's less. Mutations + the statement starts are then waited for as above. + +Statements ClickHouse already holds until they finish, such as a type-changing +`MODIFY COLUMN`, a `MODIFY TTL` that materializes, or a lightweight `DELETE`, +still hold their request as they did in 0.6. + +While it waits, `up` shows progress on stderr. In a terminal, each mutation gets a +live bar weighted by bytes, with its parts and an estimate of the time left: + +```text +→ Applying 3f9c2a1b backfill_digest + ━━━━━━━━━━━━━━━━━━━━╺━━━━━━━━━━━━━━━━━ 18.2 / 41.0 GiB · 23/41 parts · 1:12 · ~1:30 left +✓ 3f9c2a1b backfill_digest mutation_7.txt · 41 parts · 41.0 GiB · 2:03 +``` + +Self-hosted replicated tables get one row per replica. When output isn't a +terminal (CI logs, pipes, coding agents), `up` prints plain lines instead: one +when a wait starts, one at each 10% of the parts or every 30 seconds, and one +when it finishes: + +```text +→ Applying 3f9c2a1b backfill_digest + waiting on mutation_7.txt (demo_dev.events): 0/41 parts + waiting on mutation_7.txt (demo_dev.events): 5/41 parts · 0:14 +✓ 3f9c2a1b backfill_digest mutation_7.txt · 41 parts · 2:03 +``` + +Part sizes come from `system.parts`. If the migration role can't read it, the bar +counts parts only and says so once; progress never changes whether `up` succeeds. + +**When `up` stops short.** After a timeout, Ctrl-C, dropped connection, killed +process or failed statement, ClickHouse keeps working and the migration isn't +recorded. `up` and `down` note each migration in `_ch_migrate_in_progress` when it +starts and clear the note once its version is recorded, so the next run knows the +migration was interrupted. That run waits for unfinished work, then: + +- **Runs the migration again on its own** when every statement in it passes the + idempotency gate without a waiver, so a repeat leaves the same result. +- **Stops before sending anything** when the migration has a waived statement or + SQL the gate can't read, because that statement may already have applied. It + names each one. Check what the earlier run changed, fix the SQL if needed, then + name the revision: `ch-migrate up ENV --rerun-interrupted REV` (or `down`), or + `-x rerun_interrupted=REV` with `alembic`. Naming it accepts the repeat. + +Every `UPDATE`, `DELETE` and `INSERT ... SELECT` needs a waiver, so an interrupted +backfill always takes that second path. + +**Failed mutation.** ClickHouse's `latest_fail_reason` stops the run, and `up` +prints the scoped `KILL MUTATION` statement for an operator; ch-migrate never +runs it. ClickHouse keeps retrying a failed mutation, and later mutations on that +table queue behind it. If the cause is temporary, such as memory or disk, fix it +and run `up` again. If the statement itself is wrong, kill the mutation, fix the +SQL and run `up` again, naming the revision with `--rerun-interrupted` if it's +waived. Killing a mutation doesn't undo the parts it already rewrote. + +Run one migration runner per database, using a CI concurrency key or an external +lock; the in-progress notes aren't a lock. Re-run `bootstrap` when upgrading a +restricted migration user to obtain the `system.replicas` and `system.parts` grants. + ### Re-runnable migrations `up` statically checks pending upgrade statements before Alembic executes any of them. Idempotency and standalone-SET errors refuse the whole run and print the source file, line, statement, and suggested fix. There is no command-line bypass; `--skip-mv-check` does not skip this gate. Other findings are warnings in `up`; standalone `lint` -retains their configured severities. +retains their configured severities. A direct `alembic upgrade` runs the same gate +from `env.py` before its first migration. + +The gate reads SQL in `run_sql` files and in literal strings, f-strings and +`text("...")` passed to `op.execute`, or to `execute` and `exec_driver_sql` on a +connection such as `op.get_bind()`. SQL that only exists at run time, such as a +variable, a built string or `run_sql(name)`, can't be checked, so it needs a +reasoned waiver like any other statement the gate can't vouch for. Static lint rejects a standalone `SET` when `migrations/env.py` lacks the `CH_MIGRATE_ENV_VERSION = 2` marker: that statement would be ignored on the old @@ -267,7 +372,7 @@ INSERT INTO {db}.target SELECT id FROM {db}.source; ``` For inline Python SQL, use the same directive in a `#` comment directly above -`op.execute`. Empty reasons remain errors. Waivers print as INFO with their +the call that sends it. Empty reasons remain errors. Waivers print as INFO with their reasons in `lint` and `up`; they do not make the statement idempotent. `MODIFY`, `MATERIALIZE`, `CLEAR COLUMN`, `TRUNCATE`, `OPTIMIZE`, `SYSTEM`, grants, revokes, `REPLACE PARTITION`, attaching or detaching a whole partition, and @@ -276,11 +381,12 @@ The reviewable classification corpus is `tests/corpus/idempotency.yaml`. **Recovery:** fix the cause of a partial failure, then run `ch-migrate up ENV` again. Earlier idempotent statements can run again; the version advances only -when the revision completes. On ClickHouse Cloud, a column that the failed attempt -dropped or renamed stays locked until that change's mutation finishes: a retry -before then fails with "affected by mutation ... which is not finished yet", even -with `IF EXISTS`. Wait for it in `system.mutations`, then run `up` again. Before retrying waived operations, review what the -earlier attempt actually changed. +when the revision completes. A dropped or renamed column stays locked until that +change's mutation finishes, and `up` waits for it before running the next +statement, so a retry after a later failure doesn't hit "affected by mutation ... +which is not finished yet". If `up` was stopped while it waited, run it again: it +waits for that mutation, then runs the revision again, unless the revision has a +statement the gate can't vouch for (see below). **Existing history:** `upgrade-env` records the current local script head(s) in `config.yaml` as `lint.gate_baseline`, preserving YAML comments. It records @@ -393,7 +499,7 @@ Example: `ch-migrate new dev add_status --table logs` ### `up` -`ch-migrate up ENV [-r REV] [--skip-mv-check] [--verbose]` applies migrations to `head` by default after the idempotency gate passes, printing one line per migration. `-r/--revision` selects a target. `--skip-mv-check` skips nonblocking materialized-view declaration checks, not the idempotency gate; use it only after reviewing those findings. If a migration fails, `up` names it, the SQL file, the statement and its line, and ClickHouse's error; `--verbose` adds the Python traceback. +`ch-migrate up ENV [-r REV] [--timeout SECONDS] [--skip-mv-check] [--verbose]` applies migrations to `head` by default after the idempotency gate passes, printing one line per migration. `-r/--revision` selects a target. `--timeout` is a finite positive waiting limit; omitting it waits without a deadline. `--skip-mv-check` skips nonblocking materialized-view declaration checks, not the idempotency gate; use it only after reviewing those findings. If a migration fails, `up` names it, the SQL file, the statement and its line, and ClickHouse's error; `--verbose` adds the Python traceback. Example: `ch-migrate up dev --revision abc123` @@ -507,7 +613,13 @@ The legacy `CH__PASSWORD` remains supported. Never commit credentials or pa ### Hooks -Top-level hooks run SQL on the migration connection. `pre_migrate` runs before the migration batch; `post_migrate` runs after each revision. `{db}` is substituted. Hooks execute as SQLAlchemy text, not through the SQL-file splitter; supply one statement per entry. Hook SQL is logged, so do not put secrets in it. +Top-level hooks run SQL on the migration connection. `pre_migrate` runs before the +migration batch; `post_migrate` runs after each revision body **before its version +is completed**, so hook mutations cannot escape the waiting barrier. A resumed +upgrade reuses its unfinished batch's receipts instead of repeating completed +hook writes. `{db}` is substituted. Hooks execute as SQLAlchemy text, not through +the SQL-file splitter; supply one statement per entry. Hook SQL is logged, so do +not put secrets in it. ```yaml hooks: diff --git a/ch_migrate/__init__.py b/ch_migrate/__init__.py index d1c9f6c..e01ef59 100644 --- a/ch_migrate/__init__.py +++ b/ch_migrate/__init__.py @@ -5,7 +5,7 @@ from ch_migrate import read_sql, get_db, get_env_config, create_dictionary """ -__version__ = "0.6.0" +__version__ = "0.7.0" from typing import Any diff --git a/ch_migrate/alembic_env.py b/ch_migrate/alembic_env.py index baca369..8d55bff 100644 --- a/ch_migrate/alembic_env.py +++ b/ch_migrate/alembic_env.py @@ -4,6 +4,7 @@ import logging import os +from functools import partial from logging.config import fileConfig from pathlib import Path from types import SimpleNamespace @@ -18,6 +19,7 @@ from sqlalchemy import URL, Column, MetaData, String, Table, create_engine, pool, text from sqlalchemy.sql.dml import Insert +from ch_migrate import ui from ch_migrate.config import get_env_config from ch_migrate.hooks import HookRegistry, run_hooks from ch_migrate.rebase import _literal_assignment @@ -28,6 +30,8 @@ inspect_version_table, sync_version_replica, ) +from ch_migrate.waiting import MigrationWaiter +from ch_migrate.waiting.reruns import RerunGuard ENV_VERSION = 2 DEFAULT_SESSION_TIMEOUT = 1800 @@ -96,18 +100,23 @@ def run() -> None: """Run the selected environment on one official-dialect session.""" root = Path.cwd() load_dotenv(root / ".env.local") - env_config = get_env_config(os.environ.get("CH_ENVIRONMENT", "dev"), root / "config.yaml") + # `ch-migrate up` runs Alembic in its own process and names the environment directly. + environment = context.config.attributes.get( + "ch_migrate_environment", os.environ.get("CH_ENVIRONMENT", "dev") + ) + env_config = get_env_config(environment, root / "config.yaml") os.environ["CH_DATABASE"] = env_config["database"] if env_config.get("cluster"): os.environ["CH_CLUSTER"] = env_config["cluster"] else: os.environ.pop("CH_CLUSTER", None) if context.config.config_file_name is not None: - fileConfig(context.config.config_file_name) + fileConfig(context.config.config_file_name, disable_existing_loggers=False) + _route_logs(context.config.attributes.get("ch_migrate_log_handler")) if context.is_offline_mode(): _run_offline(env_config) else: - _run_online(env_config) + _run_online(env_config, environment) def has_current_env(path: Path) -> bool: @@ -118,6 +127,18 @@ def has_current_env(path: Path) -> bool: ) +def _route_logs(handler: logging.Handler | None) -> None: + # `ch-migrate up` reports each migration in its own words, so Alembic's and + # ch-migrate's records go to its handler instead of alembic.ini's console handler. + if handler is None: + return + for name in ("alembic", "ch_migrate"): + named = logging.getLogger(name) + named.handlers = [handler] + named.propagate = False + named.setLevel(logging.INFO) + + def _run_offline(env_config: dict[str, Any]) -> None: context.configure( url=_url(env_config), @@ -135,7 +156,7 @@ def _run_offline(env_config: dict[str, Any]) -> None: context.run_migrations() -def _run_online(env_config: dict[str, Any]) -> None: +def _run_online(env_config: dict[str, Any], environment: str) -> None: # The session lives on whichever replica its connection reached: ClickHouse Cloud routes # each TCP connection, not each session id. A pool of its own keeps the session on one # connection instead of sharing the driver's default pool with other in-process clients. @@ -146,36 +167,56 @@ def _run_online(env_config: dict[str, Any]) -> None: ) try: with engine.connect() as connection: - _run_connection(connection, env_config) + _run_connection(connection, env_config, environment) finally: engine.dispose() session_pool.clear() -def _run_connection(connection, env_config: dict[str, Any]) -> None: +def _run_connection(connection, env_config: dict[str, Any], environment: str) -> None: hooks = HookRegistry.from_config(env_config.get("hooks")) - database = env_config["database"] + state = _checked_session(connection, env_config) + context.configure( + connection=connection, + target_metadata=None, + version_table="alembic_version", + version_table_schema=state.database, + on_version_apply=_post_migrate(hooks, state.database), + ch_migrate_version_state=state, + ) + waiter = MigrationWaiter(connection, state, context.config.attributes.get("ch_migrate_timeout")) + guard = _rerun_guard(waiter.client, state, environment) + try: + waiter.install(context.config) + with context.begin_transaction(): + # Work an earlier run left unfinished finishes before anything is sent again. + waiter.wait(before_run=True) + run_hooks( + connection, + hooks.pre_migrate, + db=state.database, + phase="pre_migrate", + revision="all", + ) + runtime = context.get_context() + runtime._migrations_fn = guard.steps(runtime._migrations_fn) + context.run_migrations() + finally: + waiter.close() + + +def _checked_session(connection, env_config: dict[str, Any]) -> VersionTableState: # Establish the session before requiring its existence on every later request. connection.exec_driver_sql("SELECT 1") client = connection.connection.dbapi_connection.client client.set_client_setting("session_check", 1) - state = inspect_version_table(client, database, env_config.get("cluster")) + state = inspect_version_table(client, env_config["database"], env_config.get("cluster")) assert_version_mutations_healthy(client, state) state = sync_version_replica(client, state) if warning := state.warning(): logger.warning(warning) connection.dialect.ddl_compiler = VersionTableDDLCompiler - context.configure( - connection=connection, - target_metadata=None, - version_table="alembic_version", - version_table_schema=database, - on_version_apply=_post_migrate(hooks, database), - ch_migrate_version_state=state, - ) - with context.begin_transaction(): - run_hooks(connection, hooks.pre_migrate, db=database, phase="pre_migrate", revision="all") - context.run_migrations() + return state def _url(env_config: dict[str, Any]) -> URL: @@ -210,3 +251,27 @@ def after_revision(ctx, step, heads, run_args): ) return after_revision + + +def _rerun_guard(client, state: VersionTableState, environment: str) -> RerunGuard: + attributes = context.config.attributes + # `ch-migrate up` passes --rerun-interrupted directly; `down` and a direct alembic + # command pass it as -x rerun_interrupted=REV[,REV]. + named = attributes.get("ch_migrate_rerun_interrupted") or context.get_x_argument( + as_dictionary=True + ).get("rerun_interrupted", "").split(",") + guard = RerunGuard(client, state, frozenset(name.strip() for name in named if name.strip())) + if not attributes.get("ch_migrate_gate_checked"): + # A direct `alembic upgrade` hasn't been through `ch-migrate up`'s gate. + guard.before_first_upgrade = partial(_enforce_gate, environment) + return guard + + +def _enforce_gate(environment: str) -> None: + from ch_migrate.gate import enforce_up_gate # lint imports this module + + try: + enforce_up_gate(Path.cwd(), environment) + except ui.Failure as failure: + failure.show() + raise SystemExit(1) from None diff --git a/ch_migrate/bootstrap.py b/ch_migrate/bootstrap.py index 67fe52d..5954db7 100644 --- a/ch_migrate/bootstrap.py +++ b/ch_migrate/bootstrap.py @@ -147,6 +147,8 @@ def build_bootstrap_sql( f"GRANT SELECT ON system.databases TO {project}_migration_role;", f"GRANT SELECT ON system.tables TO {project}_migration_role;", f"GRANT SELECT ON system.mutations TO {project}_migration_role;", + f"GRANT CURRENT GRANTS(SELECT ON system.replicas) TO {project}_migration_role;", + f"GRANT CURRENT GRANTS(SELECT ON system.parts) TO {project}_migration_role;", f"GRANT CURRENT GRANTS(SYSTEM SYNC REPLICA ON {db}.*) TO {project}_migration_role;", f"GRANT CURRENT GRANTS(SYSTEM SYNC DATABASE REPLICA ON {db}.*) TO {project}_migration_role;", f"GRANT CURRENT GRANTS(CLUSTER ON *.*) TO {project}_migration_role;", @@ -324,7 +326,9 @@ def run_bootstrap( secure = env_config.get("secure", True) port = env_config.get("port", 8443 if secure else 8123) - ui.step(f"Bootstrapping {env_name}: database {env_config['database']} on {host}, as {admin_user}") + ui.step( + f"Bootstrapping {env_name}: database {env_config['database']} on {host}, as {admin_user}" + ) ui.detail(f"Migration user: {migration_user}") if dict_reader_name: ui.detail(f"Dict reader: {dict_reader_name}") diff --git a/ch_migrate/cli.py b/ch_migrate/cli.py index 8866ce6..48ef4ef 100644 --- a/ch_migrate/cli.py +++ b/ch_migrate/cli.py @@ -2,6 +2,7 @@ from __future__ import annotations +import math import os import re import shutil @@ -42,41 +43,6 @@ def render_template(template_path: Path, **kwargs: str) -> str: return content -def _enforce_up_gate(environment: str, skip_mv_check: bool) -> None: - from ch_migrate.gate import lint_pending_up - - try: - report = lint_pending_up(Path.cwd(), environment, skip_mv_check) - except Exception as error: - raise ui.Failure(f"Migration preflight failed: {error}") from error - if _print_gate_findings(report): - raise ui.Failure( - "Migration gate failed; nothing was applied.", - "Fix SQL or add a reasoned in-file waiver.", - ) - - -def _print_gate_findings(report) -> bool: - from ch_migrate.display import lint_finding_line - from ch_migrate.lint import GATE_RULES, Severity - - blocked = False - for finding in report.results: - gate_error = finding.rule in GATE_RULES and finding.severity == Severity.ERROR - blocked |= gate_error - severity = finding.severity.value - if severity == "error" and not gate_error: - severity = "warn" - position = finding.file or "config.yaml" - if finding.line is not None: - position += f":{finding.line}" - console = ui.err if gate_error else ui.out - console.print(lint_finding_line(finding, location=position, severity=severity)) - if finding.statement: - ui.detail(finding.statement, stderr=gate_error) - return blocked - - def _require_current_env() -> None: from ch_migrate.alembic_env import has_current_env @@ -122,11 +88,15 @@ def _report_irreversible_range( summary = "The migration it would revert is irreversible:" else: verb = "is" if len(blocked) == 1 else "are" - summary = f"Of the {len(reasons)} migrations it would revert, {len(blocked)} {verb} irreversible:" + summary = ( + f"Of the {len(reasons)} migrations it would revert, {len(blocked)} {verb} irreversible:" + ) ui.error(f"Downgrade refused; nothing was run. {summary}") for rev, reason in reasons.items(): name = graph.migrations[rev].description or "" - marker, status = ("✗", f"irreversible: {reason}") if reason is not None else (" ", "reversible") + marker, status = ( + ("✗", f"irreversible: {reason}") if reason is not None else (" ", "reversible") + ) ui.detail(f"{marker} {rev[:8]} {name} ({status})", stderr=True) newest_blocked = blocked[0] above = list(reasons).index(newest_blocked) @@ -258,13 +228,33 @@ def bootstrap(environment: str, dry_run: bool, verbose: bool) -> None: @main.command() @click.argument("environment") @click.option("--revision", "-r", default="head", help="Revision to upgrade to (default: head)") +@click.option( + "--timeout", + type=click.FloatRange(min=0, min_open=True), + default=None, + help="Maximum total waiting seconds; default waits without a deadline", +) @click.option( "--skip-mv-check", is_flag=True, help="Skip materialized view declaration validation", ) +@click.option( + "--rerun-interrupted", + "rerun_interrupted", + multiple=True, + metavar="REVISION", + help="Run this interrupted revision again after checking what its earlier run changed", +) @click.option("--verbose", is_flag=True, help="Show the full traceback if a migration fails") -def up(environment: str, revision: str, skip_mv_check: bool, verbose: bool) -> None: +def up( + environment: str, + revision: str, + skip_mv_check: bool, + timeout: float | None, + rerun_interrupted: tuple[str, ...], + verbose: bool, +) -> None: """Apply pending migrations. Runs all unapplied migrations to bring the database to the latest version. @@ -272,24 +262,45 @@ def up(environment: str, revision: str, skip_mv_check: bool, verbose: bool) -> N Idempotency and standalone-SET gate errors refuse the run before Alembic. Other findings are warnings; --skip-mv-check only skips MV declaration checks. + A revision interrupted on an earlier run reruns on its own only if every statement + in it passes the gate without a waiver; otherwise name it with --rerun-interrupted. """ + if timeout is not None and not math.isfinite(timeout): + raise click.BadParameter("must be finite", param_hint="--timeout") _require_current_env() - _enforce_up_gate(environment, skip_mv_check) - sys.exit(run_migrations(environment, ["upgrade", revision], verbose=verbose)) + from ch_migrate.gate import enforce_up_gate + + enforce_up_gate(Path.cwd(), environment, skip_mv_check) + from ch_migrate.migration_runner import Upgrade, run_upgrade + + sys.exit(run_upgrade(Upgrade(environment, revision, timeout, verbose, rerun_interrupted))) @main.command() @click.argument("environment") @click.option("--revision", "-r", default="-1", help="Revision to downgrade to (default: -1)") +@click.option( + "--rerun-interrupted", + "rerun_interrupted", + multiple=True, + metavar="REVISION", + help="Run this interrupted revision again after checking what its earlier run changed", +) @click.option("--verbose", is_flag=True, help="Show the full traceback if a migration fails") -def down(environment: str, revision: str, verbose: bool) -> None: +def down( + environment: str, revision: str, rerun_interrupted: tuple[str, ...], verbose: bool +) -> None: """Rollback migrations. By default, rolls back the last migration. Use --revision to specify a target. + A downgrade interrupted on an earlier run reruns on its own only if every statement + in it passes the gate without a waiver; otherwise name it with --rerun-interrupted. """ _require_current_env() _refuse_irreversible_downgrade(environment, revision) - sys.exit(run_migrations(environment, ["downgrade", revision], verbose=verbose)) + # The downgrade runs in an Alembic subprocess; -x passes the names on to env.py. + named = ["-x", f"rerun_interrupted={','.join(rerun_interrupted)}"] if rerun_interrupted else [] + sys.exit(run_migrations(environment, [*named, "downgrade", revision], verbose=verbose)) @main.command() @@ -305,7 +316,9 @@ def status(environment: str) -> None: from ch_migrate.display import render_status state = _load_migration_state(environment) - render_status(environment, state.env_config, state.graph, state.applied, db_error=state.db_error) + render_status( + environment, state.env_config, state.graph, state.applied, db_error=state.db_error + ) if state.db_error: ui.warn(f"Could not reach the database: {state.db_error.strip().splitlines()[0]}") return @@ -816,8 +829,13 @@ def lint(environment: str | None) -> None: @main.command() @click.argument("environment") -@click.option("--validate", "-v", "validate_sql", type=click.Path(exists=True), - help="Validate a SQL file against the dependency graph") +@click.option( + "--validate", + "-v", + "validate_sql", + type=click.Path(exists=True), + help="Validate a SQL file against the dependency graph", +) def deps(environment: str, validate_sql: str | None) -> None: """Show materialized view and dictionary dependency graph. @@ -941,6 +959,7 @@ def diff_cmd(environment: str, snapshot_dir: str | None) -> None: TableDefinition, ViewDefinition, ) + fallback_types = { "table": lambda: TableDefinition(name=name, engine="", raw_ddl=ddl), "view": lambda: ViewDefinition(name=name, select_query="", raw_ddl=ddl), diff --git a/ch_migrate/deps.py b/ch_migrate/deps.py index c355feb..a71a071 100644 --- a/ch_migrate/deps.py +++ b/ch_migrate/deps.py @@ -67,17 +67,21 @@ def validate_migration(sql: str, graph: DependencyGraph) -> list[MigrationWarnin data_flow_deps.append(edge.target) if schema_deps: - warnings.append(MigrationWarning( - severity="error", - message=f"DROP TABLE {table_name} breaks schema dependencies: {', '.join(schema_deps)}", - affected_objects=schema_deps, - )) + warnings.append( + MigrationWarning( + severity="error", + message=f"DROP TABLE {table_name} breaks schema dependencies: {', '.join(schema_deps)}", + affected_objects=schema_deps, + ) + ) if data_flow_deps: - warnings.append(MigrationWarning( - severity="warning", - message=f"DROP TABLE {table_name} breaks data flow to: {', '.join(data_flow_deps)}", - affected_objects=data_flow_deps, - )) + warnings.append( + MigrationWarning( + severity="warning", + message=f"DROP TABLE {table_name} breaks data flow to: {', '.join(data_flow_deps)}", + affected_objects=data_flow_deps, + ) + ) # Check DROP VIEW / DROP MATERIALIZED VIEW for m in _RE_DROP_VIEW.finditer(sql): @@ -86,11 +90,13 @@ def validate_migration(sql: str, graph: DependencyGraph) -> list[MigrationWarnin affected = graph.affected_by_drop(view_name) if affected: affected_names = [n.name for n in affected] - warnings.append(MigrationWarning( - severity="warning", - message=f"DROP VIEW {view_name} affects: {', '.join(affected_names)}", - affected_objects=affected_names, - )) + warnings.append( + MigrationWarning( + severity="warning", + message=f"DROP VIEW {view_name} affects: {', '.join(affected_names)}", + affected_objects=affected_names, + ) + ) # Check DROP DICTIONARY for m in _RE_DROP_DICT.finditer(sql): @@ -99,11 +105,13 @@ def validate_migration(sql: str, graph: DependencyGraph) -> list[MigrationWarnin affected = graph.affected_by_drop(dict_name) if affected: affected_names = [n.name for n in affected] - warnings.append(MigrationWarning( - severity="warning", - message=f"DROP DICTIONARY {dict_name} affects: {', '.join(affected_names)}", - affected_objects=affected_names, - )) + warnings.append( + MigrationWarning( + severity="warning", + message=f"DROP DICTIONARY {dict_name} affects: {', '.join(affected_names)}", + affected_objects=affected_names, + ) + ) return warnings diff --git a/ch_migrate/diff.py b/ch_migrate/diff.py index 1cd7ee8..f591b4c 100644 --- a/ch_migrate/diff.py +++ b/ch_migrate/diff.py @@ -52,48 +52,60 @@ def _compare_columns( remote_col = remote_map.get(name) if local_col and not remote_col: - diffs.append(FieldDiff( - field_name=f"column '{name}'", - local_value=local_col.type, - remote_value=None, - message=f"column '{name}' {local_col.type} is in the snapshot but not in the database", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}'", + local_value=local_col.type, + remote_value=None, + message=f"column '{name}' {local_col.type} is in the snapshot but not in the database", + ) + ) elif remote_col and not local_col: - diffs.append(FieldDiff( - field_name=f"column '{name}'", - local_value=None, - remote_value=remote_col.type, - message=f"column '{name}' {remote_col.type} is in the database but not in the snapshot", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}'", + local_value=None, + remote_value=remote_col.type, + message=f"column '{name}' {remote_col.type} is in the database but not in the snapshot", + ) + ) elif local_col and remote_col: if local_col.type != remote_col.type: - diffs.append(FieldDiff( - field_name=f"column '{name}' type", - local_value=local_col.type, - remote_value=remote_col.type, - message=f"column '{name}' type differs: {local_col.type} vs {remote_col.type}", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}' type", + local_value=local_col.type, + remote_value=remote_col.type, + message=f"column '{name}' type differs: {local_col.type} vs {remote_col.type}", + ) + ) if local_col.default_kind != remote_col.default_kind: - diffs.append(FieldDiff( - field_name=f"column '{name}' default_kind", - local_value=local_col.default_kind, - remote_value=remote_col.default_kind, - message=f"column '{name}' default kind differs", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}' default_kind", + local_value=local_col.default_kind, + remote_value=remote_col.default_kind, + message=f"column '{name}' default kind differs", + ) + ) if local_col.default_expr != remote_col.default_expr: - diffs.append(FieldDiff( - field_name=f"column '{name}' default_expr", - local_value=local_col.default_expr, - remote_value=remote_col.default_expr, - message=f"column '{name}' default expression differs", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}' default_expr", + local_value=local_col.default_expr, + remote_value=remote_col.default_expr, + message=f"column '{name}' default expression differs", + ) + ) if local_col.codec != remote_col.codec: - diffs.append(FieldDiff( - field_name=f"column '{name}' codec", - local_value=local_col.codec, - remote_value=remote_col.codec, - message=f"column '{name}' codec differs: {local_col.codec} vs {remote_col.codec}", - )) + diffs.append( + FieldDiff( + field_name=f"column '{name}' codec", + local_value=local_col.codec, + remote_value=remote_col.codec, + message=f"column '{name}' codec differs: {local_col.codec} vs {remote_col.codec}", + ) + ) return diffs @@ -104,51 +116,61 @@ def _compare_tables(local: TableDefinition, remote: TableDefinition) -> list[Fie # Engine if local.engine != remote.engine: - diffs.append(FieldDiff( - field_name="engine", - local_value=local.engine, - remote_value=remote.engine, - message=f"engine differs: {local.engine} vs {remote.engine}", - )) + diffs.append( + FieldDiff( + field_name="engine", + local_value=local.engine, + remote_value=remote.engine, + message=f"engine differs: {local.engine} vs {remote.engine}", + ) + ) # Columns diffs.extend(_compare_columns(local.columns, remote.columns)) # ORDER BY if local.order_by != remote.order_by: - diffs.append(FieldDiff( - field_name="order_by", - local_value=", ".join(local.order_by), - remote_value=", ".join(remote.order_by), - message=f"ORDER BY differs", - )) + diffs.append( + FieldDiff( + field_name="order_by", + local_value=", ".join(local.order_by), + remote_value=", ".join(remote.order_by), + message=f"ORDER BY differs", + ) + ) # PARTITION BY if local.partition_by != remote.partition_by: - diffs.append(FieldDiff( - field_name="partition_by", - local_value=local.partition_by, - remote_value=remote.partition_by, - message=f"PARTITION BY differs", - )) + diffs.append( + FieldDiff( + field_name="partition_by", + local_value=local.partition_by, + remote_value=remote.partition_by, + message=f"PARTITION BY differs", + ) + ) # TTL if local.ttl != remote.ttl: - diffs.append(FieldDiff( - field_name="ttl", - local_value=local.ttl, - remote_value=remote.ttl, - message=f"TTL differs", - )) + diffs.append( + FieldDiff( + field_name="ttl", + local_value=local.ttl, + remote_value=remote.ttl, + message=f"TTL differs", + ) + ) # Settings if local.settings != remote.settings: - diffs.append(FieldDiff( - field_name="settings", - local_value=str(local.settings), - remote_value=str(remote.settings), - message=f"SETTINGS differ", - )) + diffs.append( + FieldDiff( + field_name="settings", + local_value=str(local.settings), + remote_value=str(remote.settings), + message=f"SETTINGS differ", + ) + ) return diffs @@ -156,6 +178,7 @@ def _compare_tables(local: TableDefinition, remote: TableDefinition) -> list[Fie def _normalize_ddl(raw: str) -> str: """Normalize raw DDL for fallback string comparison.""" import re + s = raw.strip() s = re.sub(r"\s+", " ", s) return s @@ -164,12 +187,14 @@ def _normalize_ddl(raw: str) -> str: def _compare_raw_ddl(local_ddl: str, remote_ddl: str) -> list[FieldDiff]: """Fallback: normalized string comparison for unparseable objects.""" if _normalize_ddl(local_ddl) != _normalize_ddl(remote_ddl): - return [FieldDiff( - field_name="raw_ddl", - local_value=local_ddl[:200] if local_ddl else None, - remote_value=remote_ddl[:200] if remote_ddl else None, - message="DDL definition differs (raw comparison)", - )] + return [ + FieldDiff( + field_name="raw_ddl", + local_value=local_ddl[:200] if local_ddl else None, + remote_value=remote_ddl[:200] if remote_ddl else None, + message="DDL definition differs (raw comparison)", + ) + ] return [] @@ -200,18 +225,30 @@ def compare_schemas(local: Schema, live: Schema) -> list[SchemaDiff]: live_obj = live_objs.get(name) if local_obj and not live_obj: - results.append(SchemaDiff( - name=name, obj_type=obj_type, status=DiffStatus.LOCAL_ONLY, - )) + results.append( + SchemaDiff( + name=name, + obj_type=obj_type, + status=DiffStatus.LOCAL_ONLY, + ) + ) elif live_obj and not local_obj: - results.append(SchemaDiff( - name=name, obj_type=obj_type, status=DiffStatus.REMOTE_ONLY, - )) + results.append( + SchemaDiff( + name=name, + obj_type=obj_type, + status=DiffStatus.REMOTE_ONLY, + ) + ) else: # Both exist — compare field_diffs: list[FieldDiff] = [] - if obj_type == "table" and isinstance(local_obj, TableDefinition) and isinstance(live_obj, TableDefinition): + if ( + obj_type == "table" + and isinstance(local_obj, TableDefinition) + and isinstance(live_obj, TableDefinition) + ): field_diffs = _compare_tables(local_obj, live_obj) else: # Fallback to raw DDL comparison for views, MVs, dicts @@ -220,13 +257,21 @@ def compare_schemas(local: Schema, live: Schema) -> list[SchemaDiff]: field_diffs = _compare_raw_ddl(local_ddl, live_ddl) if field_diffs: - results.append(SchemaDiff( - name=name, obj_type=obj_type, - status=DiffStatus.MODIFIED, field_diffs=field_diffs, - )) + results.append( + SchemaDiff( + name=name, + obj_type=obj_type, + status=DiffStatus.MODIFIED, + field_diffs=field_diffs, + ) + ) else: - results.append(SchemaDiff( - name=name, obj_type=obj_type, status=DiffStatus.IN_SYNC, - )) + results.append( + SchemaDiff( + name=name, + obj_type=obj_type, + status=DiffStatus.IN_SYNC, + ) + ) return results diff --git a/ch_migrate/display.py b/ch_migrate/display.py index c90f915..3573282 100644 --- a/ch_migrate/display.py +++ b/ch_migrate/display.py @@ -326,7 +326,9 @@ def render_diff_report( remote_only = [d for d in diffs if d.status == DiffStatus.REMOTE_ONLY] if not (modified or local_only or remote_only): - console.print(Text.assemble(("✓ ", "green"), f"All {_count(len(in_sync), 'object')} in sync.")) + console.print( + Text.assemble(("✓ ", "green"), f"All {_count(len(in_sync), 'object')} in sync.") + ) return for d in modified: diff --git a/ch_migrate/gate.py b/ch_migrate/gate.py index 1492140..d28d9fe 100644 --- a/ch_migrate/gate.py +++ b/ch_migrate/gate.py @@ -4,13 +4,28 @@ from dotenv import load_dotenv +from ch_migrate import ui from ch_migrate.config import get_env_config, load_config from ch_migrate.connection import get_migration_state -from ch_migrate.lint import LintConfig, LintReport, Severity, lint_migrations +from ch_migrate.display import lint_finding_line +from ch_migrate.lint import GATE_RULES, LintConfig, LintReport, Severity, lint_migrations from ch_migrate.rebase import build_revision_graph from ch_migrate.statements import pending_revisions +def enforce_up_gate(project_root: Path, environment: str, skip_mv_check: bool = False) -> None: + """Print the pending upgrades' findings, and refuse the run if a gate rule failed.""" + try: + report = lint_pending_up(project_root, environment, skip_mv_check) + except Exception as error: + raise ui.Failure(f"Migration preflight failed: {error}") from error + if print_gate_findings(report): + raise ui.Failure( + "Migration gate failed; nothing was applied.", + "Fix SQL or add a reasoned in-file waiver.", + ) + + def lint_pending_up( project_root: Path, environment: str, skip_mv_check: bool = False ) -> LintReport: @@ -24,3 +39,21 @@ def lint_pending_up( env_config = get_env_config(environment, config_path) pending = pending_revisions(graph, get_migration_state(env_config).heads) return lint_migrations(versions, config=config, revisions=pending) + + +def print_gate_findings(report: LintReport) -> bool: + blocked = False + for finding in report.results: + gate_error = finding.rule in GATE_RULES and finding.severity == Severity.ERROR + blocked |= gate_error + severity = finding.severity.value + if severity == "error" and not gate_error: + severity = "warn" + position = finding.file or "config.yaml" + if finding.line is not None: + position += f":{finding.line}" + console = ui.err if gate_error else ui.out + console.print(lint_finding_line(finding, location=position, severity=severity)) + if finding.statement: + ui.detail(finding.statement, stderr=gate_error) + return blocked diff --git a/ch_migrate/introspect.py b/ch_migrate/introspect.py index 48a9c2b..d185d79 100644 --- a/ch_migrate/introspect.py +++ b/ch_migrate/introspect.py @@ -172,9 +172,7 @@ def affected_by_drop(self, name: str) -> list[ObjectNode]: _RE_SETTINGS = re.compile(r"SETTINGS\s+(.+)$", re.IGNORECASE | re.MULTILINE) _RE_CREATE_VIEW = re.compile( - r"CREATE\s+VIEW\s+(?:IF\s+NOT\s+EXISTS\s+)?" - r"(?:`?(\w+)`?\.)?`?(\w+)`?" - r"\s+AS\s+", + r"CREATE\s+VIEW\s+(?:IF\s+NOT\s+EXISTS\s+)?" r"(?:`?(\w+)`?\.)?`?(\w+)`?" r"\s+AS\s+", re.IGNORECASE, ) @@ -266,9 +264,7 @@ def _parse_single_column(line: str) -> ColumnDefinition | None: # Extract DEFAULT/MATERIALIZED/ALIAS default_kind = None default_expr = None - default_match = re.search( - r"\b(DEFAULT|MATERIALIZED|ALIAS)\s+(.+)$", rest, re.IGNORECASE - ) + default_match = re.search(r"\b(DEFAULT|MATERIALIZED|ALIAS)\s+(.+)$", rest, re.IGNORECASE) if default_match: default_kind = default_match.group(1).upper() default_expr = default_match.group(2).strip() @@ -481,15 +477,11 @@ def parse_create_dictionary(ddl: str) -> DictDefinition | None: source_type = source_match.group(1).lower() # Extract table from SOURCE(CLICKHOUSE(TABLE '...' DB '...')) - tbl_match = re.search( - r"TABLE\s+'([^']+)'", ddl[source_match.start() :], re.IGNORECASE - ) + tbl_match = re.search(r"TABLE\s+'([^']+)'", ddl[source_match.start() :], re.IGNORECASE) if tbl_match: source_table = tbl_match.group(1) - db_match = re.search( - r"DB\s+'([^']+)'", ddl[source_match.start() :], re.IGNORECASE - ) + db_match = re.search(r"DB\s+'([^']+)'", ddl[source_match.start() :], re.IGNORECASE) if db_match: source_db = db_match.group(1) @@ -548,9 +540,7 @@ def parse_create_statement( # --------------------------------------------------------------------------- -def list_objects( - client: Any, database: str, obj_type: str = "table" -) -> list[str]: +def list_objects(client: Any, database: str, obj_type: str = "table") -> list[str]: """List objects of a given type in a database. Args: @@ -575,16 +565,13 @@ def list_objects( }.get(obj_type, "1=1") result = client.query( - f"SELECT name FROM system.tables " - f"WHERE database = {{db:String}} AND {engine_filter}", + f"SELECT name FROM system.tables " f"WHERE database = {{db:String}} AND {engine_filter}", parameters={"db": database}, ) return [row[0] for row in result.result_rows] -def get_create_statement( - client: Any, database: str, name: str, obj_type: str = "table" -) -> str: +def get_create_statement(client: Any, database: str, name: str, obj_type: str = "table") -> str: """Get the CREATE statement for an object. Args: diff --git a/ch_migrate/lint.py b/ch_migrate/lint.py index 0769ec5..a0efddb 100644 --- a/ch_migrate/lint.py +++ b/ch_migrate/lint.py @@ -132,8 +132,7 @@ def check( client: Any | None = None, database: str | None = None, graph: RevisionGraph | None = None, - ) -> list[LintResult]: - ... + ) -> list[LintResult]: ... # --------------------------------------------------------------------------- @@ -142,31 +141,176 @@ def check( # Subset of CH reserved words that commonly collide with column names. # Full list is version-dependent; these are the most common traps. -_CH_RESERVED_WORDS = frozenset({ - "add", "after", "alias", "all", "alter", "and", "anti", "any", "array", - "as", "asc", "attach", "between", "both", "by", "case", "cast", "check", - "cluster", "collate", "column", "comment", "constraint", "create", - "cross", "cube", "current", "database", "databases", "date", "day", - "default", "delete", "desc", "describe", "detach", "dictionaries", - "dictionary", "distinct", "distributed", "drop", "else", "end", "engine", - "events", "except", "exists", "explain", "expression", "extract", "fetch", - "final", "first", "flush", "following", "for", "format", "from", "full", - "function", "global", "granularity", "group", "having", "hour", "if", - "ilike", "in", "index", "inject", "inner", "insert", "interval", "into", - "is", "join", "key", "kill", "last", "layout", "leading", "left", "like", - "limit", "live", "local", "logs", "materialize", "materialized", "max", - "merges", "min", "minute", "modify", "month", "move", "mutation", "no", - "not", "null", "nulls", "offset", "on", "optimize", "or", "order", - "outer", "outfile", "over", "partition", "populate", "preceding", - "primary", "prewhere", "projection", "quarter", "range", "reload", - "remove", "rename", "replace", "right", "rollup", "row", "rows", - "sample", "second", "select", "semi", "set", "settings", "show", - "source", "start", "stop", "system", "table", "tables", "temporary", - "test", "then", "ties", "timestamp", "to", "top", "totals", "trailing", - "trim", "truncate", "type", "unbounded", "union", "update", "use", - "using", "uuid", "values", "view", "volume", "watch", "week", "when", - "where", "window", "with", "year", -}) +_CH_RESERVED_WORDS = frozenset( + { + "add", + "after", + "alias", + "all", + "alter", + "and", + "anti", + "any", + "array", + "as", + "asc", + "attach", + "between", + "both", + "by", + "case", + "cast", + "check", + "cluster", + "collate", + "column", + "comment", + "constraint", + "create", + "cross", + "cube", + "current", + "database", + "databases", + "date", + "day", + "default", + "delete", + "desc", + "describe", + "detach", + "dictionaries", + "dictionary", + "distinct", + "distributed", + "drop", + "else", + "end", + "engine", + "events", + "except", + "exists", + "explain", + "expression", + "extract", + "fetch", + "final", + "first", + "flush", + "following", + "for", + "format", + "from", + "full", + "function", + "global", + "granularity", + "group", + "having", + "hour", + "if", + "ilike", + "in", + "index", + "inject", + "inner", + "insert", + "interval", + "into", + "is", + "join", + "key", + "kill", + "last", + "layout", + "leading", + "left", + "like", + "limit", + "live", + "local", + "logs", + "materialize", + "materialized", + "max", + "merges", + "min", + "minute", + "modify", + "month", + "move", + "mutation", + "no", + "not", + "null", + "nulls", + "offset", + "on", + "optimize", + "or", + "order", + "outer", + "outfile", + "over", + "partition", + "populate", + "preceding", + "primary", + "prewhere", + "projection", + "quarter", + "range", + "reload", + "remove", + "rename", + "replace", + "right", + "rollup", + "row", + "rows", + "sample", + "second", + "select", + "semi", + "set", + "settings", + "show", + "source", + "start", + "stop", + "system", + "table", + "tables", + "temporary", + "test", + "then", + "ties", + "timestamp", + "to", + "top", + "totals", + "trailing", + "trim", + "truncate", + "type", + "unbounded", + "union", + "update", + "use", + "using", + "uuid", + "values", + "view", + "volume", + "watch", + "week", + "when", + "where", + "window", + "with", + "year", + } +) # --------------------------------------------------------------------------- @@ -180,12 +324,8 @@ class DestructiveChangeRule(LintRule): name = "destructive_changes" default_severity = Severity.WARN - _RE_DROP_TABLE = re.compile( - r"\bDROP\s+TABLE\b", re.IGNORECASE - ) - _RE_DROP_COLUMN = re.compile( - r"\bDROP\s+COLUMN\b", re.IGNORECASE - ) + _RE_DROP_TABLE = re.compile(r"\bDROP\s+TABLE\b", re.IGNORECASE) + _RE_DROP_COLUMN = re.compile(r"\bDROP\s+COLUMN\b", re.IGNORECASE) def check(self, sql: str, **kwargs: Any) -> list[LintResult]: config = kwargs.get("config") or LintConfig() @@ -197,24 +337,28 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: file_path = kwargs.get("file_path") for match in self._RE_DROP_TABLE.finditer(sql): - line = sql[:match.start()].count("\n") + 1 - results.append(LintResult( - rule=self.name, - message="DROP TABLE is destructive and irreversible", - severity=severity, - file=file_path, - line=line, - )) + line = sql[: match.start()].count("\n") + 1 + results.append( + LintResult( + rule=self.name, + message="DROP TABLE is destructive and irreversible", + severity=severity, + file=file_path, + line=line, + ) + ) for match in self._RE_DROP_COLUMN.finditer(sql): - line = sql[:match.start()].count("\n") + 1 - results.append(LintResult( - rule=self.name, - message="DROP COLUMN is destructive and irreversible", - severity=severity, - file=file_path, - line=line, - )) + line = sql[: match.start()].count("\n") + 1 + results.append( + LintResult( + rule=self.name, + message="DROP COLUMN is destructive and irreversible", + severity=severity, + file=file_path, + line=line, + ) + ) return results @@ -233,6 +377,12 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: elif reason is not None: severity = Severity.INFO message = f"Idempotency waiver: {reason}" + elif not kwargs.get("readable", True): + message = ( + "This SQL only exists at run time, so ch-migrate can't check that running it " + "twice is safe. Pass it as a string literal, or put " + "# ch-migrate: allow-non-idempotent on the line above" + ) else: check = classify_idempotency(sql) if check.status == "ok": @@ -290,14 +440,16 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: for match in self._RE_COLUMN_DEF.finditer(sql): col_name = match.group(1) if col_name.lower() in _CH_RESERVED_WORDS: - line = sql[:match.start()].count("\n") + 1 - results.append(LintResult( - rule=self.name, - message=f"Column '{col_name}' is a ClickHouse reserved word", - severity=severity, - file=file_path, - line=line, - )) + line = sql[: match.start()].count("\n") + 1 + results.append( + LintResult( + rule=self.name, + message=f"Column '{col_name}' is a ClickHouse reserved word", + severity=severity, + file=file_path, + line=line, + ) + ) return results @@ -325,17 +477,19 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: for match in self._RE_DDL.finditer(sql): # Check if ON CLUSTER or {on_cluster} appears nearby - rest = sql[match.end():match.end() + 200] + rest = sql[match.end() : match.end() + 200] if not re.search(r"(?:ON\s+CLUSTER|{on_cluster})", rest, re.IGNORECASE): - line = sql[:match.start()].count("\n") + 1 + line = sql[: match.start()].count("\n") + 1 stmt_type = match.group(0).strip() - results.append(LintResult( - rule=self.name, - message=f"{stmt_type} without ON CLUSTER or {{on_cluster}} placeholder", - severity=severity, - file=file_path, - line=line, - )) + results.append( + LintResult( + rule=self.name, + message=f"{stmt_type} without ON CLUSTER or {{on_cluster}} placeholder", + severity=severity, + file=file_path, + line=line, + ) + ) return results @@ -386,17 +540,19 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: if result.result_rows: row_count = result.result_rows[0][0] if row_count > threshold: - line = sql[:match.start()].count("\n") + 1 - results.append(LintResult( - rule=self.name, - message=( - f"ALTER on '{table_name}' which has {row_count:,} parts " - f"(threshold: {threshold:,})" - ), - severity=severity, - file=file_path, - line=line, - )) + line = sql[: match.start()].count("\n") + 1 + results.append( + LintResult( + rule=self.name, + message=( + f"ALTER on '{table_name}' which has {row_count:,} parts " + f"(threshold: {threshold:,})" + ), + severity=severity, + file=file_path, + line=line, + ) + ) except Exception: pass @@ -449,28 +605,24 @@ def check(self, sql: str, **kwargs: Any) -> list[LintResult]: for table_name, match in tables_to_check: affected = dep_graph.affected_by_drop(table_name) if affected: - mv_names = [ - n.name for n in affected if n.obj_type == "materialized_view" - ] - dict_names = [ - n.name for n in affected if n.obj_type == "dictionary" - ] + mv_names = [n.name for n in affected if n.obj_type == "materialized_view"] + dict_names = [n.name for n in affected if n.obj_type == "dictionary"] if mv_names or dict_names: - line = sql[:match.start()].count("\n") + 1 + line = sql[: match.start()].count("\n") + 1 deps = [] if mv_names: deps.append(f"MVs: {', '.join(mv_names)}") if dict_names: deps.append(f"Dicts: {', '.join(dict_names)}") - results.append(LintResult( - rule=self.name, - message=( - f"'{table_name}' has dependent objects: {'; '.join(deps)}" - ), - severity=severity, - file=file_path, - line=line, - )) + results.append( + LintResult( + rule=self.name, + message=(f"'{table_name}' has dependent objects: {'; '.join(deps)}"), + severity=severity, + file=file_path, + line=line, + ) + ) return results @@ -605,6 +757,9 @@ def _lint_statement( for rule in rules: if isinstance(rule, MVDeclarationRule) or rule.get_severity(scope.config) == Severity.OFF: continue + # SQL that only exists at run time can't be parsed; only its waiver is checked. + if not statement.readable and not isinstance(rule, IdempotencyRule): + continue findings = rule.check( statement.sql, file_path=statement.source, @@ -614,6 +769,7 @@ def _lint_statement( graph=scope.graph, comments=statement.comments, session_safe=scope.session_safe, + readable=statement.readable, ) for finding in findings: finding.file = statement.source diff --git a/ch_migrate/migration_runner.py b/ch_migrate/migration_runner.py new file mode 100644 index 0000000..f444fc6 --- /dev/null +++ b/ch_migrate/migration_runner.py @@ -0,0 +1,250 @@ +"""Own the upgrade process so SIGKILL cannot leave a child advancing revisions. + +`ch-migrate up` runs Alembic in this process and reports in ch-migrate's own words, +as runner.py does for `down`: one line per migration, setup chatter dropped, and a +failure reduced to the migration, what went wrong and what to do next. Waiting +progress is drawn by a display the waiter receives through the Alembic config. +""" + +from __future__ import annotations + +import logging +import os +import re +import traceback +from contextlib import contextmanager +from dataclasses import dataclass +from pathlib import Path + +from alembic import command +from alembic.config import Config + +from ch_migrate import ui +from ch_migrate.runner import ( + _RUNNING, + _SELF_DESCRIBING, + _SETUP_CHATTER, + _Progress, + _report_success, + _start_migration, +) +from ch_migrate.sql import clean_database_error +from ch_migrate.waiting.display import WaitDisplay +from ch_migrate.waiting.errors import ( + FailedMutation, + MutationFailed, + WaitingError, + WaitTimeout, +) +from ch_migrate.waiting.reruns import InterruptedStep + +_LOCATION = re.compile( + r"^(?Pmigrations/sql/\S+) \(statement \d+ of \d+, line (?P\d+)\): " +) + + +@dataclass(frozen=True) +class Upgrade: + """One `ch-migrate up`: where, how far, how long to wait, what to rerun, what to show.""" + + environment: str + revision: str = "head" + timeout: float | None = None + verbose: bool = False + rerun_interrupted: tuple[str, ...] = () + + +def run_upgrade(upgrade: Upgrade) -> int: + """Apply migrations in this process, one reported line each; return the exit status.""" + progress, display = _Progress(), WaitDisplay() + config = _alembic_config(upgrade, progress, display) + # Project helpers intentionally read these during migration execution only. + saved = {name: os.environ.get(name) for name in ("CH_DATABASE", "CH_CLUSTER")} + try: + with _restored_loggers("alembic", "ch_migrate"): + command.upgrade(config, upgrade.revision) + except KeyboardInterrupt: + display.stop() + _report_interrupt(upgrade.environment, progress) + return 130 + except Exception as error: + display.stop() + _report_failure(upgrade, progress, error) + return 1 + finally: + _restore_environment(saved) + _report_success(upgrade.environment, upgrade.revision, progress) + return 0 + + +class _MigrationLog(logging.Handler): + """Turns Alembic's log records into ch-migrate's lines, as runner.py does for its output.""" + + def __init__(self, progress: _Progress, display: WaitDisplay) -> None: + super().__init__(logging.INFO) + self.progress, self.display = progress, display + + def emit(self, record: logging.LogRecord) -> None: + message = record.getMessage() + running = _RUNNING.match(message) + if running: + _start_migration(running, self.progress) + self.display.start_migration(self.progress.current) + elif message.startswith(_SETUP_CHATTER): + return + elif record.levelno >= logging.WARNING: + ui.warn(message) + else: + ui.detail(message) + + +def _alembic_config(upgrade: Upgrade, progress: _Progress, display: WaitDisplay) -> Config: + """The project's Alembic config, carrying what env.py needs from this invocation.""" + config = Config(str(Path.cwd() / "alembic.ini")) + config.attributes.update( + ch_migrate_environment=upgrade.environment, + ch_migrate_command="upgrade", + # `ch-migrate up` has already run the gate; env.py runs it only for a direct upgrade. + ch_migrate_gate_checked=True, + ch_migrate_rerun_interrupted=upgrade.rerun_interrupted, + ch_migrate_timeout=upgrade.timeout, + ch_migrate_display=display, + ch_migrate_log_handler=_MigrationLog(progress, display), + ) + return config + + +def _report_interrupt(environment: str, progress: _Progress) -> None: + ui.error(f"Interrupted{' during ' + progress.current if progress.current else ''}") + ui.hint( + "Work ClickHouse already started keeps running, and the migration is not recorded as " + f"applied. Run `ch-migrate up {environment}` again: it waits for that work to finish, " + "then runs the migration again, or stops first if a waived statement may already have " + "applied.", + stderr=True, + ) + + +def _report_failure(upgrade: Upgrade, progress: _Progress, error: Exception) -> None: + where = progress.current or "Upgrade" + waiting = _waiting_error(error) + if waiting is not None and (location := _LOCATION.match(str(error))): + where = f"{where} ({location['path']}:{location['line']})" + if isinstance(waiting, MutationFailed): + _report_mutation_failure(upgrade.environment, where, waiting.failure) + elif isinstance(waiting, WaitTimeout): + _report_timeout(upgrade.environment, where, waiting) + elif isinstance(waiting, InterruptedStep): + _report_interrupted_step(upgrade.environment, waiting) + elif waiting is not None: + ui.error(f"{where}: {waiting}") + else: + _report_statement_failure(upgrade, where, error) + + +def _report_mutation_failure(environment: str, where: str, failure: FailedMutation) -> None: + on_host = "" if failure.host == "shared" else f" on {failure.host}" + if failure.foreign: + ui.error( + f"{where} is blocked: mutation {failure.mutation} on {failure.table}, started " + f"before this run, failed{on_host}" + ) + else: + ui.error(f"{where} failed: mutation {failure.mutation} on {failure.table} failed{on_host}") + ui.detail(clean_database_error(failure.reason), stderr=True) + ui.hint( + "It was not recorded as applied. ClickHouse keeps retrying a failed mutation, and later " + f"mutations on {failure.table} wait behind it.", + stderr=True, + ) + ui.hint( + "If the cause is temporary, such as memory or disk, fix it and run " + f"`ch-migrate up {environment}` to wait for the retry.", + stderr=True, + ) + ui.hint( + f"If the statement itself is wrong, an operator stops it{on_host} with {failure.kill}, " + f"then fixes the SQL and runs `ch-migrate up {environment}` again.", + stderr=True, + ) + + +def _report_timeout(environment: str, where: str, timeout: WaitTimeout) -> None: + ui.error(f"Timed out after {timeout.elapsed:.1f}s waiting on {where}") + ui.detail(f"Pending: {timeout.pending}", stderr=True) + ui.hint( + "ClickHouse keeps doing the work, and the migration is not recorded as applied. " + f"Run `ch-migrate up {environment}` again: it waits for that work to finish, then runs " + "the migration again, or stops first if a waived statement may already have applied.", + stderr=True, + ) + + +def _report_interrupted_step(environment: str, stop: InterruptedStep) -> None: + revision = stop.step.revision[:8] + ui.error( + f"{revision} was interrupted on an earlier run, and ch-migrate can't vouch for " + "running it again" + ) + for item in stop.unvouched: + where = f"{item.statement.source}:{item.statement.line}" + ui.detail(f"{where} {item.why}: {item.statement.sql.splitlines()[0]}", stderr=True) + ui.hint( + "Some of its statements may already have applied. Check what the earlier run changed, " + f"then run `ch-migrate up {environment} --rerun-interrupted {revision}`.", + stderr=True, + ) + + +def _report_statement_failure(upgrade: Upgrade, where: str, error: Exception) -> None: + ui.error(f"{where} failed") + ui.detail(_reason(error), stderr=True) + ui.hint( + "It was not recorded as applied. Anything it ran before the failure stays in place, " + f"so make the SQL safe to re-run, then run `ch-migrate up {upgrade.environment}` again.", + stderr=True, + ) + if upgrade.verbose: + traceback.print_exception(error) + else: + ui.hint("Add `--verbose` to see the full traceback.", stderr=True) + + +def _reason(error: Exception) -> str: + lines = str(error).strip().splitlines() + message = clean_database_error(lines[0]) if lines else "" + kind = type(error).__name__ + if not message: + return kind + return message if kind in _SELF_DESCRIBING else f"{kind}: {message}" + + +def _waiting_error(error: BaseException) -> WaitingError | None: + current: BaseException | None = error + seen: set[int] = set() + while current is not None and id(current) not in seen: + if isinstance(current, WaitingError): + return current + seen.add(id(current)) + current = getattr(current, "orig", None) or current.__cause__ or current.__context__ + return None + + +@contextmanager +def _restored_loggers(*names: str): + saved = [(logging.getLogger(n), logging.getLogger(n).handlers[:]) for n in names] + states = [(logger.propagate, logger.level) for logger, _ in saved] + try: + yield + finally: + for (logger, handlers), (propagate, level) in zip(saved, states): + logger.handlers, logger.propagate = handlers, propagate + logger.setLevel(level) + + +def _restore_environment(saved: dict[str, str | None]) -> None: + for name, value in saved.items(): + if value is None: + os.environ.pop(name, None) + else: + os.environ[name] = value diff --git a/ch_migrate/mv_validate.py b/ch_migrate/mv_validate.py index 0ba25bf..50aaa6d 100644 --- a/ch_migrate/mv_validate.py +++ b/ch_migrate/mv_validate.py @@ -121,9 +121,7 @@ def _extract_create_date(path: Path) -> str | None: # Handles: {db}.table, db.table, table (no db prefix) # Handles backtick-quoted identifiers and comma-separated user lists _RE_GRANT_INSERT = re.compile( - r"GRANT\s+INSERT\s+ON\s+" - r"(?:(?:\{[^}]*\}|`?\w+`?)\.)?`?(\w+)`?" - r"\s+TO\s+(.+?)(?:;|\n|\Z)", + r"GRANT\s+INSERT\s+ON\s+" r"(?:(?:\{[^}]*\}|`?\w+`?)\.)?`?(\w+)`?" r"\s+TO\s+(.+?)(?:;|\n|\Z)", re.IGNORECASE, ) @@ -162,11 +160,7 @@ def _parse_user_list(users_str: str) -> list[str]: Handles backtick-quoted identifiers and strips whitespace. """ - return [ - u.strip().strip("`").lower() - for u in users_str.split(",") - if u.strip().strip("`") - ] + return [u.strip().strip("`").lower() for u in users_str.split(",") if u.strip().strip("`")] def _find_grant_inserts(content: str) -> list[tuple[str, str]]: @@ -203,8 +197,7 @@ def _find_row_policies(content: str) -> list[tuple[str, str]]: Returns list of (policy_name, table_name) tuples, lowercased. """ return [ - (m.group(1).lower(), m.group(2).lower()) - for m in _RE_CREATE_ROW_POLICY.finditer(content) + (m.group(1).lower(), m.group(2).lower()) for m in _RE_CREATE_ROW_POLICY.finditer(content) ] @@ -236,9 +229,7 @@ def _read_migration_sql_from_content(content: str, path: Path) -> str: """ sql_parts: list[str] = [] - for match in re.finditer( - r'op\.execute\(\s*(?:f?"""(.*?)"""|f?"([^"]*)")', content, re.DOTALL - ): + for match in re.finditer(r'op\.execute\(\s*(?:f?"""(.*?)"""|f?"([^"]*)")', content, re.DOTALL): sql_parts.append(match.group(1) or match.group(2) or "") for match in re.finditer(r'(?:read_sql|run_sql)\(\s*["\']([^"\']+)["\']', content): @@ -308,18 +299,13 @@ def _check_grant_inserts( target = decl.target_table.lower() for user in decl.inserting_users: - has_grant = any( - table == target and u == user.lower() for table, u in grant_inserts - ) + has_grant = any(table == target and u == user.lower() for table, u in grant_inserts) if not has_grant: errors.append( MVValidationError( file=file_name, mv_name=decl.mv_name, - message=( - f"Missing: GRANT INSERT ON {{db}}.{decl.target_table} " - f"TO {user}" - ), + message=(f"Missing: GRANT INSERT ON {{db}}.{decl.target_table} " f"TO {user}"), ) ) @@ -336,18 +322,13 @@ def _check_grant_selects( target = decl.target_table.lower() for user in decl.select_users: - has_grant = any( - table == target and u == user.lower() for table, u in grant_selects - ) + has_grant = any(table == target and u == user.lower() for table, u in grant_selects) if not has_grant: errors.append( MVValidationError( file=file_name, mv_name=decl.mv_name, - message=( - f"Missing: GRANT SELECT ON {{db}}.{decl.target_table} " - f"TO {user}" - ), + message=(f"Missing: GRANT SELECT ON {{db}}.{decl.target_table} " f"TO {user}"), ) ) @@ -365,8 +346,7 @@ def _check_source_rls( for user in decl.source_rls_users: has_policy = any( - table == source - and user.lower() in [u.strip() for u in users_str.split(",")] + table == source and user.lower() in [u.strip() for u in users_str.split(",")] for table, users_str in permissive_policies ) if not has_policy: @@ -395,8 +375,7 @@ def _check_target_rls( for policy_name in decl.target_rls_policies: has_policy = any( - table == target and policy_name.lower() in policy - for policy, table in row_policies + table == target and policy_name.lower() in policy for policy, table in row_policies ) if not has_policy: errors.append( @@ -531,24 +510,16 @@ def validate_mv_migrations( continue # Core check: GRANT INSERT on target for each inserting user - errors.extend( - _check_grant_inserts(decl, grant_inserts, file_name) - ) + errors.extend(_check_grant_inserts(decl, grant_inserts, file_name)) # Optional checks (only if the declaration includes them) if decl.select_users: - errors.extend( - _check_grant_selects(decl, grant_selects, file_name) - ) + errors.extend(_check_grant_selects(decl, grant_selects, file_name)) if decl.source_rls_users: - errors.extend( - _check_source_rls(decl, permissive_policies, file_name) - ) + errors.extend(_check_source_rls(decl, permissive_policies, file_name)) if decl.target_rls_policies: - errors.extend( - _check_target_rls(decl, row_policies, file_name) - ) + errors.extend(_check_target_rls(decl, row_policies, file_name)) return errors diff --git a/ch_migrate/scaffold.py b/ch_migrate/scaffold.py index bb18028..958e10f 100644 --- a/ch_migrate/scaffold.py +++ b/ch_migrate/scaffold.py @@ -31,9 +31,7 @@ def fetch_current_ddl(env_config: dict[str, Any], table_name: str) -> str | None return None -def find_dependent_dictionaries( - env_config: dict[str, Any], table_name: str -) -> list[str]: +def find_dependent_dictionaries(env_config: dict[str, Any], table_name: str) -> list[str]: """Find dictionaries that use this table as a source. Queries system.dictionaries to find any dictionary whose source @@ -91,9 +89,7 @@ def _make_shadow_ddl(ddl: str, table_name: str) -> str: return shadow -def generate_exchange_sql( - table_name: str, current_ddl: str | None = None -) -> str: +def generate_exchange_sql(table_name: str, current_ddl: str | None = None) -> str: """Generate the SQL file content for an EXCHANGE TABLES migration. This creates the shadow table DDL that the user should modify with diff --git a/ch_migrate/secrets.py b/ch_migrate/secrets.py index e90577e..b54a725 100644 --- a/ch_migrate/secrets.py +++ b/ch_migrate/secrets.py @@ -28,8 +28,7 @@ def _get_ssm_client(region: Optional[str] = None): # type: ignore[no-untyped-de import boto3 # type: ignore[import-not-found,import-untyped] except ImportError: raise ImportError( - "boto3 is required for SSM support. " - "Install with: pip install ch-migrate-cli[ssm]" + "boto3 is required for SSM support. " "Install with: pip install ch-migrate-cli[ssm]" ) if region: return boto3.client("ssm", region_name=region) diff --git a/ch_migrate/sql.py b/ch_migrate/sql.py index 6d98ed0..1c2f84e 100644 --- a/ch_migrate/sql.py +++ b/ch_migrate/sql.py @@ -41,7 +41,6 @@ class SqlStatementError(RuntimeError): """ - def run_sql(path: str, **values: Any) -> None: """Run every statement in a SQL file under migrations/sql/, one at a time. @@ -126,7 +125,16 @@ def clean_database_error(message: str) -> str: def _database_reason(exc: Exception) -> str: - """First line of a driver error, without the wrapper and server-version noise.""" + """Preserve operator instructions from wait failures; trim driver noise only.""" + from ch_migrate.waiting.errors import WaitingError + + current: BaseException | None = exc + seen: set[int] = set() + while current is not None and id(current) not in seen: + if isinstance(current, WaitingError): + return str(current) + seen.add(id(current)) + current = getattr(current, "orig", None) or current.__cause__ lines = str(exc).strip().splitlines() return clean_database_error(lines[0]) if lines else type(exc).__name__ diff --git a/ch_migrate/statements.py b/ch_migrate/statements.py index a5b416b..16e8455 100644 --- a/ch_migrate/statements.py +++ b/ch_migrate/statements.py @@ -22,10 +22,17 @@ class MigrationStatement: line: int comments: tuple[str, ...] direction: MigrationDirection + # False when the SQL only exists at run time; `sql` then holds the call's source. + readable: bool = True def migration_statements(path: Path) -> list[MigrationStatement]: - """Collect literal SQL in upgrade/downgrade bodies, preserving its source.""" + """Collect the SQL in upgrade/downgrade bodies, preserving its source. + + Literal SQL is returned as statements. A call that sends SQL known only at run time + (a variable, an expression) comes back as one unreadable statement, so callers can + insist on a waiver instead of passing it silently. + """ content = path.read_text() root = path.parent.parent if root.name == "migrations": @@ -45,7 +52,7 @@ def migration_statements(path: Path) -> list[MigrationStatement]: for call in calls: if _call_name(call) in ("read_sql", "run_sql"): result.extend(_file_statements(call, source, direction)) - elif _call_name(call) == "op.execute": + elif _sends_sql(call): result.extend(_inline_statements(call, source, direction)) return result @@ -79,14 +86,19 @@ def _call_name(call: ast.Call) -> str: return "" +def _sends_sql(call: ast.Call) -> bool: + """`op.execute`, and `execute`/`exec_driver_sql` on a connection such as `op.get_bind()`.""" + return isinstance(call.func, ast.Attribute) and call.func.attr in ("execute", "exec_driver_sql") + + def _file_statements( call: ast.Call, source: _Source, direction: MigrationDirection ) -> list[MigrationStatement]: - if not call.args or not isinstance(call.args[0], ast.Constant): - return [] - path_value = call.args[0].value + path_value = ( + call.args[0].value if call.args and isinstance(call.args[0], ast.Constant) else None + ) if not isinstance(path_value, str): - return [] + return [_unreadable(call, source, direction)] sql_path = source.path.parent.parent / "sql" / path_value statements = split_statements(sql_path.read_text()) filename = sql_path.relative_to(source.root).as_posix() @@ -103,7 +115,13 @@ def _inline_statements( return [] text = _literal_sql(call.args[0], source.content) if text is None: - return [] + # `op.execute(read_sql("file.sql"))`: the file's statements are collected from read_sql. + if isinstance(call.args[0], ast.Call) and _call_name(call.args[0]) in ( + "read_sql", + "run_sql", + ): + return [] + return [_unreadable(call, source, direction)] comments = _python_comments(source.lines, call.lineno) filename = source.path.relative_to(source.root).as_posix() result = [] @@ -114,7 +132,18 @@ def _inline_statements( return result +def _unreadable( + call: ast.Call, source: _Source, direction: MigrationDirection +) -> MigrationStatement: + code = ast.get_source_segment(source.content, call) or ast.unparse(call) + filename = source.path.relative_to(source.root).as_posix() + comments = _python_comments(source.lines, call.lineno) + return MigrationStatement(code, filename, call.lineno, comments, direction, readable=False) + + def _literal_sql(node: ast.AST, content: str) -> str | None: + if _is_text_call(node): + return _literal_sql(node.args[0], content) if isinstance(node, ast.Constant) and isinstance(node.value, str): return node.value if not isinstance(node, ast.JoinedStr): @@ -135,6 +164,16 @@ def _literal_sql(node: ast.AST, content: str) -> str | None: return "".join(pieces) +def _is_text_call(node: ast.AST) -> bool: + """SQLAlchemy's `text("...")` or `sa.text("...")` around the SQL.""" + if not isinstance(node, ast.Call) or len(node.args) != 1: + return False + func = node.func + return (isinstance(func, ast.Name) and func.id == "text") or ( + isinstance(func, ast.Attribute) and func.attr == "text" + ) + + def _python_comments(lines: list[str], lineno: int) -> tuple[str, ...]: comments = [] index = lineno - 2 diff --git a/ch_migrate/templates/bootstrap/init_users.sql b/ch_migrate/templates/bootstrap/init_users.sql index 200c03f..d62fa6e 100644 --- a/ch_migrate/templates/bootstrap/init_users.sql +++ b/ch_migrate/templates/bootstrap/init_users.sql @@ -25,6 +25,8 @@ GRANT SELECT ON system.grants TO {project}_migration_role; GRANT SELECT ON system.databases TO {project}_migration_role; GRANT SELECT ON system.tables TO {project}_migration_role; GRANT SELECT ON system.mutations TO {project}_migration_role; +GRANT CURRENT GRANTS(SELECT ON system.replicas) TO {project}_migration_role; +GRANT CURRENT GRANTS(SELECT ON system.parts) TO {project}_migration_role; GRANT CURRENT GRANTS(SYSTEM SYNC REPLICA ON {db}.*) TO {project}_migration_role; GRANT CURRENT GRANTS(SYSTEM SYNC DATABASE REPLICA ON {db}.*) TO {project}_migration_role; GRANT CURRENT GRANTS(CLUSTER ON *.*) TO {project}_migration_role; diff --git a/ch_migrate/ui.py b/ch_migrate/ui.py index e9809b4..803d98d 100644 --- a/ch_migrate/ui.py +++ b/ch_migrate/ui.py @@ -46,14 +46,14 @@ def show(self, file: IO[Any] | None = None) -> None: hint(line, stderr=True) -def step(message: str) -> None: +def step(message: str, *, stderr: bool = False) -> None: """Something is starting or in progress.""" - out.print(_marked("→ ", "cyan", message)) + (err if stderr else out).print(_marked("→ ", "cyan", message)) -def success(message: str) -> None: +def success(message: str, *, stderr: bool = False) -> None: """Something finished as intended.""" - out.print(_marked("✓ ", "green", message)) + (err if stderr else out).print(_marked("✓ ", "green", message)) def detail(message: str, *, stderr: bool = False) -> None: diff --git a/ch_migrate/waiting/__init__.py b/ch_migrate/waiting/__init__.py new file mode 100644 index 0000000..cbf3221 --- /dev/null +++ b/ch_migrate/waiting/__init__.py @@ -0,0 +1,13 @@ +"""Waiting for the work migrations start: mutations and ON CLUSTER DDL. + +`up` doesn't record a migration until the mutations and distributed DDL its statements +started have finished. `MigrationWaiter` (waiter.py) hooks the migration connection's +cursor events: before each write it bounds ClickHouse's own distributed DDL wait, and +after it, mutations.py waits until the database has no unfinished mutations on any +replica that must apply them. display.py shows the progress; budget.py holds the +invocation's `--timeout`. +""" + +from ch_migrate.waiting.waiter import MigrationWaiter + +__all__ = ["MigrationWaiter"] diff --git a/ch_migrate/waiting/budget.py b/ch_migrate/waiting/budget.py new file mode 100644 index 0000000..6da2baa --- /dev/null +++ b/ch_migrate/waiting/budget.py @@ -0,0 +1,47 @@ +"""One waiting deadline per invocation, and the poll between checks.""" + +from __future__ import annotations + +import time +from dataclasses import dataclass, field + +from ch_migrate.waiting.display import Progress, WaitDisplay +from ch_migrate.waiting.errors import WaitTimeout + + +@dataclass +class WaitBudget: + timeout: float | None = None + started: float = field(default_factory=time.monotonic) + poll_interval: float = 0.2 + display: WaitDisplay = field(default_factory=WaitDisplay) + + @property + def elapsed(self) -> float: + return time.monotonic() - self.started + + def check(self, pending: str) -> None: + if self.timeout is not None and self.elapsed >= self.timeout: + self.finish() + raise WaitTimeout( + f"Timed out after {self.elapsed:.1f}s; server work continues; " + f"migration incomplete. Pending: {pending}", + pending, + self.elapsed, + ) + + def pause(self, pending: str, progress: Progress | None = None) -> None: + """Show `progress` (or the pending text), then sleep one poll within the deadline.""" + self.check(pending) + self.display.update(progress if progress is not None else pending) + remaining = self.poll_interval + if self.timeout is not None: + remaining = min(remaining, max(0, self.timeout - self.elapsed)) + time.sleep(remaining) + self.check(pending) + + def complete(self, message: str) -> None: + self.display.complete(message) + + def finish(self) -> None: + self.display.stop() diff --git a/ch_migrate/waiting/display.py b/ch_migrate/waiting/display.py new file mode 100644 index 0000000..7dcbbf1 --- /dev/null +++ b/ch_migrate/waiting/display.py @@ -0,0 +1,265 @@ +"""What `up` shows while it waits for mutations. + +Waiters hand over what the server reports; this module only draws it, and nothing +here decides whether work is done. On a terminal each wait is a live bar on +stderr, weighted by bytes when part sizes are known and labelled with parts. Piped +output, for CI logs and agents, gets plain lines instead: one when a wait starts, +one at each 10% step or every 30 seconds, and one when it finishes. +""" + +from __future__ import annotations + +import time +from dataclasses import dataclass, field +from typing import Union + +from rich.console import Console, Group +from rich.live import Live +from rich.progress_bar import ProgressBar +from rich.table import Table +from rich.text import Text + +from ch_migrate import ui + +PIPED_INTERVAL = 30.0 +PIPED_STEPS = 10 +BAR_WIDTH = 38 + +# What a waiter reports: a mutation, or a plain status line. +Progress = Union["MutationProgress", str] + + +class WaitDisplay: + """Draws one wait at a time; `complete` prints its result line, `stop` clears it.""" + + def __init__(self, console: Console | None = None) -> None: + self.console = console or ui.err + self.migration = "" + self._wait: _Wait | None = None + self._live: Live | None = None + self._parts_only = False + + def start_migration(self, label: str) -> None: + self.migration = label + + @property + def wants_bytes(self) -> bool: + """Only the terminal bar shows bytes, so piped runs skip the size lookup.""" + return self._terminal and not self._parts_only + + def update(self, progress: Progress) -> None: + wait = self._wait_for(progress) + wait.observe(progress) + if self._terminal: + self._draw_live(wait) + else: + self._print_piped(wait) + + def parts_only(self, reason: str) -> None: + """Part sizes can't be read; say so once, then count parts.""" + if self._parts_only: + return + self._parts_only = True + self.console.print( + Text.assemble(("! ", "yellow"), (f"Progress counts parts only: {reason}", "yellow")) + ) + + def complete(self, fallback: str) -> None: + wait = self._wait + self.stop() + if wait is not None: + self.console.print( + Text.assemble(("✓ ", "green"), wait.summary(self.migration, fallback)) + ) + + def stop(self) -> None: + if self._live is not None: + self._live.stop() + self._live = None + self._wait = None + + def _wait_for(self, progress: Progress) -> _Wait: + key = _key(progress) + if self._wait is None or self._wait.key != key: + self.stop() + self._wait = _Wait(key, started=time.monotonic()) + return self._wait + + @property + def _terminal(self) -> bool: + # A dumb terminal can't redraw in place, so it gets the piped lines. + return self.console.is_terminal and not self.console.is_dumb_terminal + + def _draw_live(self, wait: _Wait) -> None: + if self._live is None: + self._live = Live(console=self.console, auto_refresh=False, transient=True) + self._live.start() + self._live.update(wait.renderable(), refresh=True) + + def _print_piped(self, wait: _Wait) -> None: + line = wait.piped_line(time.monotonic()) + if line is not None: + self.console.print(Text(" " + line)) + + +@dataclass(frozen=True) +class MutationProgress: + """The mutation a statement started, on every host that hasn't finished it.""" + + table: str + mutation: str + hosts: tuple[HostParts, ...] + + +@dataclass(frozen=True) +class HostParts: + """One host's remaining parts. `sizes` maps each remaining part to its bytes, if known.""" + + host: str + names: frozenset[str] + count: int + sizes: dict[str, int] | None = None + + @property + def bytes_left(self) -> int | None: + if self.sizes is None or not self.names.issubset(self.sizes): + return None + return sum(self.sizes[name] for name in self.names) + + +@dataclass +class _Wait: + key: str + started: float + latest: Progress = "" + parts_total: dict[str, int] = field(default_factory=dict) + bytes_total: dict[str, int] = field(default_factory=dict) + printed_at: float | None = None + printed_step: int = -1 + + def observe(self, progress: Progress) -> None: + self.latest = progress + if isinstance(progress, MutationProgress): + for host in progress.hosts: + self.parts_total[host.host] = max(self.parts_total.get(host.host, 0), host.count) + if host.host not in self.bytes_total and host.bytes_left is not None: + self.bytes_total[host.host] = host.bytes_left + + def renderable(self): + elapsed = _duration(time.monotonic() - self.started) + progress = self.latest + if isinstance(progress, str): + return Text(f" waiting: {progress} · {elapsed}", style="dim") + rows = [self._host_row(host, len(progress.hosts) > 1) for host in progress.hosts] + return Group(*rows) + + def piped_line(self, now: float) -> str | None: + step = int(self._fraction() * PIPED_STEPS) + due = self.printed_at is None or now - self.printed_at >= PIPED_INTERVAL + if not due and step <= self.printed_step: + return None + first = self.printed_at is None + self.printed_at, self.printed_step = now, step + elapsed = "" if first else f" · {_duration(now - self.started)}" + return self._piped_text() + elapsed + + def summary(self, migration: str, fallback: str) -> str: + elapsed = _duration(time.monotonic() - self.started) + progress = self.latest + if isinstance(progress, str): + return f"{migration or fallback} waited for {progress} · {elapsed}" + return f"{migration or progress.table} {self._mutation_totals(progress)} · {elapsed}" + + def _host_row(self, host: HostParts, named: bool): + total = self.parts_total.get(host.host, host.count) + parts = f"{total - host.count}/{total} parts" + left, size_total = host.bytes_left, self.bytes_total.get(host.host) + if left is None or not size_total: + return _bar_row(host.host if named else "", _bar(total - host.count, total), parts) + done = size_total - left + stats = ( + f"{_sizes(done, size_total)} · {parts} · {_duration(time.monotonic() - self.started)}" + ) + if eta := self._eta(done, left): + stats += f" · ~{eta} left" + return _bar_row(host.host if named else "", _bar(done, size_total), stats) + + def _eta(self, done: int, left: int) -> str: + elapsed = time.monotonic() - self.started + if done <= 0 or elapsed < 2: + return "" + return _duration(left / (done / elapsed)) + + def _fraction(self) -> float: + progress = self.latest + if isinstance(progress, str): + return 0.0 + total = sum(self.parts_total.values()) + left = sum(host.count for host in progress.hosts) + return (total - left) / total if total else 0.0 + + def _piped_text(self) -> str: + progress = self.latest + if isinstance(progress, str): + return f"waiting: {progress}" + total = sum(self.parts_total.values()) + done = total - sum(host.count for host in progress.hosts) + replicas = f" on {len(progress.hosts)} replicas" if len(progress.hosts) > 1 else "" + return f"waiting on {_named(progress.mutation)} ({progress.table}): {done}/{total} parts{replicas}" + + def _mutation_totals(self, progress: MutationProgress) -> str: + parts = sum(self.parts_total.values()) + text = f"{_named(progress.mutation)} · {parts} {'part' if parts == 1 else 'parts'}" + if self.bytes_total and len(self.bytes_total) == len(self.parts_total): + text += f" · {_size(sum(self.bytes_total.values()))}" + if len(self.parts_total) > 1: + text += f" · {len(self.parts_total)} replicas" + return text + + +def _key(progress: Progress) -> str: + if isinstance(progress, MutationProgress): + return f"mutation {progress.table} {progress.mutation}" + return "status" + + +def _bar_row(label: str, bar: ProgressBar, stats: str) -> Table: + row = Table.grid(padding=(0, 1)) + row.add_column(no_wrap=True) + row.add_column(width=BAR_WIDTH) + row.add_column(no_wrap=True) + row.add_row(Text(f" {label}".rstrip() if label else " "), bar, Text(stats)) + return row + + +def _bar(completed: float, total: float) -> ProgressBar: + return ProgressBar(total=max(total, 1), completed=min(completed, total), width=BAR_WIDTH) + + +def _named(mutation: str) -> str: + # MergeTree ids already read "mutation_7.txt"; replicated ones are bare numbers. + return mutation if mutation.startswith("mutation") else f"mutation {mutation}" + + +def _duration(seconds: float) -> str: + seconds = max(0, int(seconds)) + hours, rest = divmod(seconds, 3600) + minutes, secs = divmod(rest, 60) + return f"{hours}:{minutes:02d}:{secs:02d}" if hours else f"{minutes}:{secs:02d}" + + +def _sizes(done: int, total: int) -> str: + unit, scale = _unit(total) + return f"{done / scale:.1f} / {total / scale:.1f} {unit}" + + +def _size(total: int) -> str: + unit, scale = _unit(total) + return f"{total / scale:.1f} {unit}" + + +def _unit(total: int) -> tuple[str, int]: + for unit, scale in (("GiB", 1 << 30), ("MiB", 1 << 20), ("KiB", 1 << 10)): + if total >= scale: + return unit, scale + return "B", 1 diff --git a/ch_migrate/waiting/errors.py b/ch_migrate/waiting/errors.py new file mode 100644 index 0000000..311798a --- /dev/null +++ b/ch_migrate/waiting/errors.py @@ -0,0 +1,37 @@ +"""Why waiting stopped short: a deadline, a failed mutation, or a deployment it can't check.""" + +from __future__ import annotations + +from dataclasses import dataclass + + +class WaitingError(RuntimeError): + """Server work is unfinished; the revision must not be completed.""" + + +class WaitTimeout(WaitingError): + """The invocation's waiting deadline expired without cancelling server work.""" + + def __init__(self, message: str, pending: str, elapsed: float) -> None: + super().__init__(message) + self.pending, self.elapsed = pending, elapsed + + +class MutationFailed(WaitingError): + """ClickHouse reports `latest_fail_reason` for a mutation this run owns or waits behind.""" + + def __init__(self, message: str, failure: FailedMutation) -> None: + super().__init__(message) + self.failure = failure + + +@dataclass(frozen=True) +class FailedMutation: + """What the operator needs: where it failed, why, and the scoped KILL statement.""" + + table: str + mutation: str + host: str + reason: str + kill: str + foreign: bool = False diff --git a/ch_migrate/waiting/mutations.py b/ch_migrate/waiting/mutations.py new file mode 100644 index 0000000..00d5763 --- /dev/null +++ b/ch_migrate/waiting/mutations.py @@ -0,0 +1,264 @@ +"""Wait until a database has no unfinished mutations on any replica that has to apply them. + +The waiter doesn't track which statement started which mutation. After each write it +records the unfinished mutations the connected server reports, then polls until each one +is done everywhere it applies: on the service for SharedMergeTree, on every replica for +ReplicatedMergeTree (a replica that hasn't pulled the mutation yet is still pending), and +where it runs for anything else. Every unfinished mutation in the database counts, so a +migration also waits for, and is stopped by, another writer's work. +""" + +from __future__ import annotations + +import re +from dataclasses import dataclass, replace + +from clickhouse_connect.driver.exceptions import DatabaseError + +from ch_migrate.waiting import reruns +from ch_migrate.waiting.display import HostParts, MutationProgress +from ch_migrate.waiting.errors import FailedMutation, MutationFailed, WaitingError +from ch_migrate.waiting.sql import sql_string + +SHARED = "shared" +# ch-migrate's own tables: version_table.py waits on the version table's DELETE itself and +# refuses a stuck one before the run starts, and the rerun markers' DELETE waits the same way. +INTERNAL_TABLES = ("alembic_version", reruns.TABLE) + + +class MutationWaiter: + """Polls `system.mutations` on the migration session; never kills or resends anything.""" + + def __init__(self, client, state, budget): + self.client, self.state, self.budget = client, state, budget + self._hosts: dict[str, tuple[str, ...] | None] = {} + self._part_sizes: dict[tuple[str, str], int] = {} + + def wait(self, before_run: bool = False) -> None: + """Return once every unfinished mutation seen now has finished where it applies.""" + self._hosts.clear() + expected = {(row.table, row.mutation) for row in self._rows(set())} + while expected: + rows = self._rows(expected) + expected |= {(row.table, row.mutation) for row in rows if not row.done} + _raise_failures(self.state.database, rows, before_run) + pending = self._pending(expected, rows) + if not pending: + self.budget.complete(f"{self.state.database} mutations") + return + self.budget.pause(_pending_text(self.state.database, pending), self._snapshot(pending)) + + def _rows(self, expected: set[tuple[str, str]]) -> list[_Row]: + """Unfinished mutations in the database, and every row for the ones already expected.""" + cluster = self.state.health_cluster + source = ( + "clusterAllReplicas({cluster:String}, system.mutations)" + if cluster + else "system.mutations" + ) + rows = self.client.query( + "SELECT materialize(hostName()), table, mutation_id, is_done, parts_to_do, " + f"latest_fail_reason, parts_to_do_names FROM {source} WHERE database = {{db:String}} " + "AND NOT has({internal:Array(String)}, table) " + "AND (NOT is_done OR has({expected:Array(String)}, concat(table, '/', mutation_id)))", + parameters={ + "db": self.state.database, + "internal": list(INTERNAL_TABLES), + "cluster": cluster, + "expected": sorted(f"{table}/{mutation}" for table, mutation in expected), + }, + settings=_cluster_settings() if cluster else None, + ).result_rows + return [self._row(values) for values in rows] + + def _row(self, values: tuple) -> _Row: + host, table, mutation, done, parts, reason, names = values + # Every replica of a SharedMergeTree table reports the same shared mutation. + if self._required_hosts(table) == (SHARED,): + host = SHARED + return _Row(host, table, mutation, bool(done), parts, reason, frozenset(names)) + + def _pending(self, expected: set[tuple[str, str]], rows: list[_Row]) -> list[_Row]: + """Unfinished rows, plus a placeholder for each replica that hasn't reported one yet.""" + pending = [row for row in rows if not row.done] + seen = {(row.host, row.table, row.mutation) for row in rows} + for table, mutation in sorted(expected): + for host in self._required_hosts(table) or (): + if host != SHARED and (host, table, mutation) not in seen: + pending.append( + _Row(host, table, mutation, False, 0, "", frozenset(), missing=True) + ) + return pending + + def _required_hosts(self, table: str) -> tuple[str, ...] | None: + """Hosts that must each report a mutation done; None means wherever it appears.""" + if table not in self._hosts: + self._hosts[table] = self._resolve_hosts(table) + return self._hosts[table] + + def _resolve_hosts(self, table: str) -> tuple[str, ...] | None: + rows = self.client.query( + "SELECT engine FROM system.tables WHERE database = {db:String} AND name = {table:String}", + parameters={"db": self.state.database, "table": table}, + ).result_rows + engine = rows[0][0] if rows else "" + if engine.startswith("Shared"): + return (SHARED,) + if engine.startswith("Replicated"): + return self._replicas(table) + return None + + def _replicas(self, table: str) -> tuple[str, ...]: + path, total = self.client.query( + "SELECT zookeeper_path, total_replicas FROM system.replicas " + "WHERE database = {db:String} AND table = {table:String}", + parameters={"db": self.state.database, "table": table}, + ).result_rows[0] + cluster = self.state.health_cluster + if not cluster: + if total > 1: + raise WaitingError( + f"{self.state.database}.{table} has {total} replicas; set `cluster` in " + "config.yaml so `up` can wait for the mutation on every one" + ) + return (self.client.command("SELECT hostName()"),) + while len(hosts := self._cluster_replicas(table, path)) < total: + self.budget.pause( + f"{self.state.database}.{table}: cluster {cluster} shows {len(hosts)}/{total} replicas" + ) + return hosts + + def _cluster_replicas(self, table: str, path: str) -> tuple[str, ...]: + """Hosts the cluster can reach that hold a replica of this table's Keeper path.""" + rows = self.client.query( + "SELECT DISTINCT materialize(hostName()) FROM clusterAllReplicas({cluster:String}, " + "system.replicas) WHERE database = {db:String} AND table = {table:String} " + "AND zookeeper_path = {path:String}", + parameters={ + "cluster": self.state.health_cluster, + "db": self.state.database, + "table": table, + "path": path, + }, + settings=_cluster_settings(), + ).result_rows + return tuple(sorted(host for (host,) in rows)) + + def _snapshot(self, pending: list[_Row]) -> MutationProgress: + """The oldest pending mutation, as the display shows it.""" + table, mutation = min(((row.table, row.mutation) for row in pending), key=_mutation_order) + rows = [row for row in pending if (row.table, row.mutation) == (table, mutation)] + # A replica that hasn't received the mutation has every part its peers report. + most = max((len(row.names) or row.parts for row in rows if not row.missing), default=0) + hosts = [ + HostParts(row.host, row.names, most if row.missing else len(row.names) or row.parts) + for row in rows + ] + if self.budget.display.wants_bytes: + hosts = self._with_sizes(table, hosts) + return MutationProgress(f"{self.state.database}.{table}", mutation, tuple(hosts)) + + def _with_sizes(self, table: str, hosts: list[HostParts]) -> list[HostParts]: + # Parts never change size, so each is read once; only new names cost a query. + wanted = {(host.host, name) for host in hosts for name in host.names} + unknown = {name for _, name in wanted - self._part_sizes.keys()} + if unknown: + try: + self._part_sizes.update(self._read_sizes(table, unknown)) + except DatabaseError as error: + self.budget.display.parts_only(_sizes_unreadable(error)) + return hosts + sizes = self._part_sizes + return [ + replace( + host, + sizes={n: sizes[(host.host, n)] for n in host.names if (host.host, n) in sizes}, + ) + for host in hosts + ] + + def _read_sizes(self, table: str, names: set[str]) -> dict[tuple[str, str], int]: + cluster = self.state.health_cluster + source = "clusterAllReplicas({cluster:String}, system.parts)" if cluster else "system.parts" + rows = self.client.query( + f"SELECT materialize(hostName()), name, bytes_on_disk FROM {source} " + "WHERE database = {db:String} AND table = {table:String} AND active " + "AND has({names:Array(String)}, name)", + parameters={ + "cluster": cluster, + "db": self.state.database, + "table": table, + "names": sorted(names), + }, + settings=_cluster_settings() if cluster else None, + ).result_rows + shared = self._required_hosts(table) == (SHARED,) + return {(SHARED if shared else host, name): size for host, name, size in rows} + + +@dataclass(frozen=True) +class _Row: + """One host's view of one mutation, or a placeholder for a replica that hasn't got it.""" + + host: str + table: str + mutation: str + done: bool + parts: int + reason: str + names: frozenset[str] + missing: bool = False + + +def _raise_failures(database: str, rows: list[_Row], before_run: bool) -> None: + for row in rows: + if row.done or not row.reason: + continue + kill = ( + f"KILL MUTATION WHERE database = {sql_string(database)} " + f"AND table = {sql_string(row.table)} AND mutation_id = {sql_string(row.mutation)}" + ) + table = f"{database}.{row.table}" + prefix = "Blocked by a failed mutation" if before_run else "Mutation failed" + raise MutationFailed( + f"{prefix}: {table} mutation {row.mutation} on {row.host}: {row.reason}\n" + f"Operator only on {row.host}: {kill}", + FailedMutation(table, row.mutation, row.host, row.reason, kill, before_run), + ) + + +def _pending_text(database: str, pending: list[_Row]) -> str: + parts = [] + for row in sorted(pending, key=lambda row: (row.table, _order(row.mutation), row.host)): + where = f"{database}.{row.table} mutation {row.mutation} host {row.host}" + parts.append( + f"{where}; not received yet" if row.missing else f"{where}; parts_to_do={row.parts}" + ) + return "; ".join(parts) + + +def _mutation_order(key: tuple[str, str]) -> tuple[int, str]: + return _order(key[1]), key[0] + + +def _order(mutation: str) -> int: + numbers = re.findall(r"\d+", mutation) + return int(numbers[-1]) if numbers else 0 + + +def _cluster_settings() -> dict: + # A replica that's down is skipped, so its missing rows read as pending, not as done. + return { + "skip_unavailable_shards": 1, + "connect_timeout_with_failover_ms": 250, + "receive_timeout": 2, + } + + +def _sizes_unreadable(error: Exception) -> str: + code = re.search(r"\(([A-Z][A-Z_]+)\)", str(error)) + why = f" ({code.group(1)})" if code else "" + return ( + f"can't read system.parts{why}. Grant SELECT ON system.parts to the migration role " + "to see bytes and a time estimate." + ) diff --git a/ch_migrate/waiting/reruns.py b/ch_migrate/waiting/reruns.py new file mode 100644 index 0000000..c6c2a26 --- /dev/null +++ b/ch_migrate/waiting/reruns.py @@ -0,0 +1,182 @@ +"""Stop a rerun of an interrupted migration step that ch-migrate can't vouch for. + +Before a step runs, `up` and `down` record that it started; once its version is recorded, +the record goes. A record still there on the next run means the step was interrupted, so +some of its statements may already have applied. The step then runs again on its own only +if every statement in it passes the idempotency gate without a waiver. Otherwise the run +stops until someone checks what the earlier attempt changed and names the revision with +`--rerun-interrupted`. +""" + +from __future__ import annotations + +import logging +from dataclasses import dataclass +from pathlib import Path +from urllib.parse import quote +from uuid import uuid4 + +from clickhouse_connect.driver.binding import quote_identifier + +from ch_migrate.idempotency import classify_idempotency, waiver_reason +from ch_migrate.statements import MigrationStatement, migration_statements +from ch_migrate.waiting.errors import WaitingError +from ch_migrate.waiting.sql import sql_string + +TABLE = "_ch_migrate_in_progress" +logger = logging.getLogger(__name__) + + +class RerunGuard: + """Records each step as it starts and refuses an unsafe rerun of an interrupted one.""" + + def __init__(self, client, state, acknowledged: frozenset[str]): + self.client, self.state, self.acknowledged = client, state, acknowledged + self.before_first_upgrade = None # set by env.py when the gate hasn't run yet + self._ready = False + + def steps(self, original): + """Wrap Alembic's step iterator: check and record each step, clear it once recorded.""" + + def iterate(heads, context): + for step in original(heads, context): + if not hasattr(step, "revision"): + yield step # Stamp steps run no migration body. + continue + direction = "upgrade" if step.is_upgrade else "downgrade" + current = Step(step.revision.revision, direction, Path(step.revision.path)) + self.begin(current) + # Alembic asks for the next step only once this one's version is recorded. + yield step + self.finish(current) + + return iterate + + def begin(self, step: Step) -> None: + if step.direction == "upgrade" and self.before_first_upgrade is not None: + check, self.before_first_upgrade = self.before_first_upgrade, None + check() + self._prepare() + if self._started(step): + unvouched = unvouched_statements(step.path, step.direction) + if unvouched and not self._acknowledged(step.revision): + raise InterruptedStep(step, unvouched) + logger.warning( + f"{step.revision[:8]} was interrupted on an earlier run; running it again" + ) + self._record(step) + + def finish(self, step: Step) -> None: + self.client.command( + f"ALTER TABLE {self._table()} DELETE WHERE revision = {sql_string(step.revision)} " + "SETTINGS mutations_sync = 2" + ) + + def _prepare(self) -> None: + if self._ready: + return + if not self.client.command(f"EXISTS TABLE {self._table()}"): + self.client.command(self._create_sql()) + if self._replicated(): + # Fetch markers another replica wrote; there are no mutations here to wait for. + self.client.command(f"SYSTEM SYNC REPLICA {self._table()} LIGHTWEIGHT") + self._ready = True + + def _started(self, step: Step) -> bool: + return bool( + self.client.command( + f"SELECT count() FROM {self._table()} " + "WHERE revision = {revision:String} AND direction = {direction:String}", + parameters={"revision": step.revision, "direction": step.direction}, + settings={"select_sequential_consistency": 1} if self._shared() else None, + ) + ) + + def _acknowledged(self, revision: str) -> bool: + return any(len(given) >= 4 and revision.startswith(given) for given in self.acknowledged) + + def _record(self, step: Step) -> None: + self.client.command( + f"INSERT INTO {self._table()} (revision, direction) " + "SELECT {revision:String}, {direction:String}", + parameters={"revision": step.revision, "direction": step.direction}, + # Replicated and Shared MergeTree drop a block identical to an earlier insert. + settings={ + "insert_deduplication_token": f"chm_started_{uuid4().hex}", + "async_insert": 0, + }, + ) + + def _create_sql(self) -> str: + engine, on_cluster = "MergeTree", "" + if self.state.database_engine == "Replicated": + engine = "ReplicatedMergeTree" + elif not self._shared() and self.state.cluster: + path = f"/clickhouse/ch_migrate/{quote(self.state.database, safe='')}/{{shard}}/{TABLE}" + engine = f"ReplicatedMergeTree({sql_string(path)}, '{{replica}}')" + on_cluster = f" ON CLUSTER {quote_identifier(self.state.on_cluster)}" + return ( + f"CREATE TABLE IF NOT EXISTS {self._table()}{on_cluster} (revision String, " + "direction LowCardinality(String), started DateTime64(3) DEFAULT now64(3)) " + f"ENGINE = {engine} ORDER BY (revision, direction)" + ) + + def _table(self) -> str: + return f"{quote_identifier(self.state.database)}.{quote_identifier(TABLE)}" + + def _shared(self) -> bool: + return self.state.database_engine.startswith("Shared") + + def _replicated(self) -> bool: + return self.state.database_engine == "Replicated" or ( + not self._shared() and bool(self.state.cluster) + ) + + +@dataclass(frozen=True) +class Step: + revision: str + direction: str # "upgrade" or "downgrade" + path: Path + + +class InterruptedStep(WaitingError): + """An interrupted step has statements that might not be safe to run again.""" + + def __init__(self, step: Step, unvouched: list[Unvouched]): + self.step, self.unvouched = step, unvouched + listed = "; ".join(f"{_where(item.statement)} ({item.why})" for item in unvouched) + super().__init__( + f"{step.revision[:8]} was interrupted on an earlier run, and ch-migrate can't vouch " + f"for running it again: {listed}. Check what the earlier run changed, then run it " + f"again with --rerun-interrupted {step.revision[:8]} " + f"(with alembic, -x rerun_interrupted={step.revision[:8]})." + ) + + +@dataclass(frozen=True) +class Unvouched: + """A statement in an interrupted step that might not be safe to run again.""" + + statement: MigrationStatement + why: str + + +def unvouched_statements(path: Path, direction: str) -> list[Unvouched]: + """Statements in this step the gate can't vouch for: waived, unreadable, or unsafe.""" + found = [] + for statement in migration_statements(path): + if statement.direction != direction: + continue + reason = waiver_reason(statement.comments) + if reason is not None: + found.append(Unvouched(statement, f"waiver: {reason}" if reason else "empty waiver")) + elif not statement.readable: + found.append(Unvouched(statement, "SQL that only exists at run time")) + elif classify_idempotency(statement.sql).status != "ok": + found.append(Unvouched(statement, "not safe to run twice")) + return found + + +def _where(statement: MigrationStatement) -> str: + return f"{statement.source}:{statement.line}" diff --git a/ch_migrate/waiting/sql.py b/ch_migrate/waiting/sql.py new file mode 100644 index 0000000..bb28485 --- /dev/null +++ b/ch_migrate/waiting/sql.py @@ -0,0 +1,36 @@ +"""The little SQL the waiter needs: which statements can start work, and string literals.""" + +from __future__ import annotations + +import re + +from ch_migrate.sql import _skip_quoted_or_comment + +# Statements that only read or change the session; everything else may start a mutation +# or distributed DDL. Waiting after a read costs one poll, so unknown forms count as writes. +_READS = frozenset(("SELECT", "SHOW", "DESCRIBE", "DESC", "EXPLAIN", "EXISTS", "SET", "USE")) +_WORD = re.compile(r"[A-Za-z]+") + + +def is_write(statement: str) -> bool: + """True unless the statement's first keyword only reads or sets session state.""" + word = _WORD.match(_first_code(statement)) + return word is None or word.group(0).upper() not in _READS + + +def sql_string(value: str) -> str: + return "'" + value.replace("\\", "\\\\").replace("'", "\\'") + "'" + + +def _first_code(sql: str) -> str: + """The statement from its first character that isn't whitespace or a comment.""" + i = 0 + while i < len(sql): + if sql[i].isspace(): + i += 1 + continue + skip_to = _skip_quoted_or_comment(sql, i) + if skip_to is None or sql[i] in "'\"`": + return sql[i:] + i = skip_to + return "" diff --git a/ch_migrate/waiting/waiter.py b/ch_migrate/waiting/waiter.py new file mode 100644 index 0000000..bc75c2e --- /dev/null +++ b/ch_migrate/waiting/waiter.py @@ -0,0 +1,68 @@ +"""Hold each migration until ClickHouse has finished the work its statements started.""" + +from __future__ import annotations + +from sqlalchemy import event + +from ch_migrate.waiting.budget import WaitBudget +from ch_migrate.waiting.mutations import MutationWaiter +from ch_migrate.waiting.sql import is_write + +# ON CLUSTER statements wait inside ClickHouse. The limit stays below clickhouse-connect's +# default 300-second read timeout, so ClickHouse answers before the client gives up. +DDL_WAIT_LIMIT = 270 + + +class MigrationWaiter: + """Waits after every write on the migration connection; reads and `SET`s pass through. + + ON CLUSTER statements return only once every host has run them; a host that fails or + runs out of time is an error. After each write it waits for the database's unfinished + mutations. + """ + + def __init__(self, connection, state, timeout: float | None = None): + self.connection = connection + self.client = connection.connection.dbapi_connection.client + self.budget = WaitBudget(timeout) + self.mutations = MutationWaiter(self.client, state, self.budget) + self._installed = False + + def install(self, config) -> None: + # `ch-migrate up` passes the display it labels with each migration's name. + self.budget.display = config.attributes.get("ch_migrate_display", self.budget.display) + # Request settings, not a session SET, so a migration's own SETs can't undo them. + # wait_end_of_query makes a DDL timeout arrive as an error, not a truncated response. + self.client.set_client_setting("distributed_ddl_output_mode", "throw") + self.client.set_client_setting("distributed_ddl_task_timeout", DDL_WAIT_LIMIT) + self.client.set_client_setting("wait_end_of_query", 1) + event.listen(self.connection, "before_cursor_execute", self._before) + event.listen(self.connection, "after_cursor_execute", self._after) + self._installed = True + + def wait(self, before_run: bool = False) -> None: + """Wait for the database's unfinished mutations, including another writer's.""" + self.mutations.wait(before_run) + + def close(self) -> None: + self.budget.finish() + if self._installed: + event.remove(self.connection, "before_cursor_execute", self._before) + event.remove(self.connection, "after_cursor_execute", self._after) + self._installed = False + + def _before(self, conn, cursor, statement, parameters, context, executemany) -> None: + if is_write(statement) and self.budget.timeout is not None: + self._limit_distributed_ddl() + + def _after(self, conn, cursor, statement, parameters, context, executemany) -> None: + if is_write(statement): + self.wait() + + def _limit_distributed_ddl(self) -> None: + # `--timeout` covers the whole run, so ClickHouse waits only for what's left of it. + self.budget.check("the next statement") + left = int(self.budget.timeout - self.budget.elapsed) + self.client.set_client_setting( + "distributed_ddl_task_timeout", max(1, min(DDL_WAIT_LIMIT, left)) + ) diff --git a/docs/design/2026-10-02-ch-migrate-1.0.md b/docs/design/2026-10-02-ch-migrate-1.0.md index 614aba1..9e5cb67 100644 --- a/docs/design/2026-10-02-ch-migrate-1.0.md +++ b/docs/design/2026-10-02-ch-migrate-1.0.md @@ -99,9 +99,10 @@ both; the README quick start contains no Python. ([dialect spike](2026-10-02-dialect-spike.md)). A session that expires while idle must fail loudly, not quietly drop its settings. - The version table cannot cause a migration to run twice. The version advances by inserting - the new row and then deleting the old one, waiting for the delete. An interrupted run is - therefore refused, not repeated. Today's `ALTER … UPDATE` returns before it is applied, and - the spike reproduced a migration running twice because of it. + the new row and then deleting the old one, waiting for the delete. Interrupted + bookkeeping is never permission to repeat the migration: overlapping heads are + refused. The spike reproduced duplicate execution with the old asynchronous + `ALTER … UPDATE` approach. - The version table is created to suit the deployment: shared on ClickHouse Cloud, replicated in a replicated database or on a configured cluster, plain on a single server. - `ch-migrate upgrade-env` moves an existing project to the new `env.py`. Existing version tables @@ -110,8 +111,8 @@ both; the README quick start contains no Python. **Accept:** an integration test against a real ClickHouse server shows a `SET` in one statement taking effect in the next; an existing 0.x project upgrades with `upgrade-env` and -runs `up` without editing migration files; with the version table's delete held back, a second -`up` refuses instead of re-running a migration. +runs `up` without editing migration files; with the version table's delete held back, +a second `up` cannot re-run the migration. It refuses the overlapping state. ### 4. Re-runnable migrations @@ -142,22 +143,32 @@ again, and reaches the same schema as a clean run. `up` does not record a migration as applied until ClickHouse has finished the work the migration started. -- After each statement that creates a mutation, `up` polls `system.mutations` until the mutation - finishes, showing progress. It does not rely on `mutations_sync`, which holds one HTTP request - open for the whole rewrite and loses the outcome if the connection drops. -- For `ON CLUSTER` DDL, `up` waits on the distributed DDL queue rather than trusting the - client-side timeout. +- After each statement that can write, `up` polls `system.mutations` until the project database + has no unfinished mutation on any replica that has to apply it, showing progress. It does not + rely on `mutations_sync`, which holds one HTTP request open for the whole rewrite. +- For `ON CLUSTER` DDL, ClickHouse waits for every host (`distributed_ddl_output_mode = 'throw'`) + within a limit below the client's read timeout, so a failing or unfinished host is an error, + not a quiet client-side timeout. - Waiting is on by default. There is no default timeout; `--timeout` sets one. - Whenever `up` stops waiting (a timeout, a killed CI job, a dropped connection), the outcome is the same: the migration is not recorded as applied, ClickHouse keeps doing the work, and the - next `up` finds the unfinished mutation and waits on it instead of issuing the statement again. - Issuing `UPDATE … SET n = n + 1` twice applies it twice. + next `up` waits for that work to finish, then runs the migration again. That relies on + capability 4: every statement must be safe to repeat, or carry a written waiver. A + migration noted as started but not recorded reruns on its own only if every statement + passes the gate without a waiver; otherwise `up` stops and names the statements until + someone checks and passes `--rerun-interrupted REV` (Dan, 2026-10-27, DRY-1609). +- **Design change, 2026-10-27 (Dan):** the design approved on 2026-10-04 recorded which work + each statement started in a journal, so a rerun would never send a statement twice. It took + about 1,300 lines, duplicated what capability 4 already guarantees, and left a killed or + rejected mutation unrecoverable without editing the journal by hand (DRY-1604). 0.7 waits by + observation instead; the [waiting design](2026-10-02-waiting.md) keeps the journal design and + its experiments for the record. - A failing mutation does not end on its own: ClickHouse retries it and holds back every later mutation on the same table. `up` reads `latest_fail_reason`, fails the migration, and prints the `KILL MUTATION` statement for the operator. ch-migrate never kills a mutation itself. -The exact rules, starting with how `up` recognises a mutation it started on an earlier run, are -settled in a short design note before the 0.7 plan (see Open questions). +The execution, ownership and recovery rules are in the +[waiting design](2026-10-02-waiting.md). **Accept:** integration tests show that a migration whose mutation fails exits non-zero with the failure reason and leaves the version table unchanged; and that killing `up` while a mutation @@ -308,7 +319,10 @@ is unaffected; Python 3.9 is not supported, so 0.6 requires 3.10. 3. Does `bootstrap` (roles, users, grants) sit cleanly on the new connection path? 4. Does the dialect support Python 3.9? If not, it is ruled out for 1.0 (see Compatibility). -**A design note on waiting, before the 0.7 plan:** +**A design note on waiting, before the 0.7 plan.** Answered in the +[waiting design](2026-10-02-waiting.md). Accepted-but-unacknowledged mutation recovery +is demonstrated. Dan approved explicit failure and operator reconciliation when +missing or expired evidence prevents establishing the outcome, on 2026-10-04. 5. On a re-run, how does `up` recognise a mutation it started earlier, without mistaking someone else's mutation for its own? diff --git a/docs/design/2026-10-02-waiting.md b/docs/design/2026-10-02-waiting.md new file mode 100644 index 0000000..a3e2da6 --- /dev/null +++ b/docs/design/2026-10-02-waiting.md @@ -0,0 +1,489 @@ +# Waiting for mutations and distributed DDL + +**Superseded for 0.7 (Dan, 2026-10-27).** 0.7 doesn't ship the journal design below. It +waits by observation: after each write, `up` polls `system.mutations` until the project +database has none unfinished, and ClickHouse waits for `ON CLUSTER` DDL itself. A rerun +waits for unfinished work, then runs the migration again, relying on the idempotency gate. +Capability 5 in the [product definition](2026-10-02-ch-migrate-1.0.md) and the README's +"Migrations that wait" describe what ships. The rest of this note is the record of the +journal design and its experiments. + +**DRY-1398 · 2026-10-02 · Recovery policy approved by Dan on 2026-10-04.** + +A write-ahead intent plus a marker carried into ClickHouse's mutation record can +recover an accepted `UPDATE` after the client dies without reading its response. +The experiment passes three times: the resumed process observes the same mutation +ID, waits, and the counter ends at 1, not 2. + +That is **not an unconditional replay guarantee**. Completed mutation records can +expire while the client is dead. An intent without a surviving marker cannot tell +us whether the request never arrived, is still being submitted, or completed and +was removed from the system table. Reissuing can apply `x = x + 1` twice; skipping +can omit it. No command-normalization or time-window heuristic fixes that. + +**Approved recovery rule:** when an interrupted statement's outcome cannot be +proven, exit nonzero, do not reissue it, do not mark the migration complete, and +require operator reconciliation. Known unfinished work still reattaches +automatically. Dan approved this exception to +[capabilities 4 and 5](2026-10-02-ch-migrate-1.0.md) and explicitly resumed +local-only implementation on 2026-10-04. + +Approval revalidation on 2026-10-04: all 16 local experiments passed; the separate +three-trial kill/re-run pass passed. The local CI gate passed 641 tests on each of +Python 3.10, 3.11, 3.12, 3.13 and 3.14. The implementation rows below remain the +acceptance contract for DRY-1399/1400, not claims that those features already exist. + +## Reproduce the evidence + +The explicit probe uses the existing owned-container fixtures. It creates its own +databases, containers, network, passwords, and subprocesses; it does not use a +shared service. Its filename keeps it out of normal pytest collection. + +Run from the repository root: + +```bash +env -u CH_MIGRATE_IT_URL uv run --locked --python 3.12 pytest -q -s \ + -m integration tests/integration/waiting_spike_probe.py + +# Independent second pass: three fresh projects and three killed processes. +env -u CH_MIGRATE_IT_URL uv run --locked --python 3.12 pytest -q -s \ + -m integration tests/integration/waiting_spike_probe.py \ + -k kill_before_receipt +``` + +The checked-in [submitter](spikes/2026-10-02-waiting/submit_without_ack.py) sends the +HTTP body but deliberately never reads the response. The +[resumer](spikes/2026-10-02-waiting/resume_fence.py) reads only the pre-send intent +and server records. It never reissues the original mutation. The probe kills the +submitter only after an independent connection observes the pending mutation, so +the test proves the difficult **accepted but not acknowledged** case. It does not +pretend that it proves every possible interruption point. + +Observed server: ClickHouse 26.3.39.7. The single-node and one-shard/two-replica +fixtures use the `26.3` image. Final reproduction: **16 passed**; independent +kill-and-re-run second pass: **3 passed**. The local CI command +`uv run --locked --python pytest -q` passed with **565 tests** on each +of Python 3.10, 3.11, 3.12, 3.13 and 3.14. These are local results, not remote CI. +The SQL below uses `db` and short marker names instead of random identifiers. + +## Q5. Recognize our work without claiming another writer's work + +### What the server preserves + +The metadata probe queries `system.columns` and submits these two requests with +distinct `query_id` and `log_comment` settings: + +```sql +ALTER TABLE db.ownership UPDATE x = x + 1 +WHERE id = 1 /* chm_marker_comment */; +ALTER TABLE db.ownership UPDATE x = x + 1 +WHERE id = 2 AND 'chm_marker_expression' = 'chm_marker_expression'; +SELECT mutation_id, command, is_done +FROM system.mutations WHERE database = 'db' AND table = 'ownership'; +``` + +Observed with merges stopped: + +```text +mutation_2.txt | (UPDATE x = x + 1 WHERE id = 1) | 0 +mutation_3.txt | (UPDATE x = x + 1 WHERE (id = 2) AND + ('chm_marker_expression' = 'chm_marker_expression')) | 0 +``` + +`system.mutations` has `mutation_id` and `command`, but no query ID or log comment. +The comment disappears. The literal expression survives. `system.query_log` +preserves query IDs and log comments, but the experiment's mutation-related +`ProfileEvents` map is empty: it supplies no query-to-mutation receipt. Query logs +are also asynchronously flushed and retained for a bounded period. + +**Rejected identities:** normalized SQL, table plus timestamp, and before/after ID +set difference. Another writer can issue the same statement in the same window. +Those techniques can identify candidates or preceding blockers, not ownership. +`formatQuery()` would normalize spelling, not make a command unique. + +### A durable marker within the submitted operation + +For an `ALTER UPDATE`, a combined empty delete preserves the original predicate +and adds a unique marker: + +```sql +ALTER TABLE db.counter + UPDATE x = x + 1 WHERE 1, + DELETE WHERE 0 AND 'chm_step_TOKEN' = 'chm_step_TOKEN' +SETTINGS mutations_sync = 0, alter_sync = 0; +``` + +Observed: two command rows, **one mutation ID**. Counting rows in +`system.mutations` is not counting distinct mutations. + +```text +mutation_2.txt | (UPDATE x = x + 1 WHERE 1) | 0 +mutation_2.txt | (DELETE WHERE 0 AND ('chm_step_TOKEN' = 'chm_step_TOKEN')) | 0 +``` + +The three killed-client trials each printed: + +```text +resumed mutation_2.txt; mutation_ids=['mutation_2.txt']; command_rows=2; x=1 +``` + +The original and final sets of mutation IDs were equal. The resumer remained +running while merges were stopped and completed after `SYSTEM START MERGES`. + +For schema changes, **do not assume the marker shares the mutation ID**: + +| Combined `ALTER` action | Observed command IDs | +|---|---| +| `MODIFY COLUMN x UInt32, DELETE WHERE ` | schema `mutation_2.txt`, marker `mutation_3.txt` | +| `MATERIALIZE COLUMN y, DELETE WHERE ` | both `mutation_2.txt` | +| `CLEAR COLUMN y, DELETE WHERE ` | schema `mutation_2.txt`, marker `mutation_3.txt` | +| `RENAME COLUMN y TO renamed, DELETE WHERE ` | schema `mutation_2.txt`, marker `mutation_3.txt` | + +Thus a marker is a **completion barrier**, not necessarily the original mutation. +Wait for its command rows and all preceding outstanding work on that table; never +infer the original command's completion from a different table's marker. Inspect +failures in preceding mutations rather than reporting only the empty delete. +Persist the table UUID so that dropping and recreating a name cannot make an old +intent apply to a different table. + +The empty-marker cost probe used 10,000 rows with a payload column. It retained all +10,000 rows. `system.part_log` reported a `MutatePart` event with `read_rows = 0`, +`read_bytes = 0`, and `MutationUntouchedParts = 1`, but also file reads and buffer +allocations in `ProfileEvents`. The marker is not free. It also requires DELETE +privilege. Predicate markers avoid that extra command for suitable UPDATE/DELETE +statements, but require syntax-aware insertion and their own corpus proof. + +### Minimal runtime state + +One append-only server-side journal, deployed with the same replication policy as +the version table, stores: + +- revision and statement position; +- exact statement/parameter digest, target database/table and UUID; +- unique attempt token and operation kind; +- phase: intent, accepted receipt, completed; +- receipt: mutation marker/IDs or DDL queue entry, plus required hosts. + +Write and acknowledge intent **before** sending SQL. Never derive the intent only +after submission. Record completion before moving to the next statement. An +unfinished revision reuses its attempt, validates its digest and target UUID, +skips completed work, and attaches to known unfinished work. A changed migration +or replaced target fails before submission. Successful downgrade must invalidate +that revision's old completed journal, so a subsequent upgrade actually runs. + +The insertion-deduplication probes establish only a useful primitive, not a lock: +with a stable `insert_deduplication_token`, two single-node inserts retained the +first owner; concurrent replicated inserts through both nodes retained the same +one owner on both. Deduplication has a retention window. Do not turn this result +into an unbounded exactly-once or concurrent-run claim. Concurrent migrators need +serialization; this spike has not proved a portable fencing lock. + +### Missing-evidence recovery + +If the process dies after preparing intent but before receiving a durable receipt, +look up the marker. If it is present, attach without submitting again. If it is +absent, **absence is not completion and is not permission to retry**. + +There are at least three indistinguishable histories at this boundary: + +1. Intent was committed; SQL never reached the server. +2. SQL reached the server but has not registered its marker yet. +3. SQL completed; the process died before recording completion; the server later + removed the completed marker under its retention policy. + +The retention probe sets `finished_mutations_to_keep = 1`, `old_parts_lifetime = 0`, +`cleanup_delay_period = 1` and `cleanup_delay_period_random_add = 0` on its disposable +table, completes a marked increment and then another mutation. Observed: +only `mutation_3.txt (DELETE WHERE 0)` remains, while the counter is still 1. +The first trial without accelerated cleanup retained both records for 30 seconds; +retention is asynchronous, not immediate after the next mutation. +A resumed journal reader has no receipt to distinguish +that history from an unsubmitted operation. Unfinished records surviving a +normal mid-wait SIGKILL do not establish that completed records survive forever. + +**Approved exception:** fail with `outcome unknown; statement not reissued; +migration incomplete`, including the intent token, revision, statement position, +target UUID, and read-only diagnostic queries. The operator must stop concurrent +runners, establish the actual outcome from independent evidence, and reconcile +the data and that specific journal record in a reviewed manual operation before +resuming. An absent server record alone never authorizes replay or completion. +There is no automatic stamp, retry override, blind journal deletion, or KILL. + +## Q6. Foreign mutations ahead of ours + +The probe submits a failing foreign mutation, then a second update: + +```sql +ALTER TABLE db.foreign_probe UPDATE x = throwIf(x = 5) WHERE 1; +ALTER TABLE db.foreign_probe UPDATE x = x + 1 WHERE id = 1; +``` + +Observed: + +```text +mutation_2.txt | is_done=0 | parts_to_do=1 | Code: 395 ... throwIf ... +mutation_3.txt | is_done=0 | parts_to_do=1 | latest_fail_reason='' +rows: [(1, 1), (5, 5)] +``` + +Recommendation: before creating more work on that table, report and wait for +healthy preceding foreign mutations. A preceding failure exits nonzero with its +host, reason and exact operator-only `KILL MUTATION` command. Do not label it as +our mutation or insert another mutation behind a known permanent failure. A +foreign mutation arriving after our completion barrier is not a reason to hold +our revision open. An operator's KILL is not proof our work succeeded. + +## Q7. Every replica, not merely the connected replica + +The cluster probe creates a ReplicatedMergeTree, inserts a row, and synchronizes +node 2. It stops merges, fetches and replication queues **on node 2 only**, submits +an increment through node 1, then pulls the mutation metadata on node 2. + +```text +node 1: mutation_id=0000000000, is_done=1, parts_to_do=0 +node 2: mutation_id=0000000000, is_done=0, parts_to_do=1 +``` + +After restarting those operations and synchronizing node 2, its counter is 1. +A local-only query would have reported success too early. + +Rules for self-hosted replicas: + +- Snapshot the required topology before submission. Use the configured cluster or + a Replicated database's generated cluster, and verify coverage against the + table's replica metadata. Missing topology is an error, not permission to wait + only locally. +- Query every required host's `system.mutations`, for example with + `clusterAllReplicas`. Identify hosts as well as mutation IDs; IDs are not global + across unrelated tables/shards. +- Require `is_done = 1` for every command row of every required mutation on every + required replica. `parts_to_do = 0` is progress, not completion: a long insert + may still add a part that must be mutated. +- A host omitted by `skip_unavailable_shards` is missing evidence, not success. + Track required hosts separately. A missing record is likewise not a completed + record unless the journal already stores that host's completion. +- Persist observed completion so later retention cannot erase an already-proved + result. The crash before persisting that result remains Q5's blocker. + +## Q8. Distributed DDL and a down host + +The probe stops node 2 and submits: + +```sql +CREATE TABLE db.ddl_owned ON CLUSTER it_cluster (id UInt64) ENGINE = Memory +SETTINGS distributed_ddl_task_timeout = 0, + distributed_ddl_output_mode = 'none', log_comment = 'chm_ddl_TOKEN'; +SELECT entry, host, settings['log_comment'], status +FROM system.distributed_ddl_queue +WHERE settings['log_comment'] = 'chm_ddl_TOKEN'; +``` + +Observed one entry, `query-0000000001`, with both target hosts and the same marker: +node 1 `Active`, node 2 `Inactive`. ClickHouse inserted a UUID into the stored CREATE +query. Exact comparison with the original SQL would not identify it reliably. +The `settings['log_comment']` marker survived and is a better identity. + +Rules: + +- Write intent first. Set a unique, journaled `log_comment` for the request. Preserve + any caller comment in the journal rather than treating it as the identity. +- Submit with `distributed_ddl_task_timeout = 0` and output mode `none`, then poll + the queue by marker/entry. The server's synchronous client timeout is not the + migration's completion test. +- Success requires every recorded target host to be `Finished` with + `exception_code = 0`. `Finished` alone can mean failure. Missing rows, + `Inactive`, `Active`, `Removing` and `Unknown` are not success. +- A down host stays pending. Default wait has no deadline; `--timeout` exits + nonzero naming the host, queue entry and status. It does not delete queue work. + Once the host returns, rerun attaches to the same entry. +- Any nonzero exception exits nonzero with host, code and text. The failure probe + pre-adds `conflict String` only on node 2, then enqueues `ADD COLUMN conflict + UInt64` on both. This produces a host-specific schema error without a network + timeout or an intentionally stuck mutation. + Observed queue results: node 1 `Finished / 0`, node 2 `Finished / 15` + with `Cannot add column conflict: column with this name already exists`. +- For mutation-producing `ALTER ... ON CLUSTER`, finish both barriers: all hosts + executed DDL **and** all required mutations finished. Neither subsumes the other. +- Queue retention creates the same unknown-outcome boundary as Q5. A missing + entry must not trigger blind DDL resubmission. + +## Q9. Cloud: reasoned, not tested + +DRY-1413 has no `service ready` comment. No Cloud connection or credential lookup +was made for this note. These conclusions come from official documentation, not +the local replicated harness: + +- The [system.mutations documentation](https://clickhouse.com/docs/reference/system-tables/mutations) + says each Cloud node exposes all cluster mutations, so `clusterAllReplicas` is + unnecessary for this table. +- The [SharedMergeTree reference](https://clickhouse.com/docs/products/cloud/features/infrastructure/shared-merge-tree) + describes shared mutation work, not a per-replica rewrite. It also states that + `system.replication_queue` is not the SharedMergeTree introspection API. +- Cloud converts MergeTree-family tables to SharedMergeTree-family tables. The + journal should follow that policy, not introduce a self-hosted Keeper path. +- `ON CLUSTER` is unnecessary on Cloud. Skip the distributed-DDL mechanism for + ordinary Cloud DDL; do not invent a cluster or infer completion from a + self-hosted replica queue. + +The 0.7 acceptance ticket must prove mutation visibility, marker preservation, +retention behavior, session continuity and journal persistence on the authorized +Cloud service. Local success cannot close these checks. + +## Which statements create mutation records + +The corpus experiment runs each statement on a fresh seeded MergeTree table and +queries `system.mutations` afterward. It leaves completed records in place for +inspection. + +| Statement | Observed record on 26.3.39.7 | +|---|---| +| `ALTER ... UPDATE` | `UPDATE ...` | +| `ALTER ... DELETE` | `DELETE ...` | +| lightweight `DELETE FROM` | `UPDATE _row_exists = 0 ...` | +| lightweight `UPDATE ... SET` | no mutation record; patch data already visible | +| `MODIFY COLUMN x UInt32` | `MODIFY COLUMN x UInt32` | +| `DROP COLUMN y` | `DROP COLUMN y` | +| `CLEAR COLUMN y` | `CLEAR COLUMN y` | +| `MATERIALIZE COLUMN y` | `MATERIALIZE COLUMN y` | +| `RENAME COLUMN y TO z` | `RENAME COLUMN y TO z` | +| `MODIFY TTL ts + INTERVAL 1 DAY` | `MATERIALIZE TTL` | +| `UPDATE x = throwIf(x = 5)` | pending record with failure code 395 | + +The first lightweight UPDATE attempt failed with code 48 because the required +storage columns were disabled. With `enable_block_number_column = 1`, +`enable_block_offset_column = 1`, and `allow_experimental_lightweight_update = 1`, +it returned rows `[(1, 11), (2, 20)]` and **no** `system.mutations` rows. It must not +be modeled as a background mutation that is complete merely because a lookup is +empty. Wait for its synchronous request; patch-merge completion is not a mutation +completion signal. Do not silently rewrite it as a heavyweight UPDATE. + +`MODIFY TTL` can produce work under a different command string. Generic command +matching would miss it. The implementation corpus must cover query/session +settings that change materialization, including `materialize_ttl_after_modify`. + +## Execution, sessions, version state and output + +### Hook at the actual cursor boundary + +Use SQLAlchemy `before_cursor_execute` with `retval=True` and +`after_cursor_execute` on the migration connection, as established by DRY-1391. +This covers `op.execute`, `op.get_bind()` and `run_sql`'s `exec_driver_sql` path. +Do not use only `ChMigrateImpl._exec`. Guard journal, polling and progress queries +against recursive instrumentation. Offline SQL generation does not connect or +invent journal receipts. + +Poll on the same migration session, using bounded requests and an interval less +than its idle timeout. This both preserves session `SET`s and keeps the session +alive. Propagate DRY-1392's expired-session failure; never silently recreate it. +A separate monitoring client alone does not keep the migration session alive. + +Online `up` invokes Alembic in the CLI process, so SIGKILL cannot leave a child +Alembic runner advancing the version after its parent dies. A narrow revision-step +adapter establishes the journal scope; cursor events still cover every SQL path. +Pre-hooks share the resumable batch scope. Post-hooks run after the revision body +but before its version transition, so their mutations are covered by the same +completion barrier. + +### Do not hold a request open to wait + +The managed request uses asynchronous mutation settings and polls afterward. +The settings probe submits a stopped mutation with both `mutations_sync = 2` and +then `mutations_sync = 0`; it returns while both command rows remain unfinished. +That proves the last duplicate value wins on this server, not that text appending +is a general SQL-rewriting strategy. Parse and replace managed settings instead +of relying on duplicate-setting precedence. Do not permanently reset unrelated +session settings. + +`MODIFY TTL` cannot take an appended ALTER marker: its comma-separated TTL-rule +grammar consumes the apparent next action. The runtime therefore acknowledges +the metadata change with automatic materialization disabled, journals that +boundary, and submits `MATERIALIZE TTL` with its own marker. A resume does not +repeat either submitted operation. Existing session/per-query opt-out is honored. +An unacknowledged metadata change remains an unknown outcome under the approved +rule; it is not silently reissued. Executemany writes re-enter the single-statement +path for each parameter set, preserving an independent intent and completion +barrier rather than collapsing intermediate TTL effects into one materialization. + +The journal and version INSERTs explicitly disable asynchronous insertion. A +migration's session `async_insert=1, wait_for_async_insert=0` must not turn control +state into queued, unacknowledged data. + +### The version table's delete + +DRY-1412 inserts the new head before deleting the previous one. Its delete is a +real mutation and currently uses the official implementation's synchronous wait. +The waiting implementation must put that delete through the same asynchronous +polling/error surface, without weakening the insert-first safety invariant. +After interruption, both heads may be present; that is a fail-closed overlap, not +permission to replay the revision. Attach to the known version-delete receipt +before evaluating the next revision. Never emit “applied” until the delete barrier +finishes. This is not a promise of transactional rollback of the inserted head. + +### Progress and exits + +Stable message semantics (data substituted, not literal output fixtures): + +- Progress: `Waiting: db.table mutation ID host HOST; N parts; Ns elapsed`. +- Failure, exit 1: `Mutation failed: ... REASON`, followed by an operator-only + `KILL MUTATION WHERE database = 'db' AND table = 'table' AND mutation_id = 'ID'`. + On clusters, name the affected host; do not suggest an unscoped kill. +- Timeout, exit 1: `Timed out; server work continues; revision not completed`, + followed by every pending mutation/host/DDL entry and its last known state. +- Foreign failure, exit 1: `Blocked by foreign mutation ...`, reason and command. +- Unknown outcome, exit 1: `Outcome unknown; statement not reissued; + migration incomplete`, intent token, target UUID and reconciliation requirement. +- Configuration/permission/topology error: exit 1 with the actual error; never a + zero-work success. Existing CLI usage errors remain Click's exit 2. + +Use a live line only on a TTY. In CI, emit an initial plain line, state changes, +periodic summaries, and a final result to stderr. Use monotonic elapsed time and +one optional `--timeout SECONDS` budget for the invocation's waits, not a fresh +budget for each host. No default timeout; no automatic mutation/DDL cancellation. + +## Numbered implementation test matrix + +These are required implementation checks, **not claimed implementation results**. +Unknown-outcome rows use the recovery rule approved on 2026-10-04. The experiment +names make the subset already demonstrated reproducible. + +| Row | Owner | Required observable result | +|---|---|---| +| W01 | DRY-1399 | Every mutation-producing corpus statement waits; schema mutations and TTL are not missed; lightweight UPDATE is not falsely identified as a background mutation. | +| W02 | DRY-1399 | `throwIf` failure exits nonzero with reason, table, ID and operator KILL; no automatic KILL; pre-revision head unchanged. | +| W03 | DRY-1399 | SIGKILL while waiting; rerun attaches to the same ID and increments once. Run three independent trials. | +| W04 | DRY-1399 | Kill after accepted send but before reading receipt; recovery uses pre-send intent and marker; no second command. | +| W05 | DRY-1399 | Timeout reports outstanding IDs, parts and hosts; work remains on the server; rerun finishes without replay. | +| W06 | DRY-1399 | A paused second replica prevents success even after the first finishes; returning replica permits completion. | +| W07 | DRY-1399 | Missing required host/record is not success, including a query using skip-unavailable behavior. | +| W08 | DRY-1399 | Healthy foreign predecessor is reported and waited; failed predecessor is named; trailing foreign work is not adopted as ours. | +| W09 | DRY-1399 | Same SQL from another writer is not owned; table recreation and changed migration digest refuse before replay. | +| W10 | DRY-1399 | Expired/missing ownership evidence exits nonzero with diagnostics and operator-reconciliation guidance; never silently completes or resubmits the statement. | +| W11 | DRY-1399 | Polls longer than a short session timeout preserve a prior SET; an actually expired session fails loudly. | +| W12 | DRY-1399 | Python op.execute, op.get_bind().execute/exec_driver_sql and run_sql all use the hook; polling does not recurse. | +| W13 | DRY-1399 | Interrupted/failed version DELETE uses the same diagnostics, preserves insert-first safety, and never re-executes an applied revision. | +| W14 | DRY-1399 | TTY and CI progress identify real state; timeout budget is not reset per mutation; zero parts alone does not mean done. | +| W15 | DRY-1399 | Completed earlier statements are not repeated on resume; downgrade then upgrade does not reuse an old completed journal. | +| W16 | DRY-1400 | CREATE then ALTER ON CLUSTER finishes on all hosts before success and before version completion. | +| W17 | DRY-1400 | Stopped node remains pending; timeout names it; restart and rerun attach to the same queue entry. | +| W18 | DRY-1400 | One-host schema conflict exits nonzero with host/code/text even when queue status is Finished. | +| W19 | DRY-1400 | Accepted DDL with lost client receipt is found by journaled log_comment, not exact normalized query text. | +| W20 | DRY-1400 | Mutation-producing ON CLUSTER ALTER satisfies both the DDL and mutation barriers. | +| W21 | DRY-1400 | Missing/expired queue entry is not success or automatic resubmission; uses the agreed unknown-outcome rule. | +| W22 | DRY-1400 | Single-node/no-ON-CLUSTER statements do not query a fictitious distributed DDL queue. | +| W23 | DRY-1401 | Authorized Cloud acceptance proves SharedMergeTree mutation visibility, markers, journal persistence and same-session polling; no self-hosted replica assumptions. | + +## Sources and limits + +- [system.mutations](https://clickhouse.com/docs/reference/system-tables/mutations): + command rows, IDs, completion versus parts, Cloud visibility. +- [system.distributed_ddl_queue](https://clickhouse.com/docs/reference/system-tables/distributed_ddl_queue): + settings, per-host status and exception columns. +- [SharedMergeTree](https://clickhouse.com/docs/products/cloud/features/infrastructure/shared-merge-tree): + shared mutation work and metadata visibility. +- Local SQL/output: `tests/integration/waiting_spike_probe.py` and the two scripts + linked above. These probes are not the `up` implementation and do not imply that + its acceptance tests pass yet. + +This note does not authorize Cloud use or release a version. Its approved +unknown-outcome rule does not claim an unbounded exactly-once protocol from +bounded server records. diff --git a/docs/design/spikes/2026-10-02-waiting/resume_fence.py b/docs/design/spikes/2026-10-02-waiting/resume_fence.py new file mode 100644 index 0000000..668a9cd --- /dev/null +++ b/docs/design/spikes/2026-10-02-waiting/resume_fence.py @@ -0,0 +1,49 @@ +"""Recover only from the pre-send intent and server marker; never resubmit SQL.""" + +import time +from pathlib import Path + +from clickhouse_alembic.config import get_env_config +from clickhouse_alembic.connection import get_client + +config = get_env_config("it", Path("config.yaml")) +client = get_client(config) +database = config["database"] +try: + token, expected_uuid = client.query( + f"SELECT token, table_uuid FROM {database}.waiting_intent" + ).result_rows[0] + actual_uuid = client.query( + "SELECT uuid FROM system.tables WHERE database = {db:String} AND name = 'counter'", + parameters={"db": database}, + ).result_rows[0][0] + assert str(actual_uuid) == expected_uuid + markers = client.query( + "SELECT mutation_id FROM system.mutations WHERE database = {db:String} " + "AND table = 'counter' AND position(command, {token:String}) > 0", + parameters={"db": database, "token": token}, + ).result_rows + assert len(markers) == 1 + marker = markers[0][0] + Path("resumed_marker").write_text(marker) + deadline = time.monotonic() + 20 + while time.monotonic() < deadline: + rows = client.query( + "SELECT mutation_id, command, is_done FROM system.mutations " + "WHERE database = {db:String} AND table = 'counter' ORDER BY mutation_id", + parameters={"db": database}, + ).result_rows + if all(row[2] for row in rows): + value = client.command(f"SELECT x FROM {database}.counter") + ids = {row[0] for row in rows} + print( + f"resumed {marker}; mutation_ids={sorted(ids)}; command_rows={len(rows)}; x={value}", + flush=True, + ) + assert value == 1 and ids == {marker} and len(rows) == 2 + break + time.sleep(0.05) + else: + raise TimeoutError("Experiment did not finish") +finally: + client.close() diff --git a/docs/design/spikes/2026-10-02-waiting/submit_without_ack.py b/docs/design/spikes/2026-10-02-waiting/submit_without_ack.py new file mode 100644 index 0000000..c8d2742 --- /dev/null +++ b/docs/design/spikes/2026-10-02-waiting/submit_without_ack.py @@ -0,0 +1,30 @@ +"""Submit one owned ALTER over HTTP without ever reading its response.""" + +import http.client +import sys +import time +from pathlib import Path +from urllib.parse import urlencode + +from clickhouse_alembic.config import get_env_config + +config = get_env_config("it", Path("config.yaml")) +token = sys.argv[1] +query = ( + f"ALTER TABLE {config['database']}.counter UPDATE x = x + 1 WHERE 1, " + f"DELETE WHERE 0 AND '{token}' = '{token}'" +) +connection_type = ( + http.client.HTTPSConnection if config.get("secure") else http.client.HTTPConnection +) +connection = connection_type(config["host"], config["port"], timeout=30) +parameters = urlencode({"mutations_sync": 0, "alter_sync": 0, "query_id": token}) +connection.request( + "POST", + "/?" + parameters, + body=query.encode(), + headers={"X-ClickHouse-User": config["migration_user"], "X-ClickHouse-Key": config["password"]}, +) +# Deliberately never call getresponse() or write a mutation-id receipt. +Path("submitted_without_ack").write_text("sent") +time.sleep(60) diff --git a/tests/integration/conftest.py b/tests/integration/conftest.py index 3fb011d..a30d5a7 100644 --- a/tests/integration/conftest.py +++ b/tests/integration/conftest.py @@ -39,7 +39,8 @@ def connect(self): password=self.password, secure=self.secure, connect_timeout=5, - send_receive_timeout=15, + # Cloud DDL such as DROP DATABASE SYNC can take longer than a local container. + send_receive_timeout=60 if self.secure else 15, ) @@ -81,6 +82,14 @@ class MigrationProject: database: str cloud: bool = False + # SharedMergeTree lists metadata-only ALTERs in system.mutations with no block numbers + # (DRY-1446). They rewrite no parts, so tests count only part-rewriting mutations. + # Needs {db:String} and {table:String} parameters. + REWRITES_PARTS = ( + "NOT (empty(block_numbers.number) AND startsWith((SELECT engine FROM system.tables " + "WHERE database = {db:String} AND name = {table:String}), 'Shared'))" + ) + @property def sql_dir(self): return self.root / "migrations" / "sql" @@ -126,11 +135,35 @@ def query_log(self): return "system.query_log" def everywhere(self, table): - """FROM source for a Memory table: on Cloud its rows live only on the writing replica.""" + """FROM source for a per-node table (Memory, most system tables) on every Cloud replica.""" if self.cloud: return f"clusterAllReplicas('default', {table})" return table + def flush_logs(self): + """Flush query logs on every replica; Cloud keeps them per node.""" + on = " ON CLUSTER 'default'" if self.cloud else "" + self.client.command(f"SYSTEM FLUSH LOGS{on}") + + def drain_async_queue(self, table): + """Flush `table`'s async queue; Cloud forbids the flush, so wait for it to empty.""" + if not self.cloud: + self.client.command(f"SYSTEM FLUSH ASYNC INSERT QUEUE {self.database}.{table}") + return + deadline = time.monotonic() + 60 + while self.client.command( + f"SELECT count() FROM {self.everywhere('system.asynchronous_inserts')} " + "WHERE database = {db:String} AND table = {table:String}", + parameters={"db": self.database, "table": table}, + ): + if time.monotonic() > deadline: + pytest.fail(f"Async insert queue for {table} did not drain within 60 seconds") + time.sleep(0.2) + + def budget(self, seconds): + """An `up --timeout` value; Cloud's all-replica checks and grace need at least a minute.""" + return str(max(seconds, 60) if self.cloud else seconds) + @staticmethod def new_password(): # Cloud's password policy needs an uppercase letter and a special character. diff --git a/tests/integration/test_cloud.py b/tests/integration/test_cloud.py index b5969be..15dcae5 100644 --- a/tests/integration/test_cloud.py +++ b/tests/integration/test_cloud.py @@ -1,5 +1,9 @@ """ClickHouse Cloud-only checks; they skip on self-hosted servers (DRY-1413).""" +import subprocess +import sys +import time + import pytest pytestmark = [pytest.mark.integration, pytest.mark.cloud] @@ -61,3 +65,110 @@ def test_cloud_version_table_is_shared_and_round_trips(project): parameters={"db": project.database}, ) assert engine == "SharedMergeTree" + + +def test_cloud_up_advances_version_only_after_mutation_finishes(project): + """Cloud cannot hold a mutation with STOP MERGES; a deliberately slow one shows the wait.""" + db = project.database + _slow_update(project, "x + 1", seconds=2) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + assert ( + project.client.command( + f"SELECT x FROM {db}.counter", settings={"select_sequential_consistency": 1} + ) + == 1 + ) + mutated, versioned = _part_log_times(project) + print(f"\nCLOUD_WAIT mutation_done={mutated} version_row={versioned}") + assert mutated.year > 1970, "part_log shows no MutatePart for counter; waiting unproven" + assert versioned >= mutated + + +def test_cloud_killed_up_waits_for_its_mutation_then_runs_again(project): + """SIGKILL mid-wait; the waived UPDATE's rerun stops until named, then waits and resends.""" + db = project.database + _slow_update(project, "1", seconds=3) + _kill_up_once_its_mutation_runs(project) + stopped = project.run("up", "it") + assert stopped.exit_code == 1, stopped.output + assert "was interrupted on an earlier run" in stopped.output + resumed = project.run("up", "it", "--rerun-interrupted", "aaaa") + assert resumed.exit_code == 0, resumed.output + value = project.client.command( + f"SELECT x FROM {db}.counter", settings={"select_sequential_consistency": 1} + ) + rewrites = project.client.query( + "SELECT create_time, is_done FROM system.mutations " + "WHERE database = {db:String} AND table = {table:String} AND " + + project.REWRITES_PARTS + + " ORDER BY create_time", + parameters={"db": db, "table": "counter"}, + ).result_rows + print(f"\nCLOUD_SIGKILL value={value} mutations={len(rewrites)}") + assert value == 1 and len(rewrites) == 2 and all(done for _, done in rewrites) + assert project.client.query(f"SELECT version_num FROM {db}.alembic_version").result_rows == [ + ("aaaa",) + ] + + +def _slow_update(project, value, seconds): + db = project.database + project.client.command( + f"CREATE TABLE {db}.counter (id UInt64, x UInt64) ENGINE = MergeTree ORDER BY id" + ) + project.client.command(f"INSERT INTO {db}.counter VALUES (1, 0)") + (project.sql_dir / "slow.sql").write_text( + "-- ch-migrate: allow-non-idempotent One controlled, slow update\n" + f"ALTER TABLE {{db}}.counter UPDATE x = {value} + sleepEachRow({seconds}) WHERE id = 1;\n" + ) + project.write_revision( + "aaaa", {"upgrade": "from ch_migrate import run_sql\nrun_sql('slow.sql')"} + ) + + +def _kill_up_once_its_mutation_runs(project): + with (project.root / "first-up.log").open("w") as log: + first = subprocess.Popen( + [sys.executable, "-m", "ch_migrate.cli", "up", "it"], + cwd=project.root, + stdout=log, + stderr=subprocess.STDOUT, + ) + try: + _until_running_mutation(project) + finally: + first.kill() + first.wait(timeout=10) + + +def _until_running_mutation(project): + deadline = time.monotonic() + 60 + while time.monotonic() < deadline: + pending = project.client.command( + "SELECT count() FROM system.mutations WHERE database = {db:String} " + "AND table = 'counter' AND NOT is_done", + parameters={"db": project.database}, + ) + if pending: + return + time.sleep(0.05) + pytest.fail("The slow UPDATE was never seen running") + + +def _part_log_times(project): + """Latest mutated `counter` part and the version row's part, across every replica.""" + deadline = time.monotonic() + 60 + while True: + project.client.command("SYSTEM FLUSH LOGS") + mutated, versioned = project.client.query( + "SELECT maxIf(event_time_microseconds, table = 'counter' AND event_type = 'MutatePart'), " + "maxIf(event_time_microseconds, table = 'alembic_version' AND event_type = 'NewPart') " + "FROM clusterAllReplicas('default', merge('system', '^part_log')) " + "WHERE database = {db:String}", + parameters={"db": project.database}, + settings={"skip_unavailable_shards": 1}, + ).result_rows[0] + if (mutated.year > 1970 and versioned.year > 1970) or time.monotonic() > deadline: + return mutated, versioned + time.sleep(2) diff --git a/tests/integration/test_lint_pending.py b/tests/integration/test_lint_pending.py index d549542..5abdaad 100644 --- a/tests/integration/test_lint_pending.py +++ b/tests/integration/test_lint_pending.py @@ -12,9 +12,7 @@ def test_lint_pending_reports_only_second_revision(project, monkeypatch): (project.sql_dir / "b.sql").write_text( "-- pending change\n\nDROP TABLE IF EXISTS {db}.pending_only;\n" ) - project.write_revision( - "aaaa", {"upgrade": "from ch_migrate import run_sql\nrun_sql('a.sql')"} - ) + project.write_revision("aaaa", {"upgrade": "from ch_migrate import run_sql\nrun_sql('a.sql')"}) project.write_revision( "bbbb", {"upgrade": "from ch_migrate import run_sql\nrun_sql('b.sql')"}, diff --git a/tests/integration/test_mutation_waiting.py b/tests/integration/test_mutation_waiting.py new file mode 100644 index 0000000..3029298 --- /dev/null +++ b/tests/integration/test_mutation_waiting.py @@ -0,0 +1,550 @@ +"""Real-server waiting: `up` records a migration only after the work it started finishes.""" + +import os +import subprocess +import sys +import time +from contextlib import contextmanager + +import pytest +import yaml + +pytestmark = pytest.mark.integration + +# Mutations ClickHouse starts and answers before finishing; STOP MERGES holds each one. +HELD = { + "update": "ALTER TABLE {db}.counter UPDATE x = 1 WHERE id = 1", + "delete": "ALTER TABLE {db}.counter DELETE WHERE id = 5", + "materialize": "ALTER TABLE {db}.counter MATERIALIZE COLUMN doubled", +} + + +@pytest.mark.parametrize("statement", HELD.values(), ids=HELD.keys()) +def test_up_records_a_migration_only_after_its_mutation(project, statement): + _seed(project) + _upgrade_sql(project, statement) + project.stop_merges(f"{project.database}.counter") + try: + held = project.run("up", "it", "--timeout", "1") + assert held.exit_code == 1 and "Timed out" in held.output, held.output + assert "parts_to_do=" in held.output + assert _heads(project) == ["aaaa"] + finally: + project.client.command(f"SYSTEM START MERGES {project.database}.counter") + resumed = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert resumed.exit_code == 0, resumed.output + assert all(row[2] for row in _mutations(project)) + assert _heads(project) == ["bbbb"] + + +def test_interrupted_step_with_a_waiver_stops_until_it_is_named(project): + """A waived statement may already have applied, so a rerun needs someone to check first.""" + db = project.database + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x = x + 1 WHERE id = 1") + project.stop_merges(f"{db}.counter") + try: + held = project.run("up", "it", "--timeout", "1") + assert held.exit_code == 1 and "Timed out" in held.output, held.output + finally: + project.client.command(f"SYSTEM START MERGES {db}.counter") + _until(lambda: all(row[2] for row in _mutations(project)), "first increment did not finish") + stopped = project.run("up", "it") + assert stopped.exit_code == 1, stopped.output + assert "was interrupted on an earlier run" in stopped.output + assert "waiver: Controlled waiting acceptance scenario" in stopped.output + assert "--rerun-interrupted bbbb" in stopped.output + assert project.client.command(f"SELECT x FROM {db}.counter WHERE id = 1") == 1 + assert _heads(project) == ["aaaa"] + named = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert named.exit_code == 0, named.output + # Naming the revision accepts the repeat: this increment runs a second time. + assert project.client.command(f"SELECT x FROM {db}.counter WHERE id = 1") == 2 + assert _markers(project) == 0 + assert _heads(project) == ["bbbb"] + + +def test_interrupted_step_the_gate_passes_runs_again_on_its_own(project): + db = project.database + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter MATERIALIZE COLUMN doubled", waived=False) + project.stop_merges(f"{db}.counter") + try: + held = project.run("up", "it", "--timeout", "1") + assert held.exit_code == 1 and "Timed out" in held.output, held.output + finally: + project.client.command(f"SYSTEM START MERGES {db}.counter") + resumed = project.run("up", "it") + assert resumed.exit_code == 0, resumed.output + assert "was interrupted on an earlier run; running it again" in resumed.output + assert _markers(project) == 0 + assert _heads(project) == ["bbbb"] + + +def test_completed_steps_leave_no_marker(project): + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter MATERIALIZE COLUMN doubled", waived=False) + assert project.run("up", "it").exit_code == 0 + assert _markers(project) == 0 + + +def test_direct_alembic_upgrade_runs_the_gate(project): + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x = x + 1 WHERE id = 1", waived=False) + result = subprocess.run( + [sys.executable, "-m", "alembic", "upgrade", "head"], + cwd=project.root, + env={**os.environ, "CH_ENVIRONMENT": "it"}, + capture_output=True, + text=True, + timeout=120, + ) + assert result.returncode == 1, result.stdout + result.stderr + assert "Migration gate failed; nothing was applied." in result.stderr + assert "[idempotency]" in result.stdout + result.stderr + assert project.client.command(f"SELECT x FROM {project.database}.counter WHERE id = 1") == 0 + assert _heads(project) == ["aaaa"] + + +def test_interrupted_downgrade_with_a_waiver_stops_until_it_is_named(project): + db = project.database + _seed(project) + project.write_revision( + "bbbb", + { + "upgrade": 'op.execute("SELECT 1")', + "downgrade": "# ch-migrate: allow-non-idempotent Controlled downgrade backfill\n" + "op.execute(f'ALTER TABLE {db}.counter UPDATE x = x + 1 WHERE id = 5')", + }, + "aaaa", + ) + assert project.run("up", "it").exit_code == 0 + project.stop_merges(f"{db}.counter") + try: + with _running(project, ("down", "it")) as (process, log): + _until(lambda: _mutations(project), "downgrade mutation was not submitted") + process.kill() + finally: + project.client.command(f"SYSTEM START MERGES {db}.counter") + _until(lambda: all(row[2] for row in _mutations(project)), "downgrade mutation did not finish") + stopped = project.run("down", "it") + assert stopped.exit_code == 1, stopped.output + assert "was interrupted on an earlier run" in stopped.output + assert _heads(project) == ["bbbb"] + named = project.run("down", "it", "--rerun-interrupted", "bbbb") + assert named.exit_code == 0, named.output + assert _heads(project) == ["aaaa"] + + +def test_failed_mutation_is_not_recorded_and_names_its_kill(project): + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x = throwIf(x = 5) WHERE 1") + result = project.run("up", "it") + assert result.exit_code == 1, result.output + # On Cloud this client may reach a replica that hasn't received the failure reason yet. + failed = _until( + lambda: next((row for row in _mutations(project) if row[4]), None), + "the failed mutation's reason is not visible", + ) + assert failed[0] in result.output and "throwIf" in result.output + assert f"mutation_id = '{failed[0]}'" in result.output + assert _heads(project) == ["aaaa"] + + +def test_killed_mutation_then_fixed_sql_runs_again(project): + """DRY-1604: after the kill, fixing the file and running `up` is the whole recovery.""" + db = project.database + project.client.command( + f"CREATE TABLE {db}.orders (id UInt64, raw String, qty UInt64) ENGINE=MergeTree ORDER BY id" + ) + project.client.command(f"INSERT INTO {db}.orders VALUES (1, '3', 0), (2, 'n/a', 0)") + _baseline(project) + path = _upgrade_sql(project, "ALTER TABLE {db}.orders UPDATE qty = toUInt64(raw) WHERE 1") + failed = project.run("up", "it") + assert failed.exit_code == 1 and "Cannot parse" in failed.output, failed.output + blocked = project.run("up", "it") + assert blocked.exit_code == 1, blocked.output + assert "started before this run" in blocked.output and "KILL MUTATION" in blocked.output + mutation = next(row[0] for row in _mutations(project, "orders") if row[4]) + project.client.command( + "KILL MUTATION WHERE database = {db:String} AND mutation_id = {mutation:String}", + parameters={"db": db, "mutation": mutation}, + ) + _until(lambda: not any(row[4] for row in _mutations(project, "orders")), "kill did not land") + _write_sql(path, "ALTER TABLE {db}.orders UPDATE qty = toUInt64OrZero(raw) WHERE 1") + # The killed backfill may have rewritten some parts, so the waived rerun is named. + stopped = project.run("up", "it") + assert stopped.exit_code == 1 and "was interrupted on an earlier run" in stopped.output + fixed = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert fixed.exit_code == 0, fixed.output + rows = project.client.query(f"SELECT id, qty FROM {db}.orders ORDER BY id").result_rows + assert rows == [(1, 3), (2, 0)] + assert _heads(project) == ["bbbb"] + + +def test_rejected_statement_can_be_fixed_and_run_again(project): + """DRY-1604: ClickHouse refuses a sorting-key change; the corrected file then applies.""" + _seed(project) + path = _upgrade_sql(project, "ALTER TABLE {db}.counter MODIFY COLUMN id String") + rejected = project.run("up", "it") + assert ( + rejected.exit_code == 1 and "ALTER_OF_COLUMN_IS_FORBIDDEN" in rejected.output + ), rejected.output + assert _heads(project) == ["aaaa"] + _write_sql(path, "ALTER TABLE {db}.counter ADD COLUMN IF NOT EXISTS note String", waived=False) + fixed = project.run("up", "it") + assert fixed.exit_code == 0, fixed.output + assert _heads(project) == ["bbbb"] + + +def test_unfinished_earlier_mutation_holds_the_run(project): + """Work already queued on the database finishes before any migration statement is sent.""" + db = project.database + _seed(project) + project.client.command(f"CREATE TABLE {db}.observed (value UInt8) ENGINE=Memory") + _upgrade_sql(project, "INSERT INTO {db}.observed VALUES (1)") + project.stop_merges(f"{db}.counter") + try: + project.client.command(f"ALTER TABLE {db}.counter UPDATE x = 7 WHERE id = 5") + held = project.run("up", "it", "--timeout", "1") + assert held.exit_code == 1 and "Timed out" in held.output, held.output + assert project.client.command(f"SELECT count() FROM {db}.observed") == 0 + assert _heads(project) == ["aaaa"] + finally: + project.client.command(f"SYSTEM START MERGES {db}.counter") + resumed = project.run("up", "it") + assert resumed.exit_code == 0, resumed.output + assert project.client.command(f"SELECT count() FROM {db}.observed") == 1 + assert _heads(project) == ["bbbb"] + + +def test_down_waits_for_its_mutation(project): + db = project.database + _seed(project) + project.write_revision( + "bbbb", + { + "upgrade": 'op.execute("SELECT 1")', + "downgrade": "# ch-migrate: allow-non-idempotent Controlled downgrade backfill\n" + "op.execute(f'ALTER TABLE {db}.counter UPDATE x = 0 WHERE id = 5')", + }, + "aaaa", + ) + assert project.run("up", "it").exit_code == 0 + project.stop_merges(f"{db}.counter") + try: + with _running(project, ("down", "it")) as (process, log): + _until(lambda: _mutations(project), "downgrade mutation was not submitted") + time.sleep(1) + assert process.poll() is None, log.read_text() + project.client.command(f"SYSTEM START MERGES {db}.counter") + assert process.wait(timeout=30) == 0, log.read_text() + finally: + project.client.command(f"SYSTEM START MERGES {db}.counter") + assert all(row[2] for row in _mutations(project)) + assert _heads(project) == ["aaaa"] + + +def test_mutation_polling_preserves_session_settings(project): + _seed(project) + project.client.command(f"CREATE TABLE {project.database}.observed (value UInt64) ENGINE=Memory") + config_path = project.root / "config.yaml" + config = yaml.safe_load(config_path.read_text()) + config["environments"]["it"]["session_timeout"] = 1 + config_path.write_text(yaml.safe_dump(config)) + _upgrade_sql( + project, + "SET max_threads=3;\n" + "-- ch-migrate: allow-non-idempotent increment once\n" + "ALTER TABLE {db}.counter UPDATE x=x+1 WHERE id=1;\n" + "-- ch-migrate: allow-non-idempotent record the preserved session setting\n" + "INSERT INTO {db}.observed SELECT getSetting('max_threads')", + ) + project.stop_merges(f"{project.database}.counter") + try: + with _running(project, ("up", "it")) as (process, log): + mutation = _until(lambda: _mutations(project), "mutation was not submitted")[0][0] + _until(lambda: mutation in log.read_text(), "wait was not reported") + time.sleep(2.2) # Longer than the server session timeout, while polling continues. + assert process.poll() is None, log.read_text() + project.client.command(f"SYSTEM START MERGES {project.database}.counter") + assert process.wait(timeout=30) == 0, log.read_text() + assert project.client.query( + f"SELECT value FROM {project.database}.observed" + ).result_rows == [(3,)] + finally: + project.client.command(f"SYSTEM START MERGES {project.database}.counter") + + +@pytest.mark.parametrize("mode", ["execute", "driver", "executemany"]) +def test_mutation_raw_bind_paths_are_waited(project, mode): + _seed(project) + if mode == "execute": + body = ( + "from sqlalchemy import text\n" + "# ch-migrate: allow-non-idempotent bound counter update\n" + "op.get_bind().execute(text(f'ALTER TABLE {db}.counter UPDATE x=x+:delta WHERE id=:key'), {'delta': 1, 'key': 1})" + ) + else: + values = ( + "[{'delta': 1, 'key': 1}, {'delta': 1, 'key': 5}]" + if mode == "executemany" + else "{'delta': 1, 'key': 1}" + ) + body = ( + "# ch-migrate: allow-non-idempotent bound counter update\n" + f"op.get_bind().exec_driver_sql(f'ALTER TABLE {{db}}.counter UPDATE x=x+%(delta)s WHERE id=%(key)s', {values})" + ) + project.write_revision("bbbb", {"upgrade": body}, "aaaa") + result = project.run("up", "it") + assert result.exit_code == 0, result.output + assert all(row[2] for row in _mutations(project)) + assert project.client.query( + f"SELECT id, x FROM {project.database}.counter ORDER BY id" + ).result_rows == [(1, 1), (5, 6 if mode == "executemany" else 5)] + assert _heads(project) == ["bbbb"] + + +def test_mutation_piped_progress_prints_plain_lines(project): + """CI logs and agents get a start line naming the mutation, never a redrawn bar.""" + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x=x+1 WHERE id=1") + project.stop_merges(f"{project.database}.counter") + try: + result = subprocess.run( + [sys.executable, "-m", "ch_migrate.cli", "up", "it", "--timeout", "1"], + cwd=project.root, + env={**os.environ, "TERM": "xterm-256color"}, + capture_output=True, + text=True, + timeout=60, + ) + assert result.returncode == 1, result.stderr + start = f" waiting on mutation_2.txt ({project.database}.counter): 0/1 parts" + assert start in result.stderr.splitlines(), result.stderr + assert "\x1b" not in result.stderr and "\r" not in result.stderr + assert "Run ch-migrate up it again: it waits for that work to finish" in result.stderr + finally: + project.client.command(f"SYSTEM START MERGES {project.database}.counter") + assert _heads(project) == ["aaaa"] + + +@pytest.mark.skipif(os.name != "posix", reason="A real PTY is required for terminal progress") +def test_mutation_terminal_progress_uses_a_live_line(project): + import pty + import select + + _seed(project) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x=x+1 WHERE id=1") + project.stop_merges(f"{project.database}.counter") + master, slave = pty.openpty() + process = None + try: + process = subprocess.Popen( + [sys.executable, "-m", "ch_migrate.cli", "up", "it", "--timeout", "0.8"], + cwd=project.root, + env={**os.environ, "TERM": "xterm-256color"}, + stdout=subprocess.DEVNULL, + stderr=slave, + ) + # Keep the PTY alive and drain it while the child writes; do not depend + # on buffered output surviving the final slave close. + chunks = [] + deadline = time.monotonic() + 30 + while process.poll() is None or select.select([master], [], [], 0)[0]: + assert time.monotonic() < deadline, "terminal run did not finish" + if select.select([master], [], [], 0.2)[0]: + chunks.append(os.read(master, 4096)) + assert process.wait(timeout=5) == 1 + output = b"".join(chunks) + # The live bar redraws in place; the timeout still names what was pending. + assert "━".encode() in output and b" parts" in output + assert b"Timed out" in output and b"parts_to_do=" in output + assert _heads(project) == ["aaaa"] + finally: + if process is not None and process.poll() is None: + process.kill() + process.wait(timeout=10) + if slave is not None: + os.close(slave) + os.close(master) + project.client.command(f"SYSTEM START MERGES {project.database}.counter") + + +def test_mutation_lightweight_update_finishes_without_fabricating_a_mutation(project): + project.client.command( + f"CREATE TABLE {project.database}.patches (id UInt64, x UInt64) " + "ENGINE=MergeTree ORDER BY id SETTINGS enable_block_number_column=1, " + "enable_block_offset_column=1" + ) + project.client.command(f"INSERT INTO {project.database}.patches VALUES (1, 0)") + project.write_revision( + "aaaa", + { + "upgrade": 'op.execute("SET allow_experimental_lightweight_update=1")\n' + "# ch-migrate: allow-non-idempotent apply one synchronous patch\n" + 'op.get_bind().exec_driver_sql(f"UPDATE {db}.patches SET x=x+1 WHERE id=1")' + }, + ) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + assert project.client.command(f"SELECT x FROM {project.database}.patches") == 1 + assert _mutations(project, "patches") == [] + assert _heads(project) == ["aaaa"] + + +def test_mutation_waits_for_every_replica(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _seed_cluster(project, cluster) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x = x + 1 WHERE id = 1") + table = f"{project.database}.counter" + for action in ("MERGES", "FETCHES", "REPLICATION QUEUES"): + cluster.clients[2].command(f"SYSTEM STOP {action} {table}") + try: + with _running(project, ("up", "it")) as (process, log): + _until(lambda: _mutations(project), "replicated mutation was not submitted") + cluster.clients[2].command(f"SYSTEM SYNC REPLICA {table} PULL") + _until(lambda: all(row[2] for row in _mutations(project)), "initiator did not finish") + second = ( + cluster.clients[2] + .query( + "SELECT is_done FROM system.mutations WHERE database = {db:String} AND table = 'counter'", + parameters={"db": project.database}, + ) + .result_rows + ) + assert second and all(not row[0] for row in second) + _until( + lambda: "waiting on mutation" in log.read_text(), "replica wait was not reported" + ) + assert process.poll() is None + for action in ("MERGES", "FETCHES", "REPLICATION QUEUES"): + cluster.clients[2].command(f"SYSTEM START {action} {table}") + assert process.wait(timeout=30) == 0, log.read_text() + for client in cluster.clients.values(): + assert client.command(f"SELECT x FROM {table} WHERE id = 1") == 1 + finally: + for action in ("MERGES", "FETCHES", "REPLICATION QUEUES"): + cluster.clients[2].command(f"SYSTEM START {action} {table}") + + +def test_mutation_on_a_down_replica_is_not_success(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _seed_cluster(project, cluster) + _upgrade_sql(project, "ALTER TABLE {db}.counter UPDATE x = 1 WHERE id = 1") + table = f"{project.database}.counter" + for client in cluster.clients.values(): + client.command(f"SYSTEM STOP MERGES {table}") + try: + first = project.run("up", "it", "--timeout", "0.5") + assert first.exit_code == 1, first.output + cluster.stop_node(2) + result = project.run("up", "it", "--timeout", "1") + assert result.exit_code == 1 and "Timed out" in result.output, result.output + assert "shows 1/2 replicas" in result.output + assert _heads(project) == ["aaaa"] + finally: + if 2 in cluster.stopped: + cluster.start_node(2) + for client in cluster.clients.values(): + client.command(f"SYSTEM START MERGES {table}") + resumed = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert resumed.exit_code == 0, resumed.output + assert _heads(project) == ["bbbb"] + + +def _seed_cluster(project, cluster): + table = f"{project.database}.counter" + cluster.clients[1].command( + f"CREATE TABLE {table} ON CLUSTER {cluster.name} (id UInt64, x UInt64) " + f"ENGINE=ReplicatedMergeTree('/clickhouse/waiting/{project.database}/counter', '{{replica}}') ORDER BY id" + ) + cluster.clients[1].command(f"INSERT INTO {table} VALUES (1, 0), (5, 5)") + cluster.clients[2].command(f"SYSTEM SYNC REPLICA {table}") + project.write_revision("aaaa", {"upgrade": 'op.execute("SELECT 1")'}) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + + +def _seed(project): + project.client.command( + f"CREATE TABLE {project.database}.counter (id UInt64, x UInt64, " + "doubled UInt64 DEFAULT x * 2) ENGINE=MergeTree ORDER BY id" + ) + project.client.command(f"INSERT INTO {project.database}.counter (id, x) VALUES (1, 0), (5, 5)") + _baseline(project) + + +def _baseline(project): + project.write_revision("aaaa", {"upgrade": 'op.execute("SELECT 1")'}) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + + +def _upgrade_sql(project, sql, waived=True): + path = project.sql_dir / "mutation.sql" + _write_sql(path, sql, waived) + project.write_revision( + "bbbb", + {"upgrade": "from ch_migrate import run_sql\nrun_sql('mutation.sql')"}, + "aaaa", + ) + return path + + +def _write_sql(path, sql, waived=True): + waiver = "-- ch-migrate: allow-non-idempotent Controlled waiting acceptance scenario\n" + path.write_text((waiver if waived else "") + sql + ";\n") + + +def _markers(project): + return project.client.command(f"SELECT count() FROM {project.database}._ch_migrate_in_progress") + + +def _mutations(project, table="counter"): + return project.client.query( + "SELECT mutation_id, command, is_done, parts_to_do, latest_fail_reason FROM system.mutations " + "WHERE database = {db:String} AND table = {table:String} AND " + + project.REWRITES_PARTS + + " ORDER BY mutation_id", + parameters={"db": project.database, "table": table}, + ).result_rows + + +def _heads(project): + return [ + row[0] + for row in project.client.query( + f"SELECT version_num FROM {project.database}.alembic_version ORDER BY version_num" + ).result_rows + ] + + +def _until(predicate, message): + deadline = time.monotonic() + 20 + while time.monotonic() < deadline: + value = predicate() + if value: + return value + time.sleep(0.05) + pytest.fail(message) + + +@contextmanager +def _running(project, args, code=None): + path = project.root / f"run-{time.monotonic_ns()}.log" + command = [sys.executable, "-c", code] if code else [sys.executable, "-m", "ch_migrate.cli"] + with path.open("w") as log: + process = subprocess.Popen( + [*command, *args], + cwd=project.root, + stdout=log, + stderr=subprocess.STDOUT, + env=os.environ.copy(), + ) + try: + yield process, path + finally: + if process.poll() is None: + process.kill() + process.wait(timeout=10) diff --git a/tests/integration/test_on_cluster.py b/tests/integration/test_on_cluster.py new file mode 100644 index 0000000..ed345bb --- /dev/null +++ b/tests/integration/test_on_cluster.py @@ -0,0 +1,199 @@ +"""ON CLUSTER statements finish on every host before a migration is recorded.""" + +import pytest + +pytestmark = pytest.mark.integration +NOT_FINISHED = "is not finished on 1 of 2 hosts" + + +def test_on_cluster_create_then_alter_finishes_every_host(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + _revision( + project, + [ + f"CREATE TABLE IF NOT EXISTS {{db}}.owned ON CLUSTER {cluster.name} (id UInt64) ENGINE=Memory", + f"ALTER TABLE {{db}}.owned ON CLUSTER {cluster.name} ADD COLUMN IF NOT EXISTS value String", + ], + ) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + for client in cluster.clients.values(): + assert client.query( + "SELECT name FROM system.columns WHERE database={db:String} AND table='owned' ORDER BY position", + parameters={"db": project.database}, + ).result_rows == [("id",), ("value",)] + assert _heads(project) == ["bbbb"] + + +def test_on_cluster_down_host_times_out_then_runs_again(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + _revision( + project, + [ + f"CREATE TABLE IF NOT EXISTS {{db}}.owned ON CLUSTER {cluster.name} (id UInt64) ENGINE=Memory" + ], + waived=False, + ) + cluster.stop_node(2) + try: + result = project.run("up", "it", "--timeout", "2") + assert result.exit_code == 1 and NOT_FINISHED in result.output, result.output + assert _heads(project) == ["aaaa"] + finally: + cluster.start_node(2) + resumed = project.run("up", "it") + assert resumed.exit_code == 0, resumed.output + for client in cluster.clients.values(): + assert client.command(f"EXISTS TABLE {project.database}.owned") == 1 + assert _heads(project) == ["bbbb"] + + +def test_on_cluster_host_exception_fails_the_run(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + table = f"{project.database}.owned" + project.client.command( + f"CREATE TABLE {table} ON CLUSTER {cluster.name} (id UInt64) ENGINE=Memory" + ) + cluster.clients[2].command(f"ALTER TABLE {table} ADD COLUMN conflict String") + _revision( + project, [f"ALTER TABLE {{db}}.owned ON CLUSTER {cluster.name} ADD COLUMN conflict UInt64"] + ) + result = project.run("up", "it") + assert result.exit_code == 1, result.output + assert cluster.hostnames[2] in result.output and "DUPLICATE_COLUMN" in result.output + assert _heads(project) == ["aaaa"] + + +def test_on_cluster_and_replicated_mutation_both_must_finish(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + table = f"{project.database}.owned" + project.client.command( + f"CREATE TABLE {table} ON CLUSTER {cluster.name} (id UInt64, x UInt64) " + f"ENGINE=ReplicatedMergeTree('/clickhouse/ddl_wait/{project.database}', '{{replica}}') ORDER BY id" + ) + project.client.command(f"INSERT INTO {table} VALUES (1, 0)") + cluster.clients[2].command(f"SYSTEM SYNC REPLICA {table}") + _revision( + project, [f"ALTER TABLE {{db}}.owned ON CLUSTER {cluster.name} UPDATE x=1 WHERE id=1"] + ) + for client in cluster.clients.values(): + client.command(f"SYSTEM STOP MERGES {table}") + try: + # Long enough for the queue entry to finish on both hosts on a slow CI runner, so the + # deadline falls in the mutation wait that STOP MERGES holds open. + result = project.run("up", "it", "--timeout", "5") + assert result.exit_code == 1 and "parts_to_do=" in result.output, result.output + assert _heads(project) == ["aaaa"] + finally: + for client in cluster.clients.values(): + client.command(f"SYSTEM START MERGES {table}") + resumed = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert resumed.exit_code == 0, resumed.output + for client in cluster.clients.values(): + assert client.command(f"SELECT x FROM {table}") == 1 + assert _heads(project) == ["bbbb"] + + +def test_on_cluster_ttl_materialization_is_waited(cluster_project, clickhouse_cluster): + """ClickHouse holds MODIFY TTL until it's materialized, so the DDL limit bounds that wait.""" + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + table = f"{project.database}.expiry" + project.client.command( + f"CREATE TABLE {table} ON CLUSTER {cluster.name} (ts DateTime) " + f"ENGINE=ReplicatedMergeTree('/clickhouse/ddl_ttl/{project.database}', '{{replica}}') ORDER BY ts" + ) + project.client.command(f"INSERT INTO {table} VALUES ('2020-01-01 00:00:00')") + cluster.clients[2].command(f"SYSTEM SYNC REPLICA {table}") + _revision( + project, + [f"ALTER TABLE {{db}}.expiry ON CLUSTER {cluster.name} MODIFY TTL ts + INTERVAL 100 YEAR"], + waived=False, + ) + for client in cluster.clients.values(): + client.command(f"SYSTEM STOP MERGES {table}") + try: + result = project.run("up", "it", "--timeout", "3") + assert result.exit_code == 1 and "is not finished on" in result.output, result.output + assert _heads(project) == ["aaaa"] + finally: + for client in cluster.clients.values(): + client.command(f"SYSTEM START MERGES {table}") + resumed = project.run("up", "it") + assert resumed.exit_code == 0, resumed.output + for client in cluster.clients.values(): + assert not client.command( + "SELECT count() FROM system.mutations WHERE database = {db:String} AND NOT is_done", + parameters={"db": project.database}, + ) + assert _heads(project) == ["bbbb"] + + +def test_on_cluster_nonreplicated_mutations_wait_on_each_host(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + _baseline(project) + table = f"{project.database}.owned" + project.client.command( + f"CREATE TABLE {table} ON CLUSTER {cluster.name} (x UInt64) ENGINE=MergeTree ORDER BY tuple()" + ) + for client in cluster.clients.values(): + client.command(f"INSERT INTO {table} VALUES (0)") + _revision(project, [f"ALTER TABLE {{db}}.owned ON CLUSTER {cluster.name} UPDATE x=1 WHERE 1"]) + cluster.clients[2].command(f"SYSTEM STOP MERGES {table}") + try: + result = project.run("up", "it", "--timeout", "2") + assert result.exit_code == 1, result.output + assert cluster.hostnames[2] in result.output + assert _heads(project) == ["aaaa"] + assert cluster.clients[1].command(f"SELECT x FROM {table}") == 1 + assert cluster.clients[2].command(f"SELECT x FROM {table}") == 0 + finally: + cluster.clients[2].command(f"SYSTEM START MERGES {table}") + resumed = project.run("up", "it", "--rerun-interrupted", "bbbb") + assert resumed.exit_code == 0, resumed.output + for client in cluster.clients.values(): + assert client.command(f"SELECT x FROM {table}") == 1 + + +def test_on_cluster_cold_version_table_creation_obeys_timeout(cluster_project, clickhouse_cluster): + project, cluster = cluster_project, clickhouse_cluster + project.write_revision("aaaa", {"upgrade": 'op.execute("SELECT 1")'}) + cluster.stop_node(2) + try: + result = project.run("up", "it", "--timeout", "1") + assert result.exit_code == 1 and NOT_FINISHED in result.output, result.output + assert _heads(project) == [] + finally: + cluster.start_node(2) + resumed = project.run("up", "it") + assert resumed.exit_code == 0, resumed.output + assert _heads(project) == ["aaaa"] + + +def _baseline(project): + project.write_revision("aaaa", {"upgrade": 'op.execute("SELECT 1")'}) + result = project.run("up", "it") + assert result.exit_code == 0, result.output + + +def _revision(project, statements, waived=True): + """Waived statements need --rerun-interrupted after an interruption; the rest rerun alone.""" + waiver = "-- ch-migrate: allow-non-idempotent Controlled distributed DDL scenario\n" + path = project.sql_dir / "ddl.sql" + path.write_text("\n".join((waiver if waived else "") + sql + ";" for sql in statements)) + project.write_revision( + "bbbb", {"upgrade": "from ch_migrate import run_sql\nrun_sql('ddl.sql')"}, "aaaa" + ) + + +def _heads(project): + return [ + row[0] + for row in project.client.query( + f"SELECT version_num FROM {project.database}.alembic_version ORDER BY version_num" + ).result_rows + ] diff --git a/tests/test_cli_output.py b/tests/test_cli_output.py index fb6bd5e..e6e66b9 100644 --- a/tests/test_cli_output.py +++ b/tests/test_cli_output.py @@ -7,7 +7,8 @@ from click.testing import CliRunner from ch_migrate import ui -from ch_migrate.cli import _print_gate_findings, main +from ch_migrate.cli import main +from ch_migrate.gate import print_gate_findings from ch_migrate.lint import LintResult, Severity @@ -70,12 +71,9 @@ def test_gate_findings_use_lint_markers(capsys): ), ] ) - assert _print_gate_findings(report) is True + assert print_gate_findings(report) is True captured = capsys.readouterr() - assert ( - captured.err - == "✗ a.up.sql:2 Use IF NOT EXISTS [idempotency]\n CREATE TABLE t\n" - ) + assert captured.err == "✗ a.up.sql:2 Use IF NOT EXISTS [idempotency]\n CREATE TABLE t\n" assert captured.out == ( "! config.yaml Drops a column [destructive_changes]\n" "✓ b.up.sql:5 Idempotency waiver: copies once [idempotency]\n" diff --git a/tests/test_deps.py b/tests/test_deps.py index 36f7feb..00eb217 100644 --- a/tests/test_deps.py +++ b/tests/test_deps.py @@ -34,7 +34,9 @@ def _make_graph() -> DependencyGraph: graph.edges = [ DependencyEdge(source="events", target="hourly_events", dep_type=DepType.DATA_FLOW), DependencyEdge(source="events", target="hourly_events", dep_type=DepType.SCHEMA), - DependencyEdge(source="hourly_events", target="hourly_events_dest", dep_type=DepType.DATA_FLOW), + DependencyEdge( + source="hourly_events", target="hourly_events_dest", dep_type=DepType.DATA_FLOW + ), DependencyEdge(source="users", target="dict_users", dep_type=DepType.SCHEMA), ] return graph @@ -136,6 +138,7 @@ def test_renders_empty_graph(self): def test_shows_edge_count(self): import re as _re + output = StringIO() console = Console(file=output, force_terminal=True, width=100) graph = _make_graph() @@ -207,13 +210,15 @@ def test_handles_circular_dependency(self): @pytest.fixture def deps_runner(tmp_path: Path, monkeypatch): monkeypatch.chdir(tmp_path) - (tmp_path / "config.yaml").write_text(""" + (tmp_path / "config.yaml").write_text( + """ environments: dev: host: localhost database: testdb user: test -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") return CliRunner() @@ -226,6 +231,7 @@ def test_shows_graph(self, mock_deps, mock_client, deps_runner): mock_deps.return_value = _make_graph() from ch_migrate.cli import main + result = deps_runner.invoke(main, ["deps", "dev"]) assert result.exit_code == 0 assert "Dependency Graph" in result.output @@ -240,6 +246,7 @@ def test_validate_safe_migration(self, mock_deps, mock_client, deps_runner, tmp_ sql_file.write_text("ALTER TABLE users ADD COLUMN email String") from ch_migrate.cli import main + result = deps_runner.invoke(main, ["deps", "dev", "--validate", str(sql_file)]) assert result.exit_code == 0 assert "passed" in result.output @@ -254,5 +261,6 @@ def test_validate_breaking_migration(self, mock_deps, mock_client, deps_runner, sql_file.write_text("DROP TABLE events") from ch_migrate.cli import main + result = deps_runner.invoke(main, ["deps", "dev", "--validate", str(sql_file)]) assert result.exit_code == 1 diff --git a/tests/test_diff.py b/tests/test_diff.py index 6ad6996..85926d8 100644 --- a/tests/test_diff.py +++ b/tests/test_diff.py @@ -84,40 +84,52 @@ def test_detects_added_column(self): local = Schema(database="db") live = Schema(database="db") local.tables["users"] = self._base_table() - live.tables["users"] = self._base_table(columns=[ - ColumnDefinition(name="id", type="UInt64"), - ColumnDefinition(name="name", type="String"), - ColumnDefinition(name="email", type="String"), - ]) + live.tables["users"] = self._base_table( + columns=[ + ColumnDefinition(name="id", type="UInt64"), + ColumnDefinition(name="name", type="String"), + ColumnDefinition(name="email", type="String"), + ] + ) diffs = compare_schemas(local, live) modified = [d for d in diffs if d.status == DiffStatus.MODIFIED] assert len(modified) == 1 - assert any("email" in fd.message and "in the database but not in the snapshot" in fd.message for fd in modified[0].field_diffs) + assert any( + "email" in fd.message and "in the database but not in the snapshot" in fd.message + for fd in modified[0].field_diffs + ) def test_detects_removed_column(self): local = Schema(database="db") live = Schema(database="db") - local.tables["users"] = self._base_table(columns=[ - ColumnDefinition(name="id", type="UInt64"), - ColumnDefinition(name="name", type="String"), - ColumnDefinition(name="phone", type="String"), - ]) + local.tables["users"] = self._base_table( + columns=[ + ColumnDefinition(name="id", type="UInt64"), + ColumnDefinition(name="name", type="String"), + ColumnDefinition(name="phone", type="String"), + ] + ) live.tables["users"] = self._base_table() diffs = compare_schemas(local, live) modified = [d for d in diffs if d.status == DiffStatus.MODIFIED] assert len(modified) == 1 - assert any("phone" in fd.message and "in the snapshot but not in the database" in fd.message for fd in modified[0].field_diffs) + assert any( + "phone" in fd.message and "in the snapshot but not in the database" in fd.message + for fd in modified[0].field_diffs + ) def test_detects_column_type_change(self): local = Schema(database="db") live = Schema(database="db") local.tables["users"] = self._base_table() - live.tables["users"] = self._base_table(columns=[ - ColumnDefinition(name="id", type="UInt64"), - ColumnDefinition(name="name", type="Nullable(String)"), - ]) + live.tables["users"] = self._base_table( + columns=[ + ColumnDefinition(name="id", type="UInt64"), + ColumnDefinition(name="name", type="Nullable(String)"), + ] + ) diffs = compare_schemas(local, live) modified = [d for d in diffs if d.status == DiffStatus.MODIFIED] @@ -193,8 +205,12 @@ def test_detects_partition_by_change(self): def test_views_fallback_to_raw_ddl(self): local = Schema(database="db") live = Schema(database="db") - local.views["v"] = ViewDefinition(name="v", select_query="SELECT 1", raw_ddl="CREATE VIEW v AS SELECT 1") - live.views["v"] = ViewDefinition(name="v", select_query="SELECT 2", raw_ddl="CREATE VIEW v AS SELECT 2") + local.views["v"] = ViewDefinition( + name="v", select_query="SELECT 1", raw_ddl="CREATE VIEW v AS SELECT 1" + ) + live.views["v"] = ViewDefinition( + name="v", select_query="SELECT 2", raw_ddl="CREATE VIEW v AS SELECT 2" + ) diffs = compare_schemas(local, live) modified = [d for d in diffs if d.status == DiffStatus.MODIFIED] @@ -218,7 +234,9 @@ def test_mixed_object_types(self): local.tables["t1"] = self._base_table("t1") live.tables["t1"] = self._base_table("t1") - local.views["v1"] = ViewDefinition(name="v1", select_query="SELECT 1", raw_ddl="CREATE VIEW v1 AS SELECT 1") + local.views["v1"] = ViewDefinition( + name="v1", select_query="SELECT 1", raw_ddl="CREATE VIEW v1 AS SELECT 1" + ) # v1 is local_only (not in live) live.dictionaries["d1"] = DictDefinition(name="d1", raw_ddl="CREATE DICTIONARY d1 (...)") @@ -239,13 +257,15 @@ def test_mixed_object_types(self): @pytest.fixture def diff_runner(tmp_path: Path, monkeypatch): monkeypatch.chdir(tmp_path) - (tmp_path / "config.yaml").write_text(""" + (tmp_path / "config.yaml").write_text( + """ environments: dev: host: localhost database: testdb user: test -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") # Create a snapshot @@ -277,6 +297,7 @@ def test_in_sync_exits_0(self, mock_schema, mock_client, diff_runner, tmp_path): mock_schema.return_value = live from ch_migrate.cli import main + result = diff_runner.invoke(main, ["diff", "dev"]) assert result.exit_code == 0 @@ -298,6 +319,7 @@ def test_drift_exits_1(self, mock_schema, mock_client, diff_runner, tmp_path): mock_schema.return_value = live from ch_migrate.cli import main + result = diff_runner.invoke(main, ["diff", "dev"]) assert result.exit_code == 1 @@ -305,16 +327,19 @@ def test_drift_exits_1(self, mock_schema, mock_client, diff_runner, tmp_path): @patch("ch_migrate.introspect.get_live_schema") def test_no_snapshot_exits_1(self, mock_schema, mock_client, tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) - (tmp_path / "config.yaml").write_text(""" + (tmp_path / "config.yaml").write_text( + """ environments: dev: host: localhost database: testdb user: test -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") from ch_migrate.cli import main + runner = CliRunner() result = runner.invoke(main, ["diff", "dev"]) assert result.exit_code == 1 @@ -342,9 +367,14 @@ def test_render_with_drift(self): console = Console(file=output, force_terminal=True, width=80) diffs = [ - SchemaDiff(name="users", obj_type="table", status=DiffStatus.MODIFIED, field_diffs=[ - FieldDiff("engine", "MergeTree", "ReplacingMergeTree", "engine differs"), - ]), + SchemaDiff( + name="users", + obj_type="table", + status=DiffStatus.MODIFIED, + field_diffs=[ + FieldDiff("engine", "MergeTree", "ReplacingMergeTree", "engine differs"), + ], + ), SchemaDiff(name="new_table", obj_type="table", status=DiffStatus.REMOTE_ONLY), SchemaDiff(name="old_view", obj_type="view", status=DiffStatus.LOCAL_ONLY), ] diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 2261a31..9abc30a 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -22,38 +22,46 @@ def test_from_empty_dict(self): assert not registry.has_hooks def test_from_post_migrate_only(self): - registry = HookRegistry.from_config({ - "post_migrate": [ - "SYSTEM RELOAD DICTIONARY {db}.dict_regions ON CLUSTER default", - ] - }) + registry = HookRegistry.from_config( + { + "post_migrate": [ + "SYSTEM RELOAD DICTIONARY {db}.dict_regions ON CLUSTER default", + ] + } + ) assert registry.pre_migrate == [] assert len(registry.post_migrate) == 1 assert registry.has_hooks def test_from_pre_and_post(self): - registry = HookRegistry.from_config({ - "pre_migrate": ["SELECT 1"], - "post_migrate": [ - "SYSTEM RELOAD DICTIONARY {db}.dict_a ON CLUSTER default", - "SELECT count() FROM {db}.users", - ], - }) + registry = HookRegistry.from_config( + { + "pre_migrate": ["SELECT 1"], + "post_migrate": [ + "SYSTEM RELOAD DICTIONARY {db}.dict_a ON CLUSTER default", + "SELECT count() FROM {db}.users", + ], + } + ) assert len(registry.pre_migrate) == 1 assert len(registry.post_migrate) == 2 assert registry.has_hooks def test_coerces_single_string_to_list(self): - registry = HookRegistry.from_config({ - "post_migrate": "SYSTEM RELOAD DICTIONARY {db}.dict_a ON CLUSTER default", - }) + registry = HookRegistry.from_config( + { + "post_migrate": "SYSTEM RELOAD DICTIONARY {db}.dict_a ON CLUSTER default", + } + ) assert len(registry.post_migrate) == 1 def test_handles_none_values(self): - registry = HookRegistry.from_config({ - "pre_migrate": None, - "post_migrate": None, - }) + registry = HookRegistry.from_config( + { + "pre_migrate": None, + "post_migrate": None, + } + ) assert registry.pre_migrate == [] assert registry.post_migrate == [] @@ -72,9 +80,7 @@ def test_executes_hooks_in_order(self): assert connection.commit.call_count == 2 # Verify the SQL was resolved - executed_sql = [ - str(c.args[0]) for c in connection.execute.call_args_list - ] + executed_sql = [str(c.args[0]) for c in connection.execute.call_args_list] assert "SYSTEM RELOAD DICTIONARY mydb.dict_a ON CLUSTER default" in executed_sql[0] assert "SELECT count() FROM mydb.users" in executed_sql[1] @@ -125,7 +131,8 @@ def test_hooks_parsed_from_config_yaml(self, tmp_path: Path, monkeypatch): from ch_migrate.config import get_env_config config_file = tmp_path / "config.yaml" - config_file.write_text(""" + config_file.write_text( + """ environments: dev: host: localhost @@ -135,7 +142,8 @@ def test_hooks_parsed_from_config_yaml(self, tmp_path: Path, monkeypatch): hooks: post_migrate: - "SYSTEM RELOAD DICTIONARY {db}.dict_regions ON CLUSTER default" -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") env_config = get_env_config("dev", config_file) @@ -145,20 +153,25 @@ def test_hooks_parsed_from_config_yaml(self, tmp_path: Path, monkeypatch): registry = HookRegistry.from_config(env_config.get("hooks")) assert registry.has_hooks - assert registry.post_migrate[0] == "SYSTEM RELOAD DICTIONARY {db}.dict_regions ON CLUSTER default" + assert ( + registry.post_migrate[0] + == "SYSTEM RELOAD DICTIONARY {db}.dict_regions ON CLUSTER default" + ) def test_no_hooks_section_works(self, tmp_path: Path, monkeypatch): """Existing configs without hooks section continue to work.""" from ch_migrate.config import get_env_config config_file = tmp_path / "config.yaml" - config_file.write_text(""" + config_file.write_text( + """ environments: dev: host: localhost database: testdb user: test -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") env_config = get_env_config("dev", config_file) @@ -174,14 +187,18 @@ class TestHookExecutionOrder: def test_pre_hooks_fire_before_post_hooks(self): """Pre-migrate hooks should fire before post-migrate hooks.""" connection = MagicMock() - registry = HookRegistry.from_config({ - "pre_migrate": ["SELECT 'pre'"], - "post_migrate": ["SELECT 'post'"], - }) + registry = HookRegistry.from_config( + { + "pre_migrate": ["SELECT 'pre'"], + "post_migrate": ["SELECT 'post'"], + } + ) # Simulate the env.py execution order: pre-hooks, then post-hooks run_hooks(connection, registry.pre_migrate, db="mydb", phase="pre_migrate", revision="all") - run_hooks(connection, registry.post_migrate, db="mydb", phase="post_migrate", revision="abc123") + run_hooks( + connection, registry.post_migrate, db="mydb", phase="post_migrate", revision="abc123" + ) assert connection.execute.call_count == 2 calls = [str(c.args[0]) for c in connection.execute.call_args_list] @@ -224,6 +241,7 @@ def test_upgrade_env_creates_new_env_py(self, tmp_path: Path): # command is registered and validates missing migrations dir # Test with no migrations dir in cwd assert result.exit_code != 0 or "Updated" in result.output or "not found" in result.output + def test_upgrade_env_backs_up_existing(self, tmp_path: Path, monkeypatch): """upgrade-env creates a .bak backup of existing env.py.""" from click.testing import CliRunner diff --git a/tests/test_lint.py b/tests/test_lint.py index bafdc68..5797e3a 100644 --- a/tests/test_lint.py +++ b/tests/test_lint.py @@ -42,25 +42,29 @@ def test_from_config_empty(self): assert config.large_table_threshold == 100_000_000 def test_from_config_with_values(self): - config = LintConfig.from_config({ - "lint": { - "large_table_threshold": 50_000, - "rules": { - "destructive_changes": "error", - "missing_on_cluster": "off", - }, + config = LintConfig.from_config( + { + "lint": { + "large_table_threshold": 50_000, + "rules": { + "destructive_changes": "error", + "missing_on_cluster": "off", + }, + } } - }) + ) assert config.large_table_threshold == 50_000 assert config.rules["destructive_changes"] == Severity.ERROR assert config.rules["missing_on_cluster"] == Severity.OFF def test_from_config_invalid_severity_ignored(self): - config = LintConfig.from_config({ - "lint": { - "rules": {"destructive_changes": "invalid_value"}, + config = LintConfig.from_config( + { + "lint": { + "rules": {"destructive_changes": "invalid_value"}, + } } - }) + ) assert "destructive_changes" not in config.rules @@ -77,10 +81,12 @@ def test_empty_report(self): assert report.warning_count == 0 def test_report_with_errors(self): - report = LintReport(results=[ - LintResult(rule="test", message="bad", severity=Severity.ERROR), - LintResult(rule="test", message="meh", severity=Severity.WARN), - ]) + report = LintReport( + results=[ + LintResult(rule="test", message="bad", severity=Severity.ERROR), + LintResult(rule="test", message="meh", severity=Severity.WARN), + ] + ) assert report.has_errors assert report.error_count == 1 assert report.warning_count == 1 @@ -111,10 +117,12 @@ def test_no_flags_on_safe_sql(self): assert results == [] def test_multiple_drops(self): - sql = textwrap.dedent("""\ + sql = textwrap.dedent( + """\ DROP TABLE mydb.old_events; ALTER TABLE mydb.users DROP COLUMN phone; - """) + """ + ) results = DestructiveChangeRule().check(sql) assert len(results) == 2 @@ -191,25 +199,29 @@ def test_passes_create_mv_if_not_exists(self): class TestReservedWordRule: def test_flags_reserved_column_name(self): - sql = textwrap.dedent("""\ + sql = textwrap.dedent( + """\ CREATE TABLE mydb.t ( `id` UInt64, `key` String, `select` String ) - """) + """ + ) results = ReservedWordRule().check(sql) reserved_names = {r.message.split("'")[1] for r in results} assert "key" in reserved_names assert "select" in reserved_names def test_passes_non_reserved_names(self): - sql = textwrap.dedent("""\ + sql = textwrap.dedent( + """\ CREATE TABLE mydb.t ( `user_id` UInt64, `event_name` String ) - """) + """ + ) results = ReservedWordRule().check(sql) assert results == [] @@ -251,10 +263,12 @@ def test_passes_with_placeholder(self): def test_flags_alter_and_drop(self): config = LintConfig(rules={"missing_on_cluster": Severity.WARN}) - sql = textwrap.dedent("""\ + sql = textwrap.dedent( + """\ ALTER TABLE mydb.t ADD COLUMN foo String; DROP TABLE mydb.t; - """) + """ + ) results = MissingOnClusterRule().check(sql, config=config) assert len(results) == 2 @@ -275,27 +289,21 @@ def _make_client(self, row_count: int) -> MagicMock: def test_flags_large_table(self): client = self._make_client(200_000_000) sql = "ALTER TABLE mydb.users ADD COLUMN phone String" - results = LargeTableMutationRule().check( - sql, client=client, database="mydb" - ) + results = LargeTableMutationRule().check(sql, client=client, database="mydb") assert len(results) == 1 assert "200,000,000" in results[0].message def test_passes_small_table(self): client = self._make_client(1000) sql = "ALTER TABLE mydb.users ADD COLUMN phone String" - results = LargeTableMutationRule().check( - sql, client=client, database="mydb" - ) + results = LargeTableMutationRule().check(sql, client=client, database="mydb") assert results == [] def test_respects_custom_threshold(self): client = self._make_client(5000) config = LintConfig(large_table_threshold=1000) sql = "ALTER TABLE mydb.users ADD COLUMN phone String" - results = LargeTableMutationRule().check( - sql, client=client, database="mydb", config=config - ) + results = LargeTableMutationRule().check(sql, client=client, database="mydb", config=config) assert len(results) == 1 def test_skips_without_client(self): @@ -339,9 +347,7 @@ def test_flags_drop_on_mv_source(self): sql = "DROP TABLE IF EXISTS events" with patch("ch_migrate.introspect.get_dependencies", return_value=graph): - results = MVDependencyRule().check( - sql, client=client, database="mydb" - ) + results = MVDependencyRule().check(sql, client=client, database="mydb") assert len(results) == 1 assert "hourly_mv" in results[0].message @@ -353,9 +359,7 @@ def test_passes_on_unrelated_table(self): sql = "DROP TABLE IF EXISTS unrelated_table" with patch("ch_migrate.introspect.get_dependencies", return_value=graph): - results = MVDependencyRule().check( - sql, client=client, database="mydb" - ) + results = MVDependencyRule().check(sql, client=client, database="mydb") assert results == [] @@ -400,7 +404,8 @@ def _create_migration(self, tmp_path: Path, name: str, sql_content: str) -> Path versions_dir.mkdir(exist_ok=True) rev_id = name[:12].ljust(12, "0") - content = textwrap.dedent(f"""\ + content = textwrap.dedent( + f"""\ \"\"\"Migration {name} Revision ID: {rev_id} @@ -418,16 +423,15 @@ def upgrade(): def downgrade(): pass - """) + """ + ) file_path = versions_dir / f"{rev_id}_{name}.py" file_path.write_text(content) return versions_dir def test_static_lint_finds_issues(self, tmp_path: Path): - versions_dir = self._create_migration( - tmp_path, "drop_users", "DROP TABLE mydb.users" - ) + versions_dir = self._create_migration(tmp_path, "drop_users", "DROP TABLE mydb.users") report = lint_migrations(versions_dir) assert report.warning_count > 0 @@ -442,9 +446,7 @@ def test_static_lint_clean(self, tmp_path: Path): assert report.warning_count == 0 def test_lint_with_error_severity(self, tmp_path: Path): - versions_dir = self._create_migration( - tmp_path, "drop_bad", "DROP TABLE mydb.users" - ) + versions_dir = self._create_migration(tmp_path, "drop_bad", "DROP TABLE mydb.users") config = LintConfig(rules={"destructive_changes": Severity.ERROR}) report = lint_migrations(versions_dir, config=config) assert report.has_errors diff --git a/tests/test_mv_validate.py b/tests/test_mv_validate.py index 9916925..ba97eba 100644 --- a/tests/test_mv_validate.py +++ b/tests/test_mv_validate.py @@ -85,23 +85,27 @@ def _write_migration( parts.append("") parts.append(extra_python) - parts.extend([ - "", - "def upgrade() -> None:", - " db = get_db()", - ]) + parts.extend( + [ + "", + "def upgrade() -> None:", + " db = get_db()", + ] + ) if sql: parts.append(f' op.execute(f"""{sql}""")') else: parts.append(" pass") - parts.extend([ - "", - "def downgrade() -> None:", - " pass", - "", - ]) + parts.extend( + [ + "", + "def downgrade() -> None:", + " pass", + "", + ] + ) content = "\n".join(parts) file_path = versions_dir / f"{rev_id}_{name}.py" @@ -116,9 +120,7 @@ def _write_migration( class TestCreateMVDetection: def test_detects_create_mv(self): - assert _sql_has_create_mv( - "CREATE MATERIALIZED VIEW mydb.mv TO mydb.dest AS SELECT 1" - ) + assert _sql_has_create_mv("CREATE MATERIALIZED VIEW mydb.mv TO mydb.dest AS SELECT 1") def test_detects_create_mv_if_not_exists(self): assert _sql_has_create_mv( @@ -126,9 +128,7 @@ def test_detects_create_mv_if_not_exists(self): ) def test_case_insensitive(self): - assert _sql_has_create_mv( - "create materialized view mydb.mv TO mydb.dest AS SELECT 1" - ) + assert _sql_has_create_mv("create materialized view mydb.mv TO mydb.dest AS SELECT 1") def test_ignores_regular_view(self): assert not _sql_has_create_mv("CREATE VIEW mydb.v AS SELECT 1") @@ -142,9 +142,7 @@ def test_ignores_empty(self): assert not _sql_has_create_mv("") def test_ignores_drop_mv(self): - assert not _sql_has_create_mv( - "DROP MATERIALIZED VIEW IF EXISTS mydb.mv" - ) + assert not _sql_has_create_mv("DROP MATERIALIZED VIEW IF EXISTS mydb.mv") class TestGrantInsertDetection: @@ -153,16 +151,16 @@ def test_finds_basic_grant(self): assert ("my_table", "my_user") in grants def test_finds_db_placeholder(self): - grants = _find_grant_inserts( - "GRANT INSERT ON {db}.my_table TO my_user" - ) + grants = _find_grant_inserts("GRANT INSERT ON {db}.my_table TO my_user") assert ("my_table", "my_user") in grants def test_finds_multiple(self): - sql = textwrap.dedent("""\ + sql = textwrap.dedent( + """\ GRANT INSERT ON {db}.table_a TO user_a; GRANT INSERT ON {db}.table_b TO user_b; - """) + """ + ) grants = _find_grant_inserts(sql) assert ("table_a", "user_a") in grants assert ("table_b", "user_b") in grants @@ -172,9 +170,7 @@ def test_case_insensitive(self): assert ("mytable", "myuser") in grants def test_comma_separated_users(self): - grants = _find_grant_inserts( - "GRANT INSERT ON {db}.my_table TO user_a, user_b;" - ) + grants = _find_grant_inserts("GRANT INSERT ON {db}.my_table TO user_a, user_b;") assert ("my_table", "user_a") in grants assert ("my_table", "user_b") in grants @@ -189,17 +185,13 @@ def test_finds_basic_grant(self): assert ("my_table", "reader") in grants def test_finds_column_grant(self): - grants = _find_grant_selects( - "GRANT SELECT(col1, col2) ON {db}.my_table TO reader" - ) + grants = _find_grant_selects("GRANT SELECT(col1, col2) ON {db}.my_table TO reader") assert ("my_table", "reader") in grants class TestRowPolicyDetection: def test_finds_basic_policy(self): - policies = _find_row_policies( - "CREATE ROW POLICY my_deny ON {db}.my_table USING 0 TO ALL" - ) + policies = _find_row_policies("CREATE ROW POLICY my_deny ON {db}.my_table USING 0 TO ALL") assert ("my_deny", "my_table") in policies def test_finds_if_not_exists(self): @@ -837,7 +829,8 @@ def test_mv_in_sql_file_detected(self, tmp_path: Path): versions.mkdir(parents=True, exist_ok=True) rev_id = "abc123456789" - content = textwrap.dedent(f"""\ + content = textwrap.dedent( + f"""\ \"\"\"create my_mv Revision ID: {rev_id} @@ -859,7 +852,8 @@ def upgrade() -> None: def downgrade() -> None: pass - """) + """ + ) (versions / f"{rev_id}_create_my_mv.py").write_text(content) errors = validate_mv_migrations(versions) @@ -875,13 +869,17 @@ def test_up_does_not_block_on_mv_warnings(self, tmp_path: Path, monkeypatch): from ch_migrate.cli import main # Create minimal project structure - (tmp_path / "config.yaml").write_text(textwrap.dedent("""\ + (tmp_path / "config.yaml").write_text( + textwrap.dedent( + """\ environments: dev: host: localhost database: testdb migration_user: test - """)) + """ + ) + ) (tmp_path / ".env.local").write_text("CH_DEV_MIGRATION_PASSWORD=pass\n") versions = tmp_path / "migrations" / "versions" @@ -909,13 +907,17 @@ def test_up_skip_mv_check_bypasses(self, tmp_path: Path, monkeypatch): from click.testing import CliRunner from ch_migrate.cli import main - (tmp_path / "config.yaml").write_text(textwrap.dedent("""\ + (tmp_path / "config.yaml").write_text( + textwrap.dedent( + """\ environments: dev: host: localhost database: testdb migration_user: test - """)) + """ + ) + ) (tmp_path / ".env.local").write_text("CH_DEV_MIGRATION_PASSWORD=pass\n") versions = tmp_path / "migrations" / "versions" @@ -928,9 +930,7 @@ def test_up_skip_mv_check_bypasses(self, tmp_path: Path, monkeypatch): monkeypatch.chdir(tmp_path) runner = CliRunner() # With --skip-mv-check, validation is bypassed before the env-version guard. - result = runner.invoke( - main, ["up", "dev", "--skip-mv-check"], catch_exceptions=False - ) + result = runner.invoke(main, ["up", "dev", "--skip-mv-check"], catch_exceptions=False) # The validation finding must not be reported output = result.output assert "create_mv000_create_mv.py:" not in output diff --git a/tests/test_reruns.py b/tests/test_reruns.py new file mode 100644 index 0000000..e599aed --- /dev/null +++ b/tests/test_reruns.py @@ -0,0 +1,123 @@ +"""Rerunning an interrupted step: which steps ch-migrate vouches for, and the marker table.""" + +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from ch_migrate.waiting.reruns import InterruptedStep, RerunGuard, Step, unvouched_statements + + +def test_a_first_run_records_the_step_before_it_runs(tmp_path): + client = _Client(started=0) + _guard(client).begin(_step(tmp_path, "op.execute('SELECT 1')")) + assert any(sql.startswith("INSERT INTO `db`.`_ch_migrate_in_progress`") for sql in client.sent) + + +def test_an_interrupted_step_the_gate_passes_runs_again(tmp_path, caplog): + client = _Client(started=1) + step = _step(tmp_path, "op.execute('ALTER TABLE t ADD COLUMN IF NOT EXISTS c UInt8')") + _guard(client).begin(step) + assert "was interrupted on an earlier run; running it again" in caplog.text + assert any(sql.startswith("INSERT INTO") for sql in client.sent) + + +def test_an_interrupted_step_with_a_waiver_stops(tmp_path): + client = _Client(started=1) + step = _step( + tmp_path, + "# ch-migrate: allow-non-idempotent counted once per deploy\n" + " op.execute('ALTER TABLE t UPDATE n = n + 1 WHERE 1')", + ) + with pytest.raises(InterruptedStep) as stopped: + _guard(client).begin(step) + assert [item.why for item in stopped.value.unvouched] == ["waiver: counted once per deploy"] + assert "--rerun-interrupted bbbbbbbb" in str(stopped.value) + assert not any(sql.startswith("INSERT INTO") for sql in client.sent) + + +def test_naming_the_revision_lets_an_interrupted_step_run_again(tmp_path): + client = _Client(started=1) + step = _step(tmp_path, "op.execute(statement)") + _guard(client, acknowledged={"bbbbbbbb"}).begin(step) + assert any(sql.startswith("INSERT INTO") for sql in client.sent) + + +def test_a_recorded_step_clears_every_marker_for_its_revision(tmp_path): + client = _Client(started=0) + _guard(client).finish(_step(tmp_path, "op.execute('SELECT 1')")) + assert client.sent == [ + "ALTER TABLE `db`.`_ch_migrate_in_progress` DELETE WHERE revision = " + "'bbbbbbbbbbbb' SETTINGS mutations_sync = 2" + ] + + +def test_downgrades_are_judged_by_the_same_rules(tmp_path): + path = _revision( + tmp_path, + "op.execute('CREATE TABLE IF NOT EXISTS t (x UInt8) ENGINE = Memory')", + downgrade="op.execute('INSERT INTO t SELECT 1')", + ) + assert unvouched_statements(path, "upgrade") == [] + assert [item.why for item in unvouched_statements(path, "downgrade")] == [ + "not safe to run twice" + ] + + +@pytest.mark.parametrize( + ("engine", "cluster", "expected"), + [ + ("Atomic", None, "ENGINE = MergeTree ORDER BY"), + ("Shared", None, "ENGINE = MergeTree ORDER BY"), + ("Replicated", None, "ENGINE = ReplicatedMergeTree ORDER BY"), + ( + "Atomic", + "main", + "ON CLUSTER `main` (revision String, direction LowCardinality(String), started " + "DateTime64(3) DEFAULT now64(3)) ENGINE = ReplicatedMergeTree(" + "'/clickhouse/ch_migrate/db/{shard}/_ch_migrate_in_progress', '{replica}')", + ), + ], +) +def test_the_marker_table_follows_the_deployment(engine, cluster, expected): + state = _state(engine, cluster) + assert expected in RerunGuard(_Client(started=0), state, frozenset())._create_sql() + + +class _Client: + def __init__(self, started: int): + self.started, self.sent = started, [] + + def command(self, sql, parameters=None, settings=None): + if sql.startswith("EXISTS TABLE"): + return 1 + if sql.startswith("SELECT count()"): + return self.started + self.sent.append(sql) + return None + + +def _guard(client, acknowledged=()): + return RerunGuard(client, _state("Atomic", None), frozenset(acknowledged)) + + +def _state(engine, cluster): + on_cluster = cluster if engine == "Atomic" else None + return SimpleNamespace( + database="db", database_engine=engine, cluster=cluster, on_cluster=on_cluster + ) + + +def _step(tmp_path: Path, upgrade: str) -> Step: + return Step("bbbbbbbbbbbb", "upgrade", _revision(tmp_path, upgrade)) + + +def _revision(tmp_path: Path, upgrade: str, downgrade: str = "pass") -> Path: + versions = tmp_path / "migrations" / "versions" + versions.mkdir(parents=True, exist_ok=True) + path = versions / "bbbb.py" + path.write_text( + "revision = 'bbbbbbbbbbbb'\ndown_revision = None\n\n" + f"def upgrade():\n {upgrade}\n\ndef downgrade():\n {downgrade}\n" + ) + return path diff --git a/tests/test_snapshot.py b/tests/test_snapshot.py index eee1d44..8665254 100644 --- a/tests/test_snapshot.py +++ b/tests/test_snapshot.py @@ -58,13 +58,15 @@ def runner(tmp_path: Path, monkeypatch): """Set up a CLI runner in a tmp project directory.""" monkeypatch.chdir(tmp_path) (tmp_path / "migrations" / "sql" / "snapshots").mkdir(parents=True) - (tmp_path / "config.yaml").write_text(""" + (tmp_path / "config.yaml").write_text( + """ environments: dev: host: localhost database: testdb user: test -""") +""" + ) monkeypatch.setenv("CH_DEV_PASSWORD", "pass") return CliRunner() @@ -179,6 +181,7 @@ def test_timestamped_dirs_no_overwrite(self, mock_schema, mock_client, runner, t # Ensure a second snapshot creates a new directory (different timestamp) import time + time.sleep(1.1) result2 = runner.invoke(main, ["snapshot", "dev"]) diff --git a/tests/test_statements.py b/tests/test_statements.py index fe403bc..fffabdf 100644 --- a/tests/test_statements.py +++ b/tests/test_statements.py @@ -127,6 +127,59 @@ def test_missing_referenced_file_is_not_silently_ignored(project): migration_statements(path) +def test_every_way_of_sending_sql_is_collected(project): + path = _revision( + project, + "def upgrade():\n" + " op.execute(text('UPDATE {db}.t SET x = 1 WHERE 1'))\n" + " op.get_bind().execute(sa.text(f'DELETE FROM {db}.t WHERE id = 1'))\n" + " op.get_bind().exec_driver_sql('ALTER TABLE {db}.t UPDATE x = 2 WHERE 1')\n", + ) + statements = migration_statements(path) + assert [(s.sql, s.line, s.readable) for s in statements] == [ + ("UPDATE {db}.t SET x = 1 WHERE 1", 4, True), + ("DELETE FROM {db}.t WHERE id = 1", 5, True), + ("ALTER TABLE {db}.t UPDATE x = 2 WHERE 1", 6, True), + ] + + +def test_sql_known_only_at_run_time_is_unreadable_not_skipped(project): + path = _revision( + project, + "def upgrade():\n" + " # ch-migrate: allow-non-idempotent built from a reviewed list\n" + " op.execute(statement)\n" + " run_sql(name)\n" + " connection.exec_driver_sql(build())\n", + ) + statements = migration_statements(path) + assert [(s.sql, s.line, s.readable) for s in statements] == [ + ("op.execute(statement)", 5, False), + ("run_sql(name)", 6, False), + ("connection.exec_driver_sql(build())", 7, False), + ] + assert statements[0].comments == ( + "ch-migrate: allow-non-idempotent built from a reviewed list", + ) + + +def test_unreadable_sql_needs_a_waiver_and_skips_sql_rules(project): + _revision( + project, + "def upgrade():\n" + " op.execute(statement)\n" + " # ch-migrate: allow-non-idempotent each table is filled once\n" + " op.execute(other)\n", + ) + report = lint_migrations(project / "migrations/versions") + assert [(r.rule, r.severity.value, r.line) for r in report.results] == [ + ("idempotency", "error", 4), + ("idempotency", "info", 6), + ] + assert "only exists at run time" in report.results[0].message + assert report.results[1].message == "Idempotency waiver: each table is filled once" + + def _revision(project: Path, body: str) -> Path: path = project / "migrations/versions/a.py" path.write_text("revision = 'a'\ndown_revision = None\n" + body) diff --git a/tests/test_status_command.py b/tests/test_status_command.py index c53e80e..afe3bda 100644 --- a/tests/test_status_command.py +++ b/tests/test_status_command.py @@ -1,7 +1,7 @@ """`ch-migrate status` is a report: CI runs it as a non-blocking check. -Sazabi's required "ClickHouse Migration Status" job relies on status exiting 0 when -the database is unreachable or migrations are pending; only a broken project fails. +A required CI status job can rely on status exiting 0 when the database is +unreachable or migrations are pending; only a broken project fails. """ import pytest diff --git a/tests/test_waiting.py b/tests/test_waiting.py new file mode 100644 index 0000000..6cab874 --- /dev/null +++ b/tests/test_waiting.py @@ -0,0 +1,77 @@ +"""Waiting decisions without a server: the deadline, what counts as a write, and completion.""" + +from types import SimpleNamespace + +import pytest + +from ch_migrate.waiting.budget import WaitBudget +from ch_migrate.waiting.display import WaitDisplay +from ch_migrate.waiting.errors import MutationFailed, WaitTimeout +from ch_migrate.waiting.mutations import MutationWaiter +from ch_migrate.waiting.sql import is_write + + +def test_deadline_is_shared_across_waits(monkeypatch): + now = [10.0] + monkeypatch.setattr("ch_migrate.waiting.budget.time.monotonic", lambda: now[0]) + budget = WaitBudget(timeout=2, started=now[0]) + now[0] = 11.5 + assert budget.elapsed == 1.5 + now[0] = 12.0 + with pytest.raises(WaitTimeout, match="second mutation"): + budget.check("second mutation") + + +@pytest.mark.parametrize( + ("sql", "write"), + [ + ("SELECT 1", False), + (" -- note\n select count() FROM t", False), + ("/* why */ SET max_threads = 1", False), + ("SHOW TABLES", False), + ("EXISTS TABLE t", False), + ("ALTER TABLE t UPDATE x = 1 WHERE 1", True), + ("INSERT INTO t SELECT 1", True), + ("WITH source AS (SELECT 1) INSERT INTO t SELECT * FROM source", True), + ("CREATE TABLE t (x UInt8) ENGINE = Memory", True), + ("", True), + ], +) +def test_only_reads_and_session_statements_skip_the_wait(sql, write): + assert is_write(sql) is write + + +def test_zero_parts_left_is_not_completion(): + waiter = _waiter([_row("h1", "t", "mutation_1.txt", done=0, parts=0)]) + with pytest.raises(WaitTimeout, match="mutation_1.txt host h1; parts_to_do=0"): + waiter.wait() + + +def test_a_replica_without_the_mutation_is_still_pending(): + waiter = _waiter([_row("r1", "t", "0000000001", done=1)], hosts={"t": ("r1", "r2")}) + with pytest.raises(WaitTimeout, match="0000000001 host r2; not received yet"): + waiter.wait() + + +def test_a_failed_mutation_stops_with_the_scoped_kill(): + waiter = _waiter([_row("h1", "t", "mutation_2.txt", reason="Cannot parse 'n/a'")]) + with pytest.raises(MutationFailed) as raised: + waiter.wait(before_run=True) + failure = raised.value.failure + assert failure.foreign and failure.reason == "Cannot parse 'n/a'" + assert failure.kill.endswith("AND table = 't' AND mutation_id = 'mutation_2.txt'") + + +def _waiter(rows, hosts=None): + state = SimpleNamespace(database="db", health_cluster=None) + budget = WaitBudget(timeout=0, display=WaitDisplay()) + waiter = MutationWaiter(None, state, budget) + # The real wait loop runs against these stubs: the rows the server returns, and which + # hosts each table's mutations must finish on. + waiter._rows = lambda expected: [waiter._row(row) for row in rows] + waiter._resolve_hosts = lambda table: (hosts or {}).get(table) + return waiter + + +def _row(host, table, mutation, done=0, parts=1, reason=""): + return (host, table, mutation, done, parts, reason, []) diff --git a/tests/test_waiting_display.py b/tests/test_waiting_display.py new file mode 100644 index 0000000..0275674 --- /dev/null +++ b/tests/test_waiting_display.py @@ -0,0 +1,174 @@ +"""What `up` shows while it waits: the terminal bar, piped lines and failure reports.""" + +import io + +import pytest +from rich.console import Console + +from ch_migrate.migration_runner import Upgrade, _report_failure, _report_interrupt +from ch_migrate.runner import _Progress +from ch_migrate.sql import SqlStatementError +from ch_migrate.statements import MigrationStatement +from ch_migrate.waiting.display import HostParts, MutationProgress, WaitDisplay +from ch_migrate.waiting.errors import FailedMutation, MutationFailed, WaitTimeout +from ch_migrate.waiting.reruns import InterruptedStep, Step, Unvouched + +GIB = 1 << 30 + + +@pytest.fixture +def clock(monkeypatch): + now = [100.0] + monkeypatch.setattr("ch_migrate.waiting.display.time.monotonic", lambda: now[0]) + return now + + +def test_terminal_bar_weights_by_bytes_and_estimates_time(clock): + display, buffer = _display(terminal=True) + display.start_migration("3f9c2a1b backfill_status") + display.update(_progress(_mutation(41))) + clock[0] = 172.0 + display.update(_progress(_mutation(18))) + assert "23.0 / 41.0 GiB · 23/41 parts · 1:12 · ~0:56 left" in buffer.getvalue() + assert "━" in buffer.getvalue() + clock[0] = 223.0 + display.complete("demo_dev.events") + assert ( + buffer.getvalue() + .rstrip() + .endswith("✓ 3f9c2a1b backfill_status mutation 7 · 41 parts · 41.0 GiB · 2:03") + ) + + +def test_piped_lines_start_then_every_tenth_or_thirty_seconds(clock): + display, buffer = _display(terminal=False) + display.start_migration("3f9c2a1b backfill_status") + for at, left in ((100, 40), (101, 38), (102, 36), (140, 35)): + clock[0] = float(at) + display.update(_progress(_mutation(left, sizes=False))) + clock[0] = 141.0 + display.complete("demo_dev.events") + assert buffer.getvalue().splitlines() == [ + " waiting on mutation 7 (demo_dev.events): 0/40 parts", + " waiting on mutation 7 (demo_dev.events): 4/40 parts · 0:02", + " waiting on mutation 7 (demo_dev.events): 5/40 parts · 0:40", + "✓ 3f9c2a1b backfill_status mutation 7 · 40 parts · 0:41", + ] + assert "\x1b" not in buffer.getvalue() and "\r" not in buffer.getvalue() + + +def test_unreadable_part_sizes_fall_back_to_parts_once(): + display, buffer = _display(terminal=True) + assert display.wants_bytes + display.parts_only("can't read system.parts (ACCESS_DENIED)") + display.parts_only("can't read system.parts (ACCESS_DENIED)") + assert buffer.getvalue().count("! Progress counts parts only") == 1 + assert not display.wants_bytes + + +def test_parts_only_bar_has_no_time_estimate(clock): + display, buffer = _display(terminal=True) + display.update(_progress(_mutation(10, sizes=False))) + clock[0] = 110.0 + display.update(_progress(_mutation(4, sizes=False))) + assert "6/10 parts" in buffer.getvalue() and "left" not in buffer.getvalue() + + +def test_each_replica_gets_its_own_row(clock): + display, buffer = _display(terminal=True) + display.update(_progress(_mutation(5, "ch-1"), _mutation(5, "ch-2"))) + clock[0] = 105.0 + display.update(_progress(_mutation(2, "ch-2"))) + text = buffer.getvalue() + assert "ch-1" in text and "ch-2" in text + display.complete("demo_dev.events") + assert "· 2 replicas" in buffer.getvalue().splitlines()[-1] + + +def test_failed_mutation_names_reason_retry_and_kill(capsys): + failure = FailedMutation( + "demo_dev.events", + "mutation_7.txt", + "h1", + "Cannot parse string 'n/a' as UInt32", + "KILL MUTATION WHERE database = 'demo_dev' AND table = 'events' AND mutation_id = 'mutation_7.txt'", + ) + error = _statement_error(MutationFailed("failed", failure)) + _report_failure(Upgrade("dev"), _current("3f9c2a1b widen_amount"), error) + err = capsys.readouterr().err + assert err.startswith( + "✗ 3f9c2a1b widen_amount (migrations/sql/x.up.sql:2) failed: mutation mutation_7.txt " + "on demo_dev.events failed on h1" + ) + assert "Cannot parse string 'n/a' as UInt32" in err + assert "keeps retrying a failed mutation" in err and failure.kill in err + assert ( + "If the cause is temporary" in err + and "then fixes the SQL and runs ch-migrate up dev" in err + ) + + +def test_timeout_says_the_work_continues_and_how_to_resume(capsys): + timeout = WaitTimeout( + "Timed out", "demo_dev.events mutation mutation_7.txt; parts_to_do=3", 3.0 + ) + _report_failure(Upgrade("dev"), _current("3f9c2a1b backfill"), _statement_error(timeout)) + err = capsys.readouterr().err + assert err.startswith("✗ Timed out after 3.0s waiting on 3f9c2a1b backfill") + assert "Pending: demo_dev.events mutation mutation_7.txt; parts_to_do=3" in err + assert "ch-migrate up dev again: it waits for that work to finish" in err + + +def test_interrupted_step_names_what_it_cannot_vouch_for(capsys): + statement = MigrationStatement( + "ALTER TABLE {db}.events UPDATE n = n + 1 WHERE 1", + "migrations/sql/bump.up.sql", + 2, + (), + "upgrade", + ) + stop = InterruptedStep( + Step("3f9c2a1b5d6e", "upgrade", None), [Unvouched(statement, "waiver: counted once")] + ) + _report_failure(Upgrade("dev"), _current("3f9c2a1b bump"), stop) + err = capsys.readouterr().err + assert err.startswith("✗ 3f9c2a1b was interrupted on an earlier run") + assert "migrations/sql/bump.up.sql:2 waiver: counted once: ALTER TABLE {db}.events" in err + assert "ch-migrate up dev --rerun-interrupted 3f9c2a1b" in err + + +def test_interrupt_says_the_work_continues(capsys): + _report_interrupt("dev", _current("3f9c2a1b backfill")) + err = capsys.readouterr().err + assert err.startswith("✗ Interrupted during 3f9c2a1b backfill") + assert "Run ch-migrate up dev again: it waits for that work to finish" in err + + +def _display(terminal): + buffer = io.StringIO() + console = Console(file=buffer, force_terminal=terminal, width=160, color_system=None) + return WaitDisplay(console), buffer + + +def _mutation(left, host="h1", sizes=True): + names = frozenset(f"all_{i}_{i}_0" for i in range(left)) + return HostParts(host, names, left, {name: GIB for name in names} if sizes else None) + + +def _progress(*hosts): + return MutationProgress("demo_dev.events", "7", hosts) + + +def _current(label): + progress = _Progress() + progress.current, progress.count = label, 1 + return progress + + +def _statement_error(cause): + try: + raise SqlStatementError( + f"migrations/sql/x.up.sql (statement 1 of 1, line 2): {cause}" + ) from cause + except SqlStatementError as error: + return error