This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7076-baefb5d5984c86a78b829167ae2344996f2ef5a2 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 052cf38c02797d8c10afceea2c39f11d3d16dfdd Author: Yicong Huang <[email protected]> AuthorDate: Thu Aug 13 05:46:48 2026 +0000 fix(local-dev): don't abort `up` on an already-applied sql/updates changeSet (#7076) ### What changes were proposed in this PR? `bin/local-dev.sh up` against a fresh docker volume never reached the sbt build: it died on the last `sql/updates` changeSet. Postgres applies `sql/texera_ddl.sql` itself — compose mounts `sql/` into `/docker-entrypoint-initdb.d` — and that DDL is kept in sync with `sql/updates/*`, so the changeSets local-dev replays immediately afterwards re-create objects that are already there. 23–27 are incidentally idempotent and pass; `28.sql`'s `dataset_owner_uid_name_key` is not, and `sql/texera_ddl.sql`'s `UNIQUE (owner_uid, name)` on `dataset` already created it under exactly that auto-generated name. `infra_ensure_db_schema` picks seed-vs-replay by probing for the `feedback` table. On a fresh volume the entrypoint has just created it, so the replay path is taken — and the `seed` branch written for this very case (record every changeSet as applied without executing it) is unreachable, because the entrypoint always wins the race. ``` Before: fresh volume -> entrypoint applies full DDL -> replay 23-28 -> 28 fails -> no build After: fresh volume -> entrypoint applies full DDL -> 28 recorded as applied -> build runs ``` The fix is in the replay loop rather than the probe: a psql failure whose every `ERROR:` line is `already exists` means the changeSet's effect is already in the schema, so record it and carry on. That covers the next `sql/updates/N.sql` that isn't accidentally idempotent too, instead of fixing only `28.sql`. `_sql_errors_all_already_exist` is deliberately narrow — a `duplicate key` is a data conflict rather than an applied schema change, and a failure with no `ERROR:` line at all is never assumed harmless — so an incomplete schema still stops the build instead of reaching jOOQ codegen with tables that aren't there. While in the same lines: psql's stderr is kept instead of redirected to `/dev/null`. It holds the one line that explains the abort, and the old code discarded it and then told the operator to re-run the file by hand to find out why. ### Any related issues, documentation, discussions? Closes #7064 ### How was this PR tested? Unit coverage for the detector in both directions, plus two structural guards on the wiring, in the existing `infra`-job suite: ``` $ bash bin/local-dev/tests/test_local_dev_sh.sh ... ✓ already-applied detector: relation already exists (the #7064 failure) ✓ already-applied detector: several already-exists errors, nothing else ✓ already-applied detector: already-exists around harmless NOTICE/ROLLBACK chatter ✓ already-applied detector: non-ASCII identifier already exists ✓ already-applied detector: syntax error ✓ already-applied detector: missing relation ✓ already-applied detector: one already-exists mixed with one real error ✓ already-applied detector: duplicate key is a data conflict, not an applied change ✓ already-applied detector: empty stderr ✓ already-applied detector: no ERROR line at all ✓ already-applied detector: missing stderr file ✓ already-applied detector: no argument ✓ infra_apply_sql_updates consults the already-applied detector ✓ infra_apply_sql_updates keeps psql stderr for diagnosis 64 passed, 0 failed ``` The pytest half of the same job reports `1 failed, 42 passed` on this branch. That failure is `test_is_dirty_after_seed_then_edit`, which is unrelated to this change — it reproduces identically on a pristine `main` checkout, and this PR touches neither `tui.py` nor that test. It is #7075, fixed separately. End-to-end on the real stack (Ubuntu 24.04.4, docker 29.1.3), reproducing the failure and then confirming the fix: ```sh bin/local-dev.sh down docker volume rm texera-local-dev_postgres_data bin/local-dev.sh up ``` Before: ``` → postgres: applying sql/updates/28.sql (changeSet 28) ✗ postgres: sql/updates/28.sql failed -- inspect with: docker exec -i texera-postgres psql -U texera -d texera_db < sql/updates/28.sql ``` After: ``` → postgres: applying sql/updates/27.sql (changeSet 27) → postgres: applying sql/updates/28.sql (changeSet 28) ○ postgres: sql/updates/28.sql already in schema (recording changeSet 28) ✓ postgres: 6 sql/update(s) applied ... ✓ 14 of 14 services healthy ``` The changeSet is recorded, so it is not retried on the next run: ``` $ docker exec texera-postgres psql -U texera -d texera_db -tAc \ "SELECT id||':'||exectype FROM public.databasechangelog ORDER BY orderexecuted" 23:EXECUTED 24:EXECUTED 25:EXECUTED 26:EXECUTED 27:EXECUTED 28:EXECUTED ``` ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> Co-authored-by: Xinyuan Lin <[email protected]> --- bin/local-dev/main.sh | 53 +++++++++++++++++-- bin/local-dev/tests/test_local_dev_sh.sh | 91 ++++++++++++++++++++++++++++++++ 2 files changed, 141 insertions(+), 3 deletions(-) diff --git a/bin/local-dev/main.sh b/bin/local-dev/main.sh index 58fa687f0d..c342ad8bd0 100755 --- a/bin/local-dev/main.sh +++ b/bin/local-dev/main.sh @@ -1500,6 +1500,41 @@ parse_changelog_changesets() { ' "$changelog" } +# True when every error psql reported is a complaint that the object it tried +# to create is already there — i.e. the changeSet's effect is already in the +# schema and replaying it was a no-op. +# +# Why this is needed: on a fresh volume postgres runs sql/texera_ddl.sql itself +# (compose mounts ../../sql to /docker-entrypoint-initdb.d), and that DDL is +# kept in sync with sql/updates/*, so the changeSets we replay right afterwards +# re-create objects that already exist. The ones incidentally written with +# IF NOT EXISTS pass; the rest aborted `up` before the build ever started +# (#7064) — e.g. 28.sql's dataset_owner_uid_name_key, which texera_ddl.sql +# already creates as the auto-generated name for UNIQUE (owner_uid, name). +# +# Deliberately narrow. Only `already exists` counts: +# • `duplicate key` is a data conflict, not an applied schema change +# • no ERROR line at all means we cannot see why psql failed, and a failure +# we can't explain is never assumed harmless +# Everything else — syntax errors, missing relations, permissions — keeps +# failing loudly, so a genuinely incomplete schema still stops the build +# instead of reaching jOOQ codegen. +# +# The "already in the schema" equivalence holds only while texera_ddl.sql stays +# in sync with sql/updates/* (it does, by construction). psql runs with +# ON_ERROR_STOP=1 and halts at the first error, so "every error is already-exists" +# proves the whole changeSet is a no-op only when every object it touches is +# already present. Were the two to drift, a changeSet mixing an existing object +# (hit first) with a genuinely new one would be recorded as applied without the +# new object ever being created — surfacing later as a jOOQ codegen failure. +_sql_errors_all_already_exist() { + local f="${1:-}" errs="" + [[ -f "$f" ]] || return 1 + errs=$(grep 'ERROR:' "$f" 2>/dev/null) || return 1 + [[ -n "$errs" ]] || return 1 + ! printf '%s\n' "$errs" | grep -qv 'already exists' +} + # Reconcile sql/updates/* with the live DB so jOOQ codegen (which reads the # database at sbt-compile time) sees the schema the checked-out code expects. # The repo's official runner is liquibase (sql/docker-compose.yml — manual, @@ -1567,12 +1602,24 @@ infra_apply_sql_updates() { return 1 fi tui_step "postgres: applying $path (changeSet $id)" + # Keep psql's stderr: it holds the one line that explains an abort, + # and _sql_errors_all_already_exist needs it to tell "this changeSet + # is already in the schema" apart from a real failure. + local psql_err="$LOG_DIR/psql-changeset-$id.err" if ! sed 's/^\\c.*$//' "$sql_file" \ | docker exec -i "$pg" psql -U texera -d texera_db \ - -v ON_ERROR_STOP=1 -f - >/dev/null 2>&1; then - tui_err "postgres: $path failed -- inspect with: docker exec -i texera-postgres psql -U texera -d texera_db < $path" - return 1 + -v ON_ERROR_STOP=1 -f - >/dev/null 2>"$psql_err"; then + if _sql_errors_all_already_exist "$psql_err"; then + # Fall through to the INSERT below so the changeSet is + # recorded as applied and later runs stop retrying it. + tui_skip "postgres: $path already in schema (recording changeSet $id)" + else + tui_err "postgres: $path failed -- inspect with: docker exec -i texera-postgres psql -U texera -d texera_db < $path" + sed 's/^/ /' "$psql_err" >&2 + return 1 + fi fi + rm -f "$psql_err" fi docker exec "$pg" psql -U texera -d texera_db -qc " INSERT INTO public.databasechangelog diff --git a/bin/local-dev/tests/test_local_dev_sh.sh b/bin/local-dev/tests/test_local_dev_sh.sh index fbcd48a87f..f92666ab72 100755 --- a/bin/local-dev/tests/test_local_dev_sh.sh +++ b/bin/local-dev/tests/test_local_dev_sh.sh @@ -910,5 +910,96 @@ else _fail "svc_src_changed still compares directly against \$stamp" fi +# 36) A changeSet the DB already satisfies must not abort `up`. On a fresh +# volume postgres' own /docker-entrypoint-initdb.d runs sql/texera_ddl.sql, +# which is kept in sync with sql/updates/*, so replaying those changeSets +# re-creates objects that are already there; the ones not written with +# IF NOT EXISTS used to kill `up` before the build ever started (#7064). +# `_sql_errors_all_already_exist` is the gate that tells that case apart +# from a real failure, so it's tested in both directions: it must say yes +# only for "already exists", and no for every other psql error, for a +# duplicate-key data conflict, and for input where it can't see any error +# at all (never assume harmless). +tolerate_fn=$(awk '/^_sql_errors_all_already_exist\(\)/{f=1} f{print} f&&/^}/{exit}' "$MAIN_SH") +if [[ -z "$tolerate_fn" ]]; then + _fail "_sql_errors_all_already_exist helper missing" +else + _tol_dir=$(mktemp -d 2>/dev/null || mktemp -d -t ldtol) + _tol_check() { # $1=label $2=expected rc $3=stderr contents + printf '%s' "$3" > "$_tol_dir/err" + local rc=0 + ( eval "$tolerate_fn"; _sql_errors_all_already_exist "$_tol_dir/err" ) || rc=$? + if (( rc == $2 )); then + _pass "already-applied detector: $1" + else + _fail "already-applied detector: $1" "expected rc=$2, got rc=$rc" + fi + } + # Positive: the real #7064 failure, and the other spellings postgres uses + # for an object that is already present. + _tol_check "relation already exists (the #7064 failure)" 0 \ + 'psql:<stdin>:44: ERROR: relation "dataset_owner_uid_name_key" already exists' + _tol_check "several already-exists errors, nothing else" 0 \ + 'psql:<stdin>:9: ERROR: column "x" of relation "dataset" already exists +psql:<stdin>:12: ERROR: constraint "y" for relation "dataset" already exists +psql:<stdin>:15: ERROR: type "z" already exists' + _tol_check "already-exists around harmless NOTICE/ROLLBACK chatter" 0 \ + 'NOTICE: Renamed 0 duplicate dataset name(s) +psql:<stdin>:44: ERROR: relation "dataset_owner_uid_name_key" already exists +ROLLBACK' + _tol_check "non-ASCII identifier already exists" 0 \ + 'psql:<stdin>:3: ERROR: relation "café_naïve_key" already exists' + # Negative: anything we can't positively identify as already-applied has to + # keep failing loudly, or a genuinely broken schema reaches the sbt build. + _tol_check "syntax error" 1 \ + 'psql:<stdin>:7: ERROR: syntax error at or near "ALTERR"' + _tol_check "missing relation" 1 \ + 'psql:<stdin>:7: ERROR: relation "dataset" does not exist' + _tol_check "one already-exists mixed with one real error" 1 \ + 'psql:<stdin>:44: ERROR: relation "dataset_owner_uid_name_key" already exists +psql:<stdin>:51: ERROR: syntax error at or near "COMMITT"' + _tol_check "duplicate key is a data conflict, not an applied change" 1 \ + 'psql:<stdin>:44: ERROR: duplicate key value violates unique constraint "dataset_pkey"' + _tol_check "empty stderr" 1 '' + _tol_check "no ERROR line at all" 1 \ + 'NOTICE: table "dataset" does not exist, skipping' + # Edge: nothing to read. Guard against the helper "succeeding" on a path + # that was never written, which would swallow every failure. + rm -f "$_tol_dir/err" + rc=0 + ( eval "$tolerate_fn"; _sql_errors_all_already_exist "$_tol_dir/err" ) || rc=$? + if (( rc == 1 )); then + _pass "already-applied detector: missing stderr file" + else + _fail "already-applied detector: missing stderr file" "expected rc=1, got rc=$rc" + fi + rc=0 + ( eval "$tolerate_fn"; _sql_errors_all_already_exist ) || rc=$? + if (( rc == 1 )); then + _pass "already-applied detector: no argument" + else + _fail "already-applied detector: no argument" "expected rc=1, got rc=$rc" + fi + rm -rf "$_tol_dir" +fi + +# 37) Wiring for #36: the replay loop must consult the detector instead of +# aborting on the first psql failure, and must stop discarding psql's +# stderr — the old `2>&1` to /dev/null meant the one line that explains the +# abort ("relation ... already exists") never reached the operator, who was +# told to re-run the file by hand to find out why. +updates_body=$(awk '/^infra_apply_sql_updates\(\)/{f=1} f{print} f&&/^}/{exit}' "$MAIN_SH") +if [[ "$updates_body" == *"_sql_errors_all_already_exist"* ]]; then + _pass "infra_apply_sql_updates consults the already-applied detector" +else + _fail "infra_apply_sql_updates aborts without checking for already-applied changeSets" +fi +if [[ "$updates_body" == *"ON_ERROR_STOP"* ]] \ + && ! printf '%s' "$updates_body" | grep -qE '\-f -[[:space:]]*>/dev/null[[:space:]]*2>&1'; then + _pass "infra_apply_sql_updates keeps psql stderr for diagnosis" +else + _fail "infra_apply_sql_updates still throws psql stderr away" +fi + printf "\n%d passed, %d failed\n" "$PASS" "$FAIL" (( FAIL == 0 ))
