diff --git a/.ci/scripts/schema_diff.py b/.ci/scripts/schema_diff.py new file mode 100755 index 0000000000..9b7af72434 --- /dev/null +++ b/.ci/scripts/schema_diff.py @@ -0,0 +1,227 @@ +#!/usr/bin/env python3 +# +# Get a diff showing the change in the database schema. +# +# Usage: +# # Compare against develop (default): +# PGUSER=postgres PGPASSWORD=postgres scripts-dev/schema_diff.py +# +# # Compare against a specific branch/commit: +# PGUSER=postgres PGPASSWORD=postgres scripts-dev/schema_diff.py --base origin/release-v1.100 + +import argparse +import os +import subprocess +import sys +import tempfile +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent.parent +SCHEMA_DIR = "synapse/storage/schema" +MAKE_FULL_SCHEMA_SCRIPT = REPO_ROOT / "scripts-dev" / "make_full_schema.sh" + + +def run_make_full_schema(output_dir: Path) -> None: + """Run make_full_schema.sh, piping the password via stdin.""" + pg_user = os.environ.get("PGUSER", "") + pg_password = os.environ.get("PGPASSWORD", "") + if not pg_user: + print("ERROR: PGUSER environment variable not set.", file=sys.stderr) + sys.exit(1) + if not pg_password: + print("ERROR: PGPASSWORD environment variable not set.", file=sys.stderr) + sys.exit(1) + + cmd: list[str] = [ + "poetry", + "run", + str(MAKE_FULL_SCHEMA_SCRIPT), + "-p", + pg_user, + "-o", + str(output_dir), + "-c", + "-n", + "9999", + ] + + print(f"Running: {' '.join(cmd)}", file=sys.stderr) + + proc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.STDOUT, + cwd=REPO_ROOT, + text=True, + ) + stdout, _ = proc.communicate(input=pg_password + "\n") + # Forward script output to stderr so stdout stays clean for markdown + if stdout: + print(stdout, file=sys.stderr, end="") + if proc.returncode != 0: + print( + f"ERROR: make_full_schema.sh failed with exit code {proc.returncode}", + file=sys.stderr, + ) + sys.exit(proc.returncode) + + +def diff_schemas( + before_dir: Path, after_dir: Path, before_ref: str, after_ref: str +) -> str: + """Diff SQLite and Postgres full schemas, return a Markdown report.""" + parts: list[str] = [ + "## Schema Diff", + "", + "Please check that this looks as expected!", + "", + ] + + for db in ["common", "main", "state"]: + for engine in ["sqlite", "postgres"]: + filename = f"full.sql.{engine}" + + before_file = before_dir / db / "full_schemas" / "9999" / filename + + after_file = after_dir / db / "full_schemas" / "9999" / filename + + if not before_file.exists(): + raise RuntimeError(f"No before file found for {db = }, {engine = }") + if not after_file.exists(): + raise RuntimeError(f"No after file found for {db = }, {engine = }") + + result = subprocess.run( + ["diff", "-U", "10", str(before_file), str(after_file)], + capture_output=True, + text=True, + ) + + if result.returncode == 0: + parts.append(f"### {db} ({engine})\n\nUnchanged\n") + else: + parts.append(f"### {db} ({engine})\n\n```diff\n{result.stdout}\n```\n") + + return "\n".join(parts) + + +def main() -> None: + parser = argparse.ArgumentParser(description="Show database schema changes") + parser.add_argument( + "--base", + default="develop", + help="Base commit/branch to compare against (default: develop)", + ) + args = parser.parse_args() + + # Create temp output directory with before/after subdirectories + with tempfile.TemporaryDirectory(prefix="schema_diff_") as tmpdir: + after_dir = Path(tmpdir) / "after" + before_dir = Path(tmpdir) / "before" + after_dir.mkdir() + before_dir.mkdir() + + print("\n--- Running make_full_schema.sh (after) ---", file=sys.stderr) + run_make_full_schema(after_dir) + + # Checkout base and run make_full_schema.sh + print( + f"\n--- Checking out {args.base} and running make_full_schema.sh (before) ---", + file=sys.stderr, + ) + + # Save current ref so we can return to it without detaching. + # (Not useful in CI, but is useful for local development.) + # If we are on a named branch, use the branch name; otherwise use the SHA. + head_ref = subprocess.run( + ["git", "rev-parse", "--abbrev-ref", "HEAD"], + capture_output=True, + text=True, + cwd=REPO_ROOT, + check=True, + ).stdout.strip() + + before_sha = subprocess.run( + ["git", "rev-parse", args.base], + capture_output=True, + text=True, + cwd=REPO_ROOT, + check=True, + ).stdout.strip() + after_sha = subprocess.run( + ["git", "rev-parse", "HEAD"], + capture_output=True, + text=True, + cwd=REPO_ROOT, + check=True, + ).stdout.strip() + + # Check if working tree is dirty before potentially stashing + status = subprocess.run( + [ + "git", + "status", + # Machine-readable output for easy parsing + "--porcelain", + ], + capture_output=True, + text=True, + cwd=REPO_ROOT, + check=True, + ).stdout.strip() + + did_stash = False + if status: + print("Stashing local changes before checkout...", file=sys.stderr) + subprocess.run( + [ + "git", + "stash", + "push", + "--include-untracked", + "-m", + "schema_diff temporary stash", + ], + cwd=REPO_ROOT, + check=True, + ) + did_stash = True + + try: + subprocess.run(["git", "checkout", args.base], cwd=REPO_ROOT, check=True) + + # Refresh dependencies + print("Installing dependencies for base commit...", file=sys.stderr) + subprocess.run( + ["poetry", "install", "--no-root", "--extras", "postgres"], + cwd=REPO_ROOT, + check=True, + # Poetry install is noisy, so pipe its stdout to stderr + stdout=sys.stderr, + ) + + run_make_full_schema(before_dir) + finally: + print("Returning to HEAD...", file=sys.stderr) + subprocess.run( + [ + "git", + "checkout", + head_ref, + ], + cwd=REPO_ROOT, + check=True, + ) + if did_stash: + subprocess.run(["git", "stash", "pop"], cwd=REPO_ROOT, check=True) + print("✓ Restored stashed changes.", file=sys.stderr) + + # Diff + print("\n--- Diffing schemas ---", file=sys.stderr) + markdown = diff_schemas(before_dir, after_dir, before_sha, after_sha) + + print(markdown) + + +if __name__ == "__main__": + main() diff --git a/.github/workflows/schema_diff.yml b/.github/workflows/schema_diff.yml new file mode 100644 index 0000000000..0c32ef33f4 --- /dev/null +++ b/.github/workflows/schema_diff.yml @@ -0,0 +1,101 @@ +name: Schema Diff + +on: + pull_request: + paths: + - synapse/storage/schema/*/delta/** + - synapse/storage/schema/*/full_schemas/** + - .github/workflows/schema_diff.yml + +concurrency: + group: ${{ github.workflow }}-${{ github.ref }} + cancel-in-progress: true + +permissions: + contents: read + +jobs: + # Posts a GitHub PR comment that shows what the effective change to the schema is. + # Provides an excuse to run the `make_full_schema.sh` script in CI (so we keep it working) + # and can act as a review aid for schema changes, letting you easily see the diff of the + # end result, even when background updates or complex schema deltas are present. + show-schema-diff: + name: Show schema diff + runs-on: ubuntu-latest + + permissions: + pull-requests: write + + steps: + - name: Start postgres with a faked clock + background: true + id: postgres + run: | + # Build a docker image with faketime + mkdir /tmp/postgres-faketime + cat > /tmp/postgres-faketime/Dockerfile <<'EOF' + FROM postgres:14-alpine + RUN apk add --no-cache libfaketime + + # It seems like it could be harmful to fake the monotonic timer + # as it might prevent deadlock detection, etc. + # But not sure, just doing out of precaution. + ENV FAKETIME_DONT_FAKE_MONOTONIC=1 + ENTRYPOINT ["faketime", "-f", "2001-05-25 12:42:42", "docker-entrypoint.sh"] + CMD ["postgres"] + EOF + docker build -t localhost/postgres-faketime /tmp/postgres-faketime + + # Run it in the background + docker run -d --name postgres -p 5432:5432 \ + -e POSTGRES_PASSWORD=postgres \ + -e POSTGRES_INITDB_ARGS="--lc-collate C --lc-ctype C --encoding UTF8" \ + --health-cmd pg_isready --health-interval 10s \ + --health-timeout 5s --health-retries 5 \ + localhost/postgres-faketime + + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + fetch-depth: 0 + + - name: Install PostgreSQL client + run: sudo apt-get -qq install postgresql-client + + - uses: matrix-org/setup-python-poetry@5bbf6603c5c930615ec8a29f1b5d7d258d905aa4 # v2.0.0 + with: + poetry-version: "2.4.1" + extras: "postgres" + python-version: "3.x" + + - name: Wait for Postgres to be up + run: | + until [ "$(docker inspect -f '{{.State.Health.Status}}' postgres)" = healthy ]; do sleep 2; done + + - name: Generate schema diff + id: schema_diff + env: + PGHOST: localhost + PGUSER: postgres + PGPASSWORD: postgres + run: | + poetry run python .ci/scripts/schema_diff.py \ + --base origin/develop \ + > "${{ runner.temp }}/schema_diff.md" + + - name: Stop postgres + cancel: postgres + + # If the generation step failed, write an error message so the sticky + # comment step still has a file to read. + - name: Ensure output file exists on failure + if: always() && steps.schema_diff.outcome == 'failure' + run: | + echo "⚠️ Schema diff generation failed. See job logs for details." \ + > "${{ runner.temp }}/schema_diff.md" + + - name: Post sticky PR comment + uses: marocchino/sticky-pull-request-comment@3d7b8546315c63df45a03981d50a43ec19237f80 # v3 + if: always() + with: + header: schema-diff + path: ${{ runner.temp }}/schema_diff.md diff --git a/changelog.d/20027.misc b/changelog.d/20027.misc new file mode 100644 index 0000000000..bdac0dc058 --- /dev/null +++ b/changelog.d/20027.misc @@ -0,0 +1 @@ +Dust off `make_full_schema` and add CI using it to show schema diffs. \ No newline at end of file diff --git a/docs/development/database_schema.md b/docs/development/database_schema.md index 620d1c16b0..25153a06ad 100644 --- a/docs/development/database_schema.md +++ b/docs/development/database_schema.md @@ -84,17 +84,11 @@ reference only. If you want to recreate these schemas, they need to be made from a database that has had all background updates run. -To do so, use `scripts-dev/make_full_schema.sh`. This will produce new -`full.sql.postgres` and `full.sql.sqlite` files. +To do so, use the `make_full_schema.sh` script. This will produce new +`full.sql.postgres` and `full.sql.sqlite` files, after having applied ALL +schema deltas and background updates. -Ensure postgres is installed, then run: - -```sh -./scripts-dev/make_full_schema.sh -p postgres_username -o output_dir/ -``` - -NB at the time of writing, this script predates the split into separate `state`/`main` -databases so will require updates to handle that correctly. +Run `poetry run scripts-dev/make_full_schema.sh -h` for usage instructions. ## Delta files diff --git a/scripts-dev/make_full_schema.sh b/scripts-dev/make_full_schema.sh index 196869ce8f..3a40b02ff8 100755 --- a/scripts-dev/make_full_schema.sh +++ b/scripts-dev/make_full_schema.sh @@ -7,11 +7,14 @@ export PGHOST="localhost" POSTGRES_MAIN_DB_NAME="synapse_full_schema_main.$$" POSTGRES_COMMON_DB_NAME="synapse_full_schema_common.$$" POSTGRES_STATE_DB_NAME="synapse_full_schema_state.$$" -REQUIRED_DEPS=("matrix-synapse" "psycopg2") + +# Python package names that must be importable +REQUIRED_DEPS=("synapse" "sqlite3" "psycopg2") usage() { echo echo "Usage: $0 -p -o [-c] [-n ] [-h]" + echo "It is the caller's responsibility to be in the correct Python environment (e.g. using \`poetry run scripts-dev/make_full_schema.sh\`)." echo echo "-p " echo " Username to connect to local postgres instance. The password will be requested" @@ -24,14 +27,19 @@ usage() { echo "-n " echo " Schema number for the new snapshot. Used to set the location of files within " echo " the output directory, mimicking that of synapse/storage/schemas." + echo " NOTE: This does not influence which schema deltas are applied to build the full schema." + echo " Schema deltas past this version will still be applied; if you want to build a" + echo " full schema at a particular version, later deltas first need to be" + echo " temporarily deleted (as well as homeserver code temporarily tweaked" + echo " not to rely on any of those changes when applying background updates)" echo " Defaults to 9999." echo "-h" echo " Display this help text." echo "" echo "" echo "You probably want to invoke this with something like" - echo " docker run --rm -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres -e POSTGRES_DB=synapse -p 5432:5432 postgres:11-alpine" - echo " echo postgres | scripts-dev/make_full_schema.sh -p postgres -n MY_SCHEMA_NUMBER -o synapse/storage/schema" + echo " docker run --rm -e POSTGRES_PASSWORD=postgres -e POSTGRES_USER=postgres -e POSTGRES_DB=synapse -p 5432:5432 postgres:14-alpine" + echo " echo postgres | poetry run scripts-dev/make_full_schema.sh -p postgres -n MY_SCHEMA_NUMBER -o synapse/storage/schema" echo "" echo " NB: make sure to run this against the *oldest* supported version of postgres," echo " or else pg_dump might output non-backwards-compatible syntax." @@ -69,10 +77,10 @@ done # Check that required dependencies are installed unsatisfied_requirements=() for dep in "${REQUIRED_DEPS[@]}"; do - pip show "$dep" --quiet || unsatisfied_requirements+=("$dep") + python -c "import $dep" &> /dev/null || unsatisfied_requirements+=("$dep") done if [ ${#unsatisfied_requirements} -ne 0 ]; then - echo "Please install the following python packages: ${unsatisfied_requirements[*]}" + echo 'Please `poetry install --extras postgres` first as the following Python modules are not importable: '"${unsatisfied_requirements[*]}" exit 1 fi @@ -240,16 +248,25 @@ psql "$POSTGRES_STATE_DB_NAME" -w <<< "$DROP_COMMON_TABLES" echo "Dumping SQLite3 schema..." mkdir -p "$OUTPUT_DIR/"{common,main,state}"/full_schemas/$SCHEMA_NUMBER" -sqlite3 "$SQLITE_COMMON_DB" ".schema" > "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" +# `--nosys` prevents emitting (some) SQLite internal tables like `sqlite_sequence` that +# aren't managed by the Synapse application and don't need to be part of our full schema. +# +# (SQLite creates `sqlite_sequence` automatically when the database contains at least one +# table with an AUTOINCREMENT column.) +sqlite3 "$SQLITE_COMMON_DB" ".schema --nosys" > "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" sqlite3 "$SQLITE_COMMON_DB" ".dump --data-only --nosys" >> "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" -sqlite3 "$SQLITE_MAIN_DB" ".schema" > "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" +sqlite3 "$SQLITE_MAIN_DB" ".schema --nosys" > "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" sqlite3 "$SQLITE_MAIN_DB" ".dump --data-only --nosys" >> "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" -sqlite3 "$SQLITE_STATE_DB" ".schema" > "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" +sqlite3 "$SQLITE_STATE_DB" ".schema --nosys" > "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" sqlite3 "$SQLITE_STATE_DB" ".dump --data-only --nosys" >> "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.sqlite" cleanup_pg_schema() { # Cleanup as follows: # - Remove empty lines. pg_dump likes to output a lot of these. + # - Remove the `\restrict` and `\unrestrict` psql meta-commands. + # We don't run the pg_dump output through psql so no meta-commands are + # supported and so the security feature (which doesn't apply anyway + # as we trust the source database) is not relevant. # - Remove comment-only lines. pg_dump also likes to output a lot of these to visually # separate tables etc. # - Remove "public." prefix --- the schema name. @@ -274,6 +291,8 @@ cleanup_pg_schema() { # is `true` or omitted, this marks the given integer as having been consumed and # will NOT appear as the nextval. sed -e '/^$/d' \ + -e '/^\\restrict REMOVEME$/d' \ + -e '/^\\unrestrict REMOVEME$/d' \ -e '/^--/d' \ -e 's/public\.//g' \ -e '/^SET /d' \ @@ -282,12 +301,13 @@ cleanup_pg_schema() { echo "Dumping Postgres schema..." -pg_dump --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_COMMON_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" -pg_dump --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_COMMON_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" -pg_dump --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_MAIN_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" -pg_dump --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_MAIN_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" -pg_dump --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_STATE_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" -pg_dump --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_STATE_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +# --restrict-key: set the \restrict key to a static value (normally a random string) for easy find/replacement in the code above. +pg_dump --restrict-key=REMOVEME --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_COMMON_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +pg_dump --restrict-key=REMOVEME --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_COMMON_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/common/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +pg_dump --restrict-key=REMOVEME --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_MAIN_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +pg_dump --restrict-key=REMOVEME --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_MAIN_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/main/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +pg_dump --restrict-key=REMOVEME --format=plain --schema-only --no-tablespaces --no-acl --no-owner "$POSTGRES_STATE_DB_NAME" | cleanup_pg_schema > "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" +pg_dump --restrict-key=REMOVEME --format=plain --data-only --inserts --no-tablespaces --no-acl --no-owner "$POSTGRES_STATE_DB_NAME" | cleanup_pg_schema >> "$OUTPUT_DIR/state/full_schemas/$SCHEMA_NUMBER/full.sql.postgres" if [[ "$OUTPUT_DIR" == *synapse/storage/schema ]]; then echo "Updating contrib/datagrip symlinks..."