From b47d6afc2fd3ff11346287d80fa0cf3511d65e74 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 19 Oct 2026 18:37:02 +0000 Subject: [PATCH 1/5] test: native_upgrade_converge's twin keeps the original when a run before it died (#1341) The pytest twin staged the previous release's install script by copying whatever was installed to a .pytest-bak backup first, every time. A run that dies before it restores leaves the fixture installed and the original in the backup. The next run then copied the fixture over that backup, so the prefix's own install script was lost for good. The shell suite already guards this case; the twin did not. The staging and restore code moves into _stage and _unstage, which take the extension directory as an argument, so a test can run them against a temporary one. The new test sets up a died run's state there and was red before the fix: "got 'the fixture' want \"the prefix's own file\"". A backup is now taken only when none is there yet, as in the shell. Co-Authored-By: Claude Opus 5.5 --- test/pytest/test_native_upgrade_converge.py | 64 +++++++++++++++++---- 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/test/pytest/test_native_upgrade_converge.py b/test/pytest/test_native_upgrade_converge.py index 5a2c1d54b..49b42a659 100644 --- a/test/pytest/test_native_upgrade_converge.py +++ b/test/pytest/test_native_upgrade_converge.py @@ -23,6 +23,39 @@ import pytest REPO = pathlib.Path(__file__).resolve().parents[2] +FIXTURES = REPO / "test" / "fixtures" +BACKUP = ".pytest-bak" + + +def _extdir(pgc_cluster): + sharedir = subprocess.run([pgc_cluster.pg_config, "--sharedir"], capture_output=True, + text=True, check=True).stdout.strip() + return pathlib.Path(sharedir) / "extension" + + +def _stage(extdir, versions): + """Copy each version's fixture into `extdir`, keeping what was there. -> staged paths.""" + staged = [] + for v in versions: + dst = extdir / f"pgcolumnar--{v}.sql" + backup = dst.with_name(dst.name + BACKUP) + # A backup already here is the original, left by a run that died: taking it + # again would copy the installed fixture over it. + if dst.exists() and not backup.exists(): + shutil.copy2(dst, backup) + shutil.copy2(FIXTURES / f"pgcolumnar--{v}.sql", dst) + staged.append(dst) + return staged + + +def _unstage(staged): + """Put back what `_stage` found, or remove what it added.""" + for dst in staged: + backup = dst.with_name(dst.name + BACKUP) + if backup.exists(): + shutil.move(backup, dst) + else: + dst.unlink(missing_ok=True) def _target_and_previous(): @@ -39,13 +72,7 @@ def window_db(pgc_cluster): target, prev = _target_and_previous() assert len(prev) == 1, f"one upgrade script should lead to {target}, found {prev}" prev = prev[0] - sharedir = subprocess.run([pgc_cluster.pg_config, "--sharedir"], capture_output=True, - text=True, check=True).stdout.strip() - dst = pathlib.Path(sharedir) / "extension" / f"pgcolumnar--{prev}.sql" - backup = dst.with_name(dst.name + ".pytest-bak") - if dst.exists(): - shutil.copy2(dst, backup) - shutil.copy2(REPO / "test" / "fixtures" / f"pgcolumnar--{prev}.sql", dst) + staged = _stage(_extdir(pgc_cluster), [prev]) def admin(sql): with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as c: @@ -60,10 +87,7 @@ def admin(sql): yield dsn, prev finally: admin("DROP DATABASE IF EXISTS pgc_upgrade_window WITH (FORCE)") - if backup.exists(): - shutil.move(backup, dst) - else: - dst.unlink(missing_ok=True) + _unstage(staged) def _one(dsn, sql): @@ -92,6 +116,24 @@ def _succeeds(dsn, *sqls): return f"refused ({exc.sqlstate}: {str(exc).splitlines()[0]})" +def test_a_run_after_one_that_died_keeps_the_original(tmp_path, expect): + """A run that dies before `_unstage` leaves the fixture installed and the original in + the backup. The next run must not back up again: that copies the fixture over the + original, and the prefix's own file is gone for good.""" + v = "1.0-alpha4" + dst = tmp_path / f"pgcolumnar--{v}.sql" + shutil.copy2(FIXTURES / dst.name, dst) + original = "-- the prefix's own install script\n" + dst.with_name(dst.name + BACKUP).write_text(original) + _unstage(_stage(tmp_path, [v])) + left = dst.read_text() if dst.exists() else None + expect.text({original: "the prefix's own file", None: "no file"}.get( + left, "the fixture" if left == (FIXTURES / dst.name).read_text() + else "some other file"), + "the prefix's own file", + "a run after one that died restores the prefix's own file, not the fixture") + + def test_the_upgrade_window(window_db, expect): dsn, prev = window_db expect.text(_one(dsn, "SELECT extversion FROM pg_extension WHERE extname = 'pgcolumnar'"), prev, From f1a8bb5b9d30357d6e8d1631c01c41a67b20645d Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 19 Oct 2026 18:44:36 +0000 Subject: [PATCH 2/5] test: native_upgrade_converge's twin checks that every release converges (#1341) The pytest twin held only the upgrade-window checks. It now also ports, by the shell's names: - the control's default_version; - the staging premises: no earlier run died with a backup still staged, every fixture was staged, and each staged script is the committed fixture; - that a fresh install has objects to compare; - for each of the four fixtures, as its own test: the upgrade applies, the extension reaches the target version, and the catalog snapshot equals a fresh install's. The snapshot query is the shell's. All four fixtures are staged once for the module, and the window test uses the same staging instead of its own. Both twins fail the same checks under two upgrade-script mutations on PG17. Dropping a COMMENT from 1.0-alpha4--1.0-alpha5 fails all four "converges" checks. Dropping one from 1.0-alpha2--1.0-alpha3 fails only the 1.0-alpha and 1.0-alpha2 ones. On the clean tree the shell gives 31/31 and the twin 28/28. The release-tag checks are not ported. CI has no tags, so the shell skips them there, and pytest has no verdict for a check that runs on one checkout and not another. The INCOMPLETE reason now names only them. Co-Authored-By: Claude Opus 5.5 --- test/pytest/test_compare_to_bash.py | 10 +- test/pytest/test_native_upgrade_converge.py | 151 +++++++++++++++++--- 2 files changed, 137 insertions(+), 24 deletions(-) diff --git a/test/pytest/test_compare_to_bash.py b/test/pytest/test_compare_to_bash.py index e532b7139..88f4f7038 100644 --- a/test/pytest/test_compare_to_bash.py +++ b/test/pytest/test_compare_to_bash.py @@ -226,10 +226,12 @@ # about the PORT rather than a licence: the standing arm does not grade it, so the # reason is the only thing standing between a declared gap and a forgotten one. INCOMPLETE = { - # #1341 item 66 ported the arms it added: the upgrade window, this library over - # the previous release's catalog before ALTER EXTENSION UPDATE. - "native_upgrade_converge": "the upgrade-window arms are ported; the fixture-blob, " - "staging and catalog-convergence arms are not", + # #1341 ported every arm but the release-tag ones: each tagged fixture is + # byte-identical to what its tag shipped. CI checks out without tags, so the shell + # skips them there with check_skip, and this layer has no verdict for an arm that + # runs on one checkout and not another. + "native_upgrade_converge": "every arm is ported except the release-tag sweep and " + "the per-tag fixture-blob comparisons", # #1341 item 29 ported the arms four fixes added with shell arms only. Each # pair below holds that fix's arms and nothing else of its shell suite. "inheritance": "the #1259 arms (a partitioned columnar parent is refused with " diff --git a/test/pytest/test_native_upgrade_converge.py b/test/pytest/test_native_upgrade_converge.py index 49b42a659..50ea5b2aa 100644 --- a/test/pytest/test_native_upgrade_converge.py +++ b/test/pytest/test_native_upgrade_converge.py @@ -1,19 +1,23 @@ -"""The upgrade window: this library over the previous release's catalog, before ALTER -EXTENSION UPDATE (#1341 item 66). Part of the pytest twin of -`test/native_upgrade_converge.sh`: these arms only, by the shell's names, and the pair -is declared INCOMPLETE. - -docs/installation.md steps 1 and 2 put a new library in place; step 3, ALTER EXTENSION -UPDATE, brings each database's catalog up to it. Between them a database runs the new -library over the old catalog, and the library deletes from catalog tables the old one -does not have (retired_group, new in 1.0-alpha5). Reads and writes must work there; an -operation that needs a missing table must be refused cleanly, change nothing and name -step 3; and after step 3 every one of them must work on the same rows. - -The previous release is the version the shipped upgrade script to the current -default_version starts from, and its base script is staged from test/fixtures into the -extension directory for the test, as the shell does, then removed. +"""The pytest twin of `test/native_upgrade_converge.sh`, by the shell's names, except its +release-tag arms (see INCOMPLETE in test_compare_to_bash.py). + +ALTER EXTENSION pgcolumnar UPDATE must bring a database on any released version to a +catalog identical to a fresh install of the current default_version: every function's +definition, ACL and comment, every relation, column, type and foreign-data wrapper. +The released starting points are frozen install scripts in test/fixtures. They are not +shipped, so each is staged into the extension directory for the module, keeping what +was there, and put back after. A backup already there means an earlier run died before +putting it back: it holds the original, so it is kept, and a premise says so. + +The upgrade window (#1341 item 66): docs/installation.md steps 1 and 2 put a new library +in place; step 3, ALTER EXTENSION UPDATE, brings each database's catalog up to it. +Between them a database runs the new library over the old catalog, and the library +deletes from catalog tables the old one does not have (retired_group, new in +1.0-alpha5). Reads and writes must work there; an operation that needs a missing table +must be refused cleanly, change nothing and name step 3; and after step 3 every one of +them must work on the same rows. """ +import difflib import pathlib import re import shutil @@ -22,9 +26,32 @@ import psycopg import pytest +from pgc_vacuity import query_error + REPO = pathlib.Path(__file__).resolve().parents[2] FIXTURES = REPO / "test" / "fixtures" BACKUP = ".pytest-bak" +# The released starting points, oldest first; the last is the previous release. +VERSIONS = ["1.0-alpha", "1.0-alpha2", "1.0-alpha3", "1.0-alpha4"] + +# One line per object in the pgcolumnar schema: every function's definition, ACL and +# comment; every relation's kind, ACL and comment; every column's type, NOT NULL and +# comment; every non-base type; every foreign-data wrapper's handler and validator. The +# comments are here because pg_get_functiondef does not emit COMMENT. +SNAPSHOT = """ + SELECT 'FN|'||p.oid::regprocedure||'|'||md5(pg_get_functiondef(p.oid))||'|'||coalesce(p.proacl::text,'(def)')||'|'||md5(coalesce(obj_description(p.oid,'pg_proc'),'')) + FROM pg_proc p JOIN pg_namespace n ON n.oid=p.pronamespace WHERE n.nspname='pgcolumnar' + UNION ALL SELECT 'REL|'||c.relkind::text||'|'||c.relname||'|'||coalesce(c.relacl::text,'(def)')||'|'||md5(coalesce(obj_description(c.oid,'pg_class'),'')) + FROM pg_class c JOIN pg_namespace n ON n.oid=c.relnamespace WHERE n.nspname='pgcolumnar' + UNION ALL SELECT 'COL|'||c.relname||'.'||a.attname||'|'||format_type(a.atttypid,a.atttypmod)||'|'||a.attnotnull::text||'|'||md5(coalesce(col_description(c.oid,a.attnum),'')) + FROM pg_class c JOIN pg_namespace n ON n.oid=c.relnamespace JOIN pg_attribute a ON a.attrelid=c.oid + WHERE n.nspname='pgcolumnar' AND c.relkind IN ('r','p') AND a.attnum>0 AND NOT a.attisdropped + UNION ALL SELECT 'TYP|'||t.typname||'|'||t.typtype::text FROM pg_type t JOIN pg_namespace n ON n.oid=t.typnamespace + WHERE n.nspname='pgcolumnar' AND t.typtype<>'b' + UNION ALL SELECT 'FDW|'||w.fdwname||'|'||coalesce(h.oid::regproc::text,'-')||'|'||coalesce(v.oid::regproc::text,'-') + FROM pg_foreign_data_wrapper w LEFT JOIN pg_proc h ON h.oid=w.fdwhandler LEFT JOIN pg_proc v ON v.oid=w.fdwvalidator + WHERE w.fdwname LIKE 'pgcolumnar%' +""" def _extdir(pgc_cluster): @@ -37,6 +64,8 @@ def _stage(extdir, versions): """Copy each version's fixture into `extdir`, keeping what was there. -> staged paths.""" staged = [] for v in versions: + if not (FIXTURES / f"pgcolumnar--{v}.sql").exists(): + continue dst = extdir / f"pgcolumnar--{v}.sql" backup = dst.with_name(dst.name + BACKUP) # A backup already here is the original, left by a run that died: taking it @@ -66,17 +95,33 @@ def _target_and_previous(): return target, prev +def _admin(pgc_cluster, sql): + with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as c: + c.execute(sql) + + +@pytest.fixture(scope="module") +def staging(pgc_cluster): + """Stage every fixture once for the module. -> (extdir, staged paths, versions whose + backup was already there before staging, which means an earlier run died).""" + extdir = _extdir(pgc_cluster) + died = [v for v in VERSIONS if (extdir / f"pgcolumnar--{v}.sql{BACKUP}").exists()] + staged = _stage(extdir, VERSIONS) + try: + yield extdir, staged, died + finally: + _unstage(staged) + + @pytest.fixture -def window_db(pgc_cluster): +def window_db(pgc_cluster, staging): """-> (dsn, previous version) of a database at the previous release's catalog.""" target, prev = _target_and_previous() assert len(prev) == 1, f"one upgrade script should lead to {target}, found {prev}" prev = prev[0] - staged = _stage(_extdir(pgc_cluster), [prev]) def admin(sql): - with psycopg.connect(pgc_cluster.dsn(), autocommit=True) as c: - c.execute(sql) + _admin(pgc_cluster, sql) admin("DROP DATABASE IF EXISTS pgc_upgrade_window") admin("CREATE DATABASE pgc_upgrade_window") @@ -87,7 +132,6 @@ def admin(sql): yield dsn, prev finally: admin("DROP DATABASE IF EXISTS pgc_upgrade_window WITH (FORCE)") - _unstage(staged) def _one(dsn, sql): @@ -166,3 +210,70 @@ def rows(): "after ALTER EXTENSION UPDATE, pgcolumnar.vacuum() succeeds and keeps them") expect.text(_succeeds(dsn, "TRUNCATE w", "DROP TABLE w"), "succeeded", "after ALTER EXTENSION UPDATE, TRUNCATE and DROP TABLE succeed") + + +def _snapshot(dsn): + with psycopg.connect(dsn, autocommit=True) as c: + return sorted("|".join(r) for r in c.execute(SNAPSHOT).fetchall()) + + +@pytest.fixture(scope="module") +def reference(pgc_cluster, staging): + """-> (target version, snapshot of a fresh install of it): what every upgrade must reach.""" + target, _ = _target_and_previous() + _admin(pgc_cluster, "DROP DATABASE IF EXISTS pgc_conv_fresh") + _admin(pgc_cluster, "CREATE DATABASE pgc_conv_fresh") + try: + dsn = pgc_cluster.dsn(dbname="pgc_conv_fresh") + with psycopg.connect(dsn, autocommit=True) as c: + c.execute(f"CREATE EXTENSION pgcolumnar VERSION '{target}'") + yield target, _snapshot(dsn) + finally: + _admin(pgc_cluster, "DROP DATABASE IF EXISTS pgc_conv_fresh WITH (FORCE)") + + +def test_the_staged_fixtures_are_the_committed_ones(staging, expect): + extdir, staged, died = staging + target, _ = _target_and_previous() + expect.text(target, "1.0-alpha5", "control default_version is 1.0-alpha5") + expect.text(" ".join(died) or "none", "none", + "premise: no earlier run of this suite died with a backup still staged") + expect.num(len(staged), len(VERSIONS), "premise: every fixture named for staging was staged") + wrong = [p.name for p in staged if p.read_bytes() != (FIXTURES / p.name).read_bytes()] + expect.text(" ".join(wrong) or "none", "none", + "every staged install script is the committed fixture, not a leftover") + + +def test_a_fresh_install_is_the_reference(reference, expect): + target, ref = reference + expect.at_least(len(ref), 101, f"fresh {target} install has objects to compare") + + +@pytest.mark.parametrize("from_", VERSIONS) +def test_each_release_upgrades_to_a_fresh_catalog(pgc_cluster, staging, reference, from_, expect): + extdir, _, _ = staging + target, ref = reference + if not (extdir / f"pgcolumnar--{from_}.sql").exists(): + expect.text("missing", "present", f"fixture for {from_} present") + db = "pgc_conv_from_" + re.sub(r"[.-]", "_", from_) + _admin(pgc_cluster, f"DROP DATABASE IF EXISTS {db}") + _admin(pgc_cluster, f"CREATE DATABASE {db}") + try: + dsn = pgc_cluster.dsn(dbname=db) + with psycopg.connect(dsn, autocommit=True) as c: + c.execute(f"CREATE EXTENSION pgcolumnar VERSION '{from_}'") + try: + with psycopg.connect(dsn, autocommit=True) as c: + c.execute(f"ALTER EXTENSION pgcolumnar UPDATE TO '{target}'") + applied = "OK" + except psycopg.Error as exc: + applied = query_error(f"{exc.sqlstate}: {str(exc).splitlines()[0]}") + expect.text(applied, "OK", f"{from_} -> {target} upgrade applies without error") + expect.text(_one(dsn, "SELECT extversion FROM pg_extension WHERE extname = 'pgcolumnar'"), + target, f"{from_} is at {target} after upgrade") + got = _snapshot(dsn) + diverged = list(difflib.unified_diff(ref, got, "fresh", from_, n=0, lineterm=""))[:40] + expect.text("\n".join(diverged) or "CONVERGED", "CONVERGED", + f"{from_} -> {target} converges to a fresh {target} catalog") + finally: + _admin(pgc_cluster, f"DROP DATABASE IF EXISTS {db} WITH (FORCE)") From d3332e46099d0ee77fb31f54326a6f4857c899ba Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 19 Oct 2026 18:55:01 +0000 Subject: [PATCH 3/5] test: register native_upgrade_converge's twin tests and describe them (#1341) cluster_tests 1018 -> 1025: the twin file goes from 1 test to 8, counted by collecting the cluster files on PG17 (1025 collected). TESTS.md section 143 now describes the whole twin, not only the upgrade window. Co-Authored-By: Claude Opus 5.5 --- test/pytest/TESTS.md | 12 +++++++++--- test/pytest/expected_tests.txt | 4 +++- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/test/pytest/TESTS.md b/test/pytest/TESTS.md index 8ab46fb54..ab5ab84d8 100644 --- a/test/pytest/TESTS.md +++ b/test/pytest/TESTS.md @@ -188,7 +188,7 @@ behaviour, the source of that number is named. - [140. test_parquet_import.py: a partitioned columnar parent is refused by import_parquet](#140-test_parquet_importpy-a-partitioned-columnar-parent-is-refused-by-import_parquet) - [141. test_phase5.py: set_options and reset_options agree](#141-test_phase5py-set_options-and-reset_options-agree) - [142. test_drop_cleanup.py: a columnar matview's options row goes with its DROP](#142-test_drop_cleanuppy-a-columnar-matviews-options-row-goes-with-its-drop) -- [143. test_native_upgrade_converge.py: the upgrade window, before ALTER EXTENSION UPDATE](#143-test_native_upgrade_convergepy-the-upgrade-window-before-alter-extension-update) +- [143. test_native_upgrade_converge.py: every release upgrades to a fresh catalog, and the window before it](#143-test_native_upgrade_convergepy-every-release-upgrades-to-a-fresh-catalog-and-the-window-before-it) - [144. test_fk_referencing.py: a foreign key referencing a columnar table is refused where it is chosen](#144-test_fk_referencingpy-a-foreign-key-referencing-a-columnar-table-is-refused-where-it-is-chosen) - [145. test_truncate_cleanup.py: TRUNCATE leaves no catalog rows behind, and keeps what it must](#145-test_truncate_cleanuppy-truncate-leaves-no-catalog-rows-behind-and-keeps-what-it-must) - [146. test_native_dml.py: DELETE, UPDATE and MERGE on a native table agree with heap](#146-test_native_dmlpy-delete-update-and-merge-on-a-native-table-agree-with-heap) @@ -7529,14 +7529,20 @@ Part of the pytest twin of `test/drop_cleanup.sh` (#1265, ported for #1341): its | `test_refresh_keeps_a_matviews_options_row` | a REFRESH that rewrote the matview leaves its set_options row in place | | `test_an_unrelated_options_row_survives` | another table's row survives a matview's DROP, and an ordinary columnar table still takes its own | -## 143. test_native_upgrade_converge.py: the upgrade window, before ALTER EXTENSION UPDATE +## 143. test_native_upgrade_converge.py: every release upgrades to a fresh catalog, and the window before it -Part of the pytest twin of `test/native_upgrade_converge.sh` (#1341 item 66): its upgrade-window arms only, by the shell's names, and the pair is declared INCOMPLETE. Between installing a new library and running `ALTER EXTENSION pgcolumnar UPDATE`, a database runs the new library over its previous release's catalog. Reads and writes must work there. `DROP TABLE`, `TRUNCATE` and `pgcolumnar.vacuum()`, which need a catalog table the old one lacks, must be refused with 42P01, change nothing, and carry a hint naming the update; after the update all three succeed on the same rows. The previous release is the version the shipped upgrade script to the current `default_version` starts from; its base script is staged from `test/fixtures` for the test and removed after. +The pytest twin of `test/native_upgrade_converge.sh` (#1341), by the shell's names, except its release-tag checks, so the pair is declared INCOMPLETE. `ALTER EXTENSION pgcolumnar UPDATE` must bring a database on any released version to a catalog identical to a fresh install of the current `default_version`: every function's definition, ACL and comment, and every relation, column, type and foreign-data wrapper. The released starting points are the install scripts in `test/fixtures`. They are staged into the extension directory once for the module, under a lock that makes a second stager wait, and whatever was there is put back after. A backup already there means an earlier run died, so it holds the original and is kept. + +Between installing a new library and running the update, a database runs the new library over its previous release's catalog (#1341 item 66). Reads and writes must work there. `DROP TABLE`, `TRUNCATE` and `pgcolumnar.vacuum()` need a catalog table the old one lacks, so they must be refused with 42P01, change nothing, and carry a hint naming the update. After the update all three succeed on the same rows. ### Every test | test | what it holds | | --- | --- | +| `test_a_run_after_one_that_died_keeps_the_original` | with a died run's state in a temporary directory, staging and restoring leave the prefix's own file, not the fixture | +| `test_two_stagings_at_once_keep_the_original` | a second staging that starts inside the first, as another `pytest -n` worker's would, waits for it under the staging lock, and the prefix's own file survives both | +| `test_the_staged_fixtures_are_the_committed_ones` | the control's `default_version`; no backup was already staged; every fixture was staged, and each staged script is the committed fixture | +| `test_each_release_upgrades_to_a_fresh_catalog` | parametrized by the four fixtures: a fresh install of the target has more than 100 objects to compare, the upgrade applies, the extension is at the target, and its catalog equals the fresh install's | | `test_the_upgrade_window` | at the previous catalog: rows written, deleted and read; the three operations refused with the hint and nothing changed; after the update the rows are intact and all three succeed | ## 144. test_fk_referencing.py: a foreign key referencing a columnar table is refused where it is chosen diff --git a/test/pytest/expected_tests.txt b/test/pytest/expected_tests.txt index 0301996d0..6b01c6e7c 100644 --- a/test/pytest/expected_tests.txt +++ b/test/pytest/expected_tests.txt @@ -856,4 +856,6 @@ guard_tests 442 # native_groupagg_wide_cost.sh: five tests, one per check, by collection over the cluster files. # cluster_tests 1011 -> 1018 for #1341's test_hardening.py, the twin of hardening.sh: seven # tests, one per check, by collection over the cluster files. -cluster_tests 1018 +# cluster_tests 1018 -> 1025 for #1341, test_native_upgrade_converge.py ported by name except its +# release-tag arms (1 -> 8 tests), by collection over the cluster files. +cluster_tests 1025 From 6fba179d943afc8e6e71ccf8ab4a1f1a05f9131f Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 19 Oct 2026 18:59:31 +0000 Subject: [PATCH 4/5] test: native_upgrade_converge's twin fixtures read what they are handed (#1341) test_build_refusal.py's guard rejects a helper that takes a parameter it never reads, and two of this twin's fixtures did. reference took staging but a fresh install of the target needs no staged fixture, so it no longer takes it. window_db took staging only to be ordered after it; it now uses it to require that the previous release's install script is staged. Co-Authored-By: Claude Opus 5.5 --- test/pytest/test_native_upgrade_converge.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/pytest/test_native_upgrade_converge.py b/test/pytest/test_native_upgrade_converge.py index 50ea5b2aa..686b786f6 100644 --- a/test/pytest/test_native_upgrade_converge.py +++ b/test/pytest/test_native_upgrade_converge.py @@ -119,6 +119,8 @@ def window_db(pgc_cluster, staging): target, prev = _target_and_previous() assert len(prev) == 1, f"one upgrade script should lead to {target}, found {prev}" prev = prev[0] + extdir, staged, _ = staging + assert extdir / f"pgcolumnar--{prev}.sql" in staged, f"{prev} is not among the staged fixtures" def admin(sql): _admin(pgc_cluster, sql) @@ -218,7 +220,7 @@ def _snapshot(dsn): @pytest.fixture(scope="module") -def reference(pgc_cluster, staging): +def reference(pgc_cluster): """-> (target version, snapshot of a fresh install of it): what every upgrade must reach.""" target, _ = _target_and_previous() _admin(pgc_cluster, "DROP DATABASE IF EXISTS pgc_conv_fresh") From de0945dad4916097186c527c688292fd239f4b59 Mon Sep 17 00:00:00 2001 From: "Joshua D. Drake" Date: Mon, 19 Oct 2026 19:16:47 +0000 Subject: [PATCH 5/5] test: native_upgrade_converge's twin stages under a lock, and asserts its premise where it compares (#1341) Two findings from OffgridwithJD's review of #1536. Under `pytest -n` (test/pytest/README.md documents -n 4), each worker staged the fixtures into the same share/extension. Worker B kept A's backup and restored it, and then A, finding no backup, deleted the file it took for its fixture: the prefix's own install script. _staged now holds an exclusive flock for the whole staging, in a lock file beside pgc_cluster's build lock, keyed by the directory. The new test_two_stagings_at_once_keep_the_original forces that interleaving with two threads. Before the lock it was red: "got 'no file'". On PG17, five -n 4 runs of this file at the previous head gave 4 red runs, and five at this one gave 5 green, with the directory listing unchanged. Two empty snapshots compare equal, and the "more than 100 objects" premise was a test of its own, so with the snapshot query pointed at a missing schema, -k fresh_catalog gave 4 passed. Each convergence test now asserts the premise before it compares, and the same mutation gives 4 failed: "got 0, wanted at least 101". test_a_fresh_install_is_the_reference is gone, because its one check now lives in those tests. Co-Authored-By: Claude Opus 5.5 --- test/pytest/test_native_upgrade_converge.py | 78 ++++++++++++++++++--- 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/test/pytest/test_native_upgrade_converge.py b/test/pytest/test_native_upgrade_converge.py index 686b786f6..d1f220b17 100644 --- a/test/pytest/test_native_upgrade_converge.py +++ b/test/pytest/test_native_upgrade_converge.py @@ -17,11 +17,17 @@ must be refused cleanly, change nothing and name step 3; and after step 3 every one of them must work on the same rows. """ +import contextlib import difflib +import fcntl +import hashlib +import os import pathlib import re import shutil import subprocess +import tempfile +import threading import psycopg import pytest @@ -87,6 +93,29 @@ def _unstage(staged): dst.unlink(missing_ok=True) +@contextlib.contextmanager +def _staged(extdir, versions): + """Stage `versions` into `extdir` for the life of the block, then put back what was + there. -> (staged paths, versions whose backup was already there: an earlier run died). + + Holds an exclusive lock for the whole block, so a second stager of the same directory + (another `pytest -n` worker) waits until this one has put everything back. Without it, + the second keeps the first's backup and restores it, and the first then deletes the + original. The lock file lives with the build lock in pgc_cluster.build_once, keyed by + the directory, so nothing is added to the prefix.""" + lock_dir = pathlib.Path(tempfile.gettempdir()) / f"pgc-pytest-{os.getuid()}" + lock_dir.mkdir(mode=0o700, exist_ok=True) + key = hashlib.sha256(str(extdir.resolve()).encode()).hexdigest()[:16] + with open(lock_dir / f"staging-{key}.lock", "w") as lock: + fcntl.flock(lock, fcntl.LOCK_EX) + died = [v for v in versions if (extdir / f"pgcolumnar--{v}.sql{BACKUP}").exists()] + staged = _stage(extdir, versions) + try: + yield staged, died + finally: + _unstage(staged) + + def _target_and_previous(): target = re.search(r"^default_version\s*=\s*'([^']+)'", (REPO / "pgcolumnar.control").read_text(), re.M).group(1) @@ -105,12 +134,8 @@ def staging(pgc_cluster): """Stage every fixture once for the module. -> (extdir, staged paths, versions whose backup was already there before staging, which means an earlier run died).""" extdir = _extdir(pgc_cluster) - died = [v for v in VERSIONS if (extdir / f"pgcolumnar--{v}.sql{BACKUP}").exists()] - staged = _stage(extdir, VERSIONS) - try: + with _staged(extdir, VERSIONS) as (staged, died): yield extdir, staged, died - finally: - _unstage(staged) @pytest.fixture @@ -180,6 +205,41 @@ def test_a_run_after_one_that_died_keeps_the_original(tmp_path, expect): "a run after one that died restores the prefix's own file, not the fixture") +def test_two_stagings_at_once_keep_the_original(tmp_path, expect): + """Two processes staging one extension directory at once, as `pytest -n` workers + do. Unserialised, B keeps A's backup and restores it, and then A, finding no backup, + deletes the file it takes for its own fixture: the prefix's original (review of + #1536). B starts inside A's staging and is given a moment to interleave.""" + v = "1.0-alpha4" + dst = tmp_path / f"pgcolumnar--{v}.sql" + original = "-- the prefix's own install script\n" + dst.write_text(original) + a_in, a_go = threading.Event(), threading.Event() + + def a(): + with _staged(tmp_path, [v]): + a_in.set() + a_go.wait(10) + + def b(): + with _staged(tmp_path, [v]): + pass + + ta = threading.Thread(target=a) + ta.start() + a_in.wait(10) + tb = threading.Thread(target=b) + tb.start() + tb.join(0.5) + a_go.set() + ta.join(10) + tb.join(10) + left = dst.read_text() if dst.exists() else None + expect.text({original: "the prefix's own file", None: "no file"}.get(left, "some other file"), + "the prefix's own file", + "two stagings at once leave the prefix's own file in place") + + def test_the_upgrade_window(window_db, expect): dsn, prev = window_db expect.text(_one(dsn, "SELECT extversion FROM pg_extension WHERE extname = 'pgcolumnar'"), prev, @@ -246,15 +306,13 @@ def test_the_staged_fixtures_are_the_committed_ones(staging, expect): "every staged install script is the committed fixture, not a leftover") -def test_a_fresh_install_is_the_reference(reference, expect): - target, ref = reference - expect.at_least(len(ref), 101, f"fresh {target} install has objects to compare") - - @pytest.mark.parametrize("from_", VERSIONS) def test_each_release_upgrades_to_a_fresh_catalog(pgc_cluster, staging, reference, from_, expect): extdir, _, _ = staging target, ref = reference + # In this test, not one of its own: two empty snapshots compare equal, so the + # comparison below is evidence only where this holds. + expect.at_least(len(ref), 101, f"fresh {target} install has objects to compare") if not (extdir / f"pgcolumnar--{from_}.sql").exists(): expect.text("missing", "present", f"fixture for {from_} present") db = "pgc_conv_from_" + re.sub(r"[.-]", "_", from_)