feat: test pg_upgrade compatibility with older extension versions #1897
Conversation
samrose
left a comment
samrose
left a comment
There was a problem hiding this comment.
Looks great! Will just need to test before we merge it
jfroche
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
8acfa60 to
f59c791
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
f59c791 to
779090b
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
24d66bb to
164f401
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
164f401 to
450b690
Compare
This comment has been minimized.
This comment has been minimized.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
f6f9d9b to
9beb48a
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
9beb48a to
218357a
Compare
jfroche
force-pushed
the
custom-github-runners
branch
from
c1c4fd7 to
9b659e4
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
971248f to
84cfa8d
Compare
yvan-sraka
force-pushed
the
custom-github-runners
branch
from
efd8f49 to
a5fc472
Compare
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
2 times, most recently
from
39db71b to
29a4d56
Compare
yvan-sraka
force-pushed
the
custom-github-runners
branch
2 times, most recently
from
03dc754 to
c159bb6
Compare
jfroche
force-pushed
the
custom-github-runners
branch
3 times, most recently
from
df5545b to
b9294ee
Compare
📝 WalkthroughWalkthroughReplaces inline PostgreSQL major-version usage with a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Failure to add the new IP will result in interrupted reviews. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/pg_stat_monitor.nix`:
- Around line 20-29: latestVersion is computed from allVersionsList but packages
are built from pgUpgradeCompatibleVersions, causing a mismatch if the newest
supported version is not pgUpgradeCompatible; change the latestVersion
definition to derive from the pgUpgradeCompatible set (e.g. use lib.last on the
attrNames of pgUpgradeCompatibleVersions or on a sorted list of
pgUpgradeCompatible version keys) so latestVersion and packages come from the
same pg_upgrade-compatible collection (update the latestVersion variable to
reference pgUpgradeCompatibleVersions instead of allVersionsList).
In `@nix/ext/tests/pg_stat_monitor.nix`:
- Around line 190-191: The PostgresExtensionTest is being instantiated without
the computed lib_name, causing the constructor to default to extension_name;
update the PostgresExtensionTest(...) call that currently uses (server,
extension_name, versions, sql_test_directory, support_upgrade, ext_schema) so it
includes the computed lib_name argument (the variable lib_name) in the parameter
list passed to PostgresExtensionTest, ensuring subsequent uses (e.g.,
test.create_schema() and later references to lib_name) target the correct
library name.
🧹 Nitpick comments (2)
nix/ext/pg_jsonschema/default.nix (1)
96-96: Consider removing or making debug output explicit.The
find $outcommand appears to be debugging output that lists build artifacts. If intentional for troubleshooting, consider adding a comment explaining its purpose or wrapping it in anechostatement. If unintentional, it can be removed.🔧 Suggested change if keeping for debugging
postInstall = '' - find $out + # Debug: list build artifacts + echo "Build artifacts:" && find $out mv $out/lib/${pname}${postgresql.dlSuffix} $out/lib/${pname}-${version}${postgresql.dlSuffix}Or remove if not needed:
postInstall = '' - find $out mv $out/lib/${pname}${postgresql.dlSuffix} $out/lib/${pname}-${version}${postgresql.dlSuffix}nix/ext/tests/pgsodium.nix (1)
197-223: Ensure pg_upgrade completes before checking update script.
switch-to-configurationcan return before services are fully ready; the immediate check forupdate_extensions.sqlcan race and fliphas_update_scripttono. Consider waiting forpostgresql.service(orpostgresql-migrate.service) after each switch.🔧 Suggested wait to reduce flakiness
server.succeed( f"{pg15_configuration}/bin/switch-to-configuration test >&2" ) + server.wait_for_unit("postgresql.service") test.drop_extension() test.install_extension(version) server.succeed( f"{pg17_configuration}/bin/switch-to-configuration test >&2" ) + server.wait_for_unit("postgresql.service") has_update_script = server.succeed( "test -f /var/lib/postgresql/update_extensions.sql && echo 'yes' || echo 'no'" ).strip() == "yes"
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
nix/ext/pg_graphql/default.nixnix/ext/pg_jsonschema/default.nixnix/ext/pg_stat_monitor.nixnix/ext/tests/default.nixnix/ext/tests/lib.pynix/ext/tests/pg_stat_monitor.nixnix/ext/tests/pgmq.nixnix/ext/tests/pgroonga.nixnix/ext/tests/pgsodium.nixnix/ext/tests/vault.nixnix/ext/versions.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: nix-eval / eval
🔇 Additional comments (26)
nix/ext/tests/lib.py (1)
57-58: LGTM - CASCADE ensures clean teardown.Adding
CASCADEto theDROP EXTENSIONcommand is appropriate for test cleanup, ensuring dependent objects (views, functions, etc.) are properly removed between test iterations. This aligns with the multi-version upgrade testing scenarios introduced in this PR.nix/ext/pg_graphql/default.nix (3)
122-124: LGTM - Centralizing pgVersion improves maintainability.Extracting
pgVersionas a variable and using it consistently throughout the file is a good refactoring that aligns with the same pattern inpg_jsonschema/default.nix.
132-162: LGTM - SQL-only packages enable pg_upgrade for unsupported versions.The
buildUnsupportedfunction correctly creates minimal SQL-only extension packages that pg_upgrade needs to reference old extension versions without requiring them to compile against the new PostgreSQL version. The approach of concatenating SQL files with semicolon separators is appropriate for this migration-only use case.
163-179: LGTM - Conditional inclusion of SQL-only packages for pg_upgrade.The logic correctly:
- Filters versions incompatible with the current
pgVersion- Skips generating SQL-only packages for PG15 (no older versions to upgrade from)
- Includes SQL-only packages in the build environment for PG17+ to support pg_upgrade
nix/ext/tests/pgroonga.nix (2)
141-143: LGTM - Configuration path variables.The explicit system and configuration path variables improve readability and provide clear references for switching between PG15 and PG17 configurations during upgrade testing.
196-221: LGTM - Comprehensive pg_upgrade test for all PG15 extension versions.This test thoroughly validates the upgrade path by:
- Testing each PG15 extension version individually
- Handling both scenarios: with update scripts (upgrade to latest) and without (version preserved)
- Running pg_regress validation after each upgrade
The clean state reset (stopping postgres, removing data) between iterations ensures isolated test runs.
nix/ext/pg_jsonschema/default.nix (2)
78-80: LGTM - Consistent pgVersion usage in preCheck.The
pgVersionvariable is used consistently for rsync paths and cargo pgrx initialization, matching the pattern established inpg_graphql/default.nix.
135-136: LGTM - Unsupported version handling for pg_upgrade.The approach correctly:
- Identifies PG15-only versions that won't compile against PG17
- Only generates placeholders when building for PG17
- Copies SQL from the earliest supported version as a migration placeholder
Note: This differs from
pg_graphqlwhich concatenates source SQL files. The simpler copy approach here works because pg_jsonschema's SQL structure allows it.Also applies to: 153-155
nix/ext/versions.json (1)
778-779: LGTM - Marking pg_stat_monitor 1.0 as pg_upgrade incompatible.The
pgUpgradeCompatible: falseflag correctly excludes this PG15-only version from pg_upgrade testing. The field is properly consumed innix/ext/pg_stat_monitor.nix(lines 21, 25-29) to filter out incompatible versions from both the version list and build packages. Version 2.1, which supports both PG15 and PG17, will be used for upgrade validation instead.nix/ext/tests/pgmq.nix (2)
122-125: LGTM: system-derived pg15/pg17 configuration paths.This keeps configuration switching consistent with other test files.
182-205: LGTM: pg_upgrade coverage across older PG15 extension versions.The loop validates upgrade behavior and update-script handling in a clear, repeatable way.
nix/ext/tests/vault.nix (2)
149-151: LGTM: consistent pg15/pg17 configuration derivation.
206-233: LGTM: pg_upgrade loop validates older-version upgrades and regression coverage.nix/ext/tests/default.nix (3)
153-155: LGTM: system-based pg15/pg17 configuration paths.
212-237: LGTM: pg_upgrade loop for older extension versions.
261-264: LGTM: extension list update (pg_partman, postgis).nix/ext/tests/pg_stat_monitor.nix (2)
30-119: LGTM: dedicated pg_stat_monitor test harness with pg15/pg17 specialisation.
230-264: LGTM: conditional pg_regress skip when no update script is generated.Clear rationale and targeted handling for schema divergence after pg_upgrade.
nix/ext/tests/pgsodium.nix (8)
50-51: LGTM: explicit psql_15/psql_17 bindings.
Keeps reuse consistent across service and migration configuration.
71-85: Verify peer auth mapping matches how tests connect.
Withlocal all postgres ... map=postgresand nopostgres root postgresentry, anypsql -U postgresexecuted as root will fail. Please confirm the harness usessudo -u postgres(or similar), or add a mapping if root needs to connect aspostgres.Optional fallback if root must connect as postgres
identMap = '' root root supabase_admin + postgres root postgres postgres postgres postgres '';
94-94: LGTM: pg17 specialization uses psql_17.
108-109: LGTM: explicit old/new PostgreSQL packages for pg_upgrade.
136-151: LGTM: harness wiring with explicit config variables.
158-175: LGTM: PG15 upgrade/pg_regress coverage.
179-179: LGTM: switch uses pg17_configuration.
182-195: LGTM: PG17 regression and last-version checks.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
4993025 to
f355ff2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/pg_jsonschema/default.nix`:
- Around line 124-136: The unsupported-version detection is too narrow: replace
the filter that defines unsupportedVersionsItems (currently using
value.postgresql == [ "15" ]) with one that selects entries where the current
pgVersion is not listed (use !(builtins.elem pgVersion value.postgresql)), and
then set unsupportedVersions to lib.attrNames unsupportedVersionsItems (remove
the special-case if pgVersion == "17"). Update the variables
unsupportedVersionsItems and unsupportedVersions accordingly so all package
entries that don't support the current pgVersion are detected.
🧹 Nitpick comments (5)
nix/ext/tests/pgmq.nix (2)
48-48: Avoid duplicatingpnamein the test name.Using
name = pnameavoids drift if the string changes later.♻️ Proposed diff
- name = "pgmq"; + name = pname;
182-205: Consider validating functionality after each pg_upgrade iteration.Right now the loop only checks versioning; adding the same after-create SQL + pg_regress step you already use elsewhere would catch runtime regressions for older versions.
🧪 Proposed diff
else: # Otherwise, the version should match the version from postgresql 15 test.assert_version_matches(version) + + test.run_sql_file("${../../../ansible/files/postgresql_extension_custom_scripts/pgmq/after-create.sql}") + test.check_pg_regress(Path("${psql_17}/lib/pgxs/src/test/regress/pg_regress"), "17", pg_regress_test_name)nix/ext/pg_graphql/default.nix (1)
132-180: Guard SQL-only packaging against accidental inclusion of update scripts.
buildUnsupportedconcatenates everysql/*.sql. If upstream includes versioned update scripts, they’d be folded into the base install file, which can skew pg_upgrade results. Consider preferring a prebuiltpg_graphql--${version}.sqlwhen present and otherwise concatenating a deterministic, filtered list. Please verify against the upstreamsql/layout.🧩 Proposed diff
- installPhase = '' - mkdir -p $out/share/postgresql/extension - for file in $src/sql/*.sql; do - filename=$(basename "$file") - if [[ "$filename" != "load_sql_config.sql" && "$filename" != "load_sql_context.sql" ]]; then - cat "$file" - echo ";" - fi - done > $out/share/postgresql/extension/${pname}--${version}.sql - ''; + installPhase = '' + mkdir -p $out/share/postgresql/extension + if [[ -f "$src/sql/${pname}--${version}.sql" ]]; then + cp "$src/sql/${pname}--${version}.sql" \ + $out/share/postgresql/extension/${pname}--${version}.sql + else + for file in $(ls $src/sql/*.sql | sort -V); do + filename=$(basename "$file") + if [[ "$filename" != "load_sql_config.sql" \ + && "$filename" != "load_sql_context.sql" \ + && "$filename" != *"--"* ]]; then + cat "$file" + echo ";" + fi + done > $out/share/postgresql/extension/${pname}--${version}.sql + fi + '';nix/ext/pg_jsonschema/default.nix (2)
153-155: Confirm which SQL version should seed unsupported versions.
lib.head versionscopies the oldest supported SQL into every unsupported version. If the goal is to align with the most recent supported schema, considerlib.last versionsor a version-specific mapping. Please confirm the intended behavior.♻️ Alternative if “latest” is intended
- for v in ${lib.concatStringsSep " " unsupportedVersions}; do - cp $out/share/postgresql/extension/${pname}--${lib.head versions}.sql $out/share/postgresql/extension/${pname}--$v.sql - done + for v in ${lib.concatStringsSep " " unsupportedVersions}; do + cp $out/share/postgresql/extension/${pname}--${lib.last versions}.sql $out/share/postgresql/extension/${pname}--$v.sql + done
95-97: Consider dropping the debugfind $out.It’s noisy and slows logs without affecting outputs.
🧹 Proposed cleanup
- postInstall = '' - find $out + postInstall = ''
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/pg_jsonschema/default.nix`:
- Line 96: Remove the leftover debug command `find $out` from the build script
so it no longer prints the entire output tree during builds; locate the `find
$out` invocation (the standalone shell command) in the nix expression and delete
that line (or guard it behind a debug flag if you need it for local debugging)
so build logs are no longer cluttered.
github-merge-queue
bot
removed this pull request from the merge queue due to a conflict with the base branch
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
e4e86f8 to
dc1c6d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/pg_graphql/default.nix`:
- Around line 163-175: The branching that builds unsupportedPackages compares
pgVersion (a string) to the number 15, so the check never matches; update the
condition in the unsupportedPackages assignment to compare strings (e.g., use if
pgVersion == "15" then ...) or otherwise coerce pgVersion to an integer before
comparing; adjust the existing conditional around unsupportedPackages (which
references pgVersion, unsupportedVersions, and buildUnsupported) so the PG15
branch is correctly taken when pgVersion is the string "15".
In `@nix/ext/tests/pgmq.nix`:
- Around line 182-206: The pg_upgrade subtest loop currently validates versions
with test.assert_version_matches(...) but never runs the actual regression
tests; add a call to test.check_pg_regress() after the upgrade handling (i.e.,
after the branch that runs /var/lib/postgresql/update_extensions.sql and after
the test.assert_version_matches(...) calls) so each upgraded state is validated;
locate the subtest "Test pg_upgrade from postgresql 15 to 17 with older
extension version" and insert test.check_pg_regress() once per iteration after
the version assertions.
🧹 Nitpick comments (3)
nix/ext/tests/pgsodium.nix (1)
50-51: Inconsistent system attribute access.This file uses
pkgs.systemwhile other test files (pgroonga.nix,vault.nix,pgmq.nix,default.nix) usepkgs.stdenv.hostPlatform.system. While functionally equivalent in most scenarios,pkgs.stdenv.hostPlatform.systemis the more explicit pattern and handles cross-compilation correctly.♻️ Suggested fix for consistency
- psql_15 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_15; - psql_17 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_17; + psql_15 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_15; + psql_17 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_17;nix/ext/tests/default.nix (1)
247-272: Consider extracting the pg_upgrade test logic into a reusable method.The pg_upgrade test block is duplicated across 5 test files (
pgroonga.nix,pgsodium.nix,pgmq.nix,vault.nix, anddefault.nix) with only minor variations. Consider adding a method likecheck_pg_upgrade_path(pg15_config, pg17_config, ...)tolib.pyto reduce duplication and ensure consistency.nix/ext/pg_jsonschema/default.nix (1)
154-156: Consider using version-specific SQL content for unsupported versions.This approach copies the same SQL file (from the first supported version) for all unsupported versions. This differs from pg_graphql's
buildUnsupportedapproach which fetches the actual source for each unsupported version.While this works for pg_upgrade to recognize the extension exists, the migration scripts generated by
create_sql_files(e.g.,0.1.0--0.1.1.sql) would contain identical/incorrect content since the base files are copies.If accurate migration paths are needed beyond testing, consider adopting the
buildUnsupportedpattern from pg_graphql that fetches actual SQL content for each version.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/pg_graphql/default.nix`:
- Around line 156-161: Fix the typo in the package metadata by updating the
meta.description value in the meta attribute set (symbol: meta and key:
description) to spell "PostgreSQL" correctly instead of "PostreSQL"; locate the
description string in the meta block (the description = "GraphQL support for
PostreSQL"; line) and change it to "GraphQL support for PostgreSQL".
| meta = with lib; { | ||
| description = "GraphQL support for PostreSQL"; | ||
| homepage = "https://github.com/supabase/${pname}"; | ||
| license = licenses.postgresql; | ||
| inherit (postgresql.meta) platforms; | ||
| }; |
There was a problem hiding this comment.
Fix typo in meta description.
"PostreSQL" should be "PostgreSQL".
Suggested fix
meta = with lib; {
- description = "GraphQL support for PostreSQL";
+ description = "GraphQL support for PostgreSQL";
homepage = "https://github.com/supabase/${pname}";
license = licenses.postgresql;
inherit (postgresql.meta) platforms;
};🤖 Prompt for AI Agents
In `@nix/ext/pg_graphql/default.nix` around lines 156 - 161, Fix the typo in the
package metadata by updating the meta.description value in the meta attribute
set (symbol: meta and key: description) to spell "PostgreSQL" correctly instead
of "PostreSQL"; locate the description string in the meta block (the description
= "GraphQL support for PostreSQL"; line) and change it to "GraphQL support for
PostgreSQL".
There was a problem hiding this comment.
Funny typo, I suggest we fix it in another PR:
postgres on git develop
> rg Postre
README.md
201:| [pg_graphql](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | [1.5.11](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | GraphQL support for PostreSQL |
235:| [pg_graphql](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | [1.5.11](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | GraphQL support for PostreSQL |
268:| [pg_graphql](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | [1.5.11](https://github.com/supabase/pg_graphql/archive/v1.5.11.tar.gz) | GraphQL support for PostreSQL |
nix/ext/wrappers/default.nix
161: description = "Various Foreign Data Wrappers (FDWs) for PostreSQL";
nix/ext/pg_graphql/default.nix
99: description = "GraphQL support for PostreSQL";
There was a problem hiding this comment.
@yvan-sraka, sounds good! That makes sense to address the typo across all files in a dedicated PR since it appears in multiple places.
Would you like me to create a GitHub issue to track this typo fix? I can open one that lists all 5 occurrences across the codebase (README.md, nix/ext/wrappers/default.nix, and nix/ext/pg_graphql/default.nix) to make it easier to address them all at once.
There was a problem hiding this comment.
If you can open a new PR that fix it, that would be even better ;)
There was a problem hiding this comment.
@yvan-sraka, I'll create a PR to fix the typo across all files! Let me plan those changes.
Task: Fix typo "PostreSQL" → "PostgreSQL" across codebase
- Fix 3 occurrences in README.md
- Fix 1 occurrence in nix/ext/wrappers/default.nix
- Fix 1 occurrence in nix/ext/pg_graphql/default.nix
- Create PR with title "fix: correct typo PostreSQL → PostgreSQL"
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
66d2f75 to
ff3e694
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/pg_jsonschema/default.nix`:
- Around line 154-156: The loop that copies SQL for unsupportedVersions assumes
every unsupported version is older than lib.head versions (oldest supported)
which is fragile; update the logic in the for loop handling unsupportedVersions
to either (A) assert at runtime that every v in unsupportedVersions is <=
lib.head versions (e.g. fail early if any unsupported version is newer than
lib.head versions) or (B) generate/build proper SQL fixtures for each
unsupported version instead of copying ${lib.head versions}.sql; locate the loop
using the symbols unsupportedVersions, versions, lib.head and pname and
implement one of these fixes and include a clear error message if you choose the
assertion approach.
In `@nix/ext/tests/pgroonga.nix`:
- Around line 196-221: After each call that switches PostgreSQL configuration
via f"{...}/bin/switch-to-configuration test" (the two occurrences before
calling test.drop_extension()/test.install_extension() and before checking for
update_extensions.sql), add an explicit
server.wait_for_unit("postgresql.service") to ensure the server is fully ready
to accept connections before invoking test.drop_extension(),
test.install_extension(), or running the update script; this mirrors the initial
setup wait and prevents flaky failures due to the database not being ready
immediately after switch-to-configuration.
🧹 Nitpick comments (1)
nix/ext/pg_graphql/default.nix (1)
132-155: Consider honoring explicit SQL load order when assembling SQL-only artifacts.Concatenating
sql/*.sqlin glob order can be brittle if the repo relies on an explicit load order or inter-file dependencies. If asql/load-order.txtexists, using it (with a fallback to a glob) makes the output deterministic and safer.♻️ Possible adjustment to respect load-order.txt when present
- installPhase = '' - mkdir -p $out/share/postgresql/extension - for file in $src/sql/*.sql; do - filename=$(basename "$file") - if [[ "$filename" != "load_sql_config.sql" && "$filename" != "load_sql_context.sql" ]]; then - cat "$file" - echo ";" - fi - done > $out/share/postgresql/extension/${pname}--${version}.sql - ''; + installPhase = '' + mkdir -p $out/share/postgresql/extension + { + if [[ -f "$src/sql/load-order.txt" ]]; then + while IFS= read -r fname; do + file="$src/sql/$fname" + filename=$(basename "$file") + if [[ "$filename" != "load_sql_config.sql" && "$filename" != "load_sql_context.sql" ]]; then + cat "$file" + echo ";" + fi + done < "$src/sql/load-order.txt" + else + shopt -s nullglob + for file in "$src"/sql/*.sql; do + filename=$(basename "$file") + if [[ "$filename" != "load_sql_config.sql" && "$filename" != "load_sql_context.sql" ]]; then + cat "$file" + echo ";" + fi + done + fi + } > $out/share/postgresql/extension/${pname}--${version}.sql + '';
| for v in ${lib.concatStringsSep " " unsupportedVersions}; do | ||
| cp $out/share/postgresql/extension/${pname}--${lib.head versions}.sql $out/share/postgresql/extension/${pname}--$v.sql | ||
| done |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# First, check if versions.json exists and examine it
echo "=== Checking versions.json ==="
if [ -f "nix/ext/versions.json" ]; then
head -50 nix/ext/versions.json
else
find . -name "versions.json" -type f 2>/dev/null | head -5
fiRepository: supabase/postgres
Length of output: 1070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
pg_version="${1:-17}"
python - "$pg_version" <<'PY'
import json, sys
pg = sys.argv[1]
with open("nix/ext/versions.json") as f:
data = json.load(f)["pg_jsonschema"]
def vkey(v):
return tuple(int(x) for x in v.split("."))
supported = sorted([v for v, m in data.items() if pg in m["postgresql"]], key=vkey)
unsupported = sorted([v for v, m in data.items() if pg not in m["postgresql"]], key=vkey)
print("pg_version:", pg)
print("supported:", supported)
print("unsupported:", unsupported)
print("oldest_supported:", supported[0] if supported else None)
print("unsupported_newer_than_oldest_supported:", [v for v in unsupported if supported and vkey(v) > vkey(supported[0])])
PYRepository: supabase/postgres
Length of output: 214
🏁 Script executed:
#!/bin/bash
# Examine the default.nix file around lines 154-156
echo "=== Checking nix/ext/pg_jsonschema/default.nix ==="
if [ -f "nix/ext/pg_jsonschema/default.nix" ]; then
sed -n '140,170p' nix/ext/pg_jsonschema/default.nix | cat -n
else
find . -name "default.nix" -type f 2>/dev/null | grep pg_jsonschema | head -1
fiRepository: supabase/postgres
Length of output: 1355
🏁 Script executed:
#!/bin/bash
# Get more context from default.nix
echo "=== Full context of pg_jsonschema/default.nix ==="
wc -l nix/ext/pg_jsonschema/default.nix
echo ""
echo "=== Looking for versions and unsupportedVersions definitions ==="
rg -n "versions\s*=" nix/ext/pg_jsonschema/default.nix | head -10
rg -n "unsupportedVersions\s*=" nix/ext/pg_jsonschema/default.nix | head -10Repository: supabase/postgres
Length of output: 350
🏁 Script executed:
#!/bin/bash
# Show the entire file to understand context
cat nix/ext/pg_jsonschema/default.nixRepository: supabase/postgres
Length of output: 6149
🏁 Script executed:
#!/bin/bash
# Check pg_graphql for comparison as mentioned in review
echo "=== Checking pg_graphql default.nix for comparison ==="
if [ -f "nix/ext/pg_graphql/default.nix" ]; then
# Look for similar version handling
rg -n -C 3 "unsupportedVersions|lib.head" nix/ext/pg_graphql/default.nix | head -30
fiRepository: supabase/postgres
Length of output: 557
Code assumes unsupported versions are always older than the oldest supported version.
Currently this is true (for PG 17: unsupported are 0.1.0–0.3.1, oldest supported is 0.3.3), but the code makes a fragile assumption. If future versions break this invariant, copying ${lib.head versions}.sql to newer unsupported versions would use incorrect schema. Consider either:
- Adding a runtime assertion that no unsupported version is newer than oldest supported, or
- Following pg_graphql's approach and building SQL files for unsupported versions
If this is strictly for pg_upgrade compatibility with older schemas, the current approach is acceptable but should be documented.
🤖 Prompt for AI Agents
In `@nix/ext/pg_jsonschema/default.nix` around lines 154 - 156, The loop that
copies SQL for unsupportedVersions assumes every unsupported version is older
than lib.head versions (oldest supported) which is fragile; update the logic in
the for loop handling unsupportedVersions to either (A) assert at runtime that
every v in unsupportedVersions is <= lib.head versions (e.g. fail early if any
unsupported version is newer than lib.head versions) or (B) generate/build
proper SQL fixtures for each unsupported version instead of copying ${lib.head
versions}.sql; locate the loop using the symbols unsupportedVersions, versions,
lib.head and pname and implement one of these fixes and include a clear error
message if you choose the assertion approach.
| with subtest("Test pg_upgrade from postgresql 15 to 17 with older extension version"): | ||
| # Test that all extension versions from postgresql 15 can be upgraded to postgresql 17 using pg_upgrade | ||
| for version in versions["15"]: | ||
| server.systemctl("stop postgresql.service") | ||
| server.succeed("rm -fr /var/lib/postgresql/update_extensions.sql /var/lib/postgresql/17") | ||
| server.succeed( | ||
| f"{pg15_configuration}/bin/switch-to-configuration test >&2" | ||
| ) | ||
| test.drop_extension() | ||
| test.install_extension(version) | ||
| server.succeed( | ||
| f"{pg17_configuration}/bin/switch-to-configuration test >&2" | ||
| ) | ||
| has_update_script = server.succeed( | ||
| "test -f /var/lib/postgresql/update_extensions.sql && echo 'yes' || echo 'no'" | ||
| ).strip() == "yes" | ||
| if has_update_script: | ||
| # Run the extension update script generated during the upgrade | ||
| test.run_sql_file("/var/lib/postgresql/update_extensions.sql") | ||
| # If there was an update script, the last version should be installed | ||
| test.assert_version_matches(versions["17"][-1]) | ||
| else: | ||
| # Otherwise, the version should match the version from postgresql 15 | ||
| test.assert_version_matches(version) | ||
|
|
||
| test.check_pg_regress(Path("${psql_17}/lib/pgxs/src/test/regress/pg_regress"), "17", pg_regress_test_name) |
There was a problem hiding this comment.
Consider adding a wait for PostgreSQL readiness after configuration switches.
After switching configurations, the test immediately calls test.drop_extension() and test.install_extension(). While switch-to-configuration test is synchronous, PostgreSQL may not be ready to accept connections immediately after the service unit reaches "active" state.
Compare with the initial test setup (lines 154-155) which explicitly waits:
server.wait_for_unit("postgresql.service")This could cause flaky test failures when the database isn't ready before the first SQL operation.
Proposed fix
server.succeed(
f"{pg15_configuration}/bin/switch-to-configuration test >&2"
)
+ server.wait_for_unit("postgresql.service")
test.drop_extension()
test.install_extension(version)
server.succeed(
f"{pg17_configuration}/bin/switch-to-configuration test >&2"
)
+ server.wait_for_unit("postgresql.service")
has_update_script = server.succeed(🤖 Prompt for AI Agents
In `@nix/ext/tests/pgroonga.nix` around lines 196 - 221, After each call that
switches PostgreSQL configuration via f"{...}/bin/switch-to-configuration test"
(the two occurrences before calling
test.drop_extension()/test.install_extension() and before checking for
update_extensions.sql), add an explicit
server.wait_for_unit("postgresql.service") to ensure the server is fully ready
to accept connections before invoking test.drop_extension(),
test.install_extension(), or running the update script; this mirrors the initial
setup wait and prevents flaky failures due to the database not being ready
immediately after switch-to-configuration.
Add test to verify that all extension versions from PostgreSQL 15 can successfully upgrade to PostgreSQL 17 using pg_upgrade. The test now validates: - Each PG 15 extension version can be upgraded to PG 17 - Extension update scripts are properly generated during upgrade - Version handling works correctly with and without update scripts - Final extension versions match expected values after upgrade
…ade compatibility When upgrading PostgreSQL versions, pg_upgrade needs access to old extension versions sql to migrate from. This adds unsupported pg_graphql versions (those not compilable with current PostgreSQL) as SQL-only packages, ensuring migration paths exist when upgrading from older PostgreSQL versions.
…pgrade compatibility When upgrading PostgreSQL versions, pg_upgrade needs access to old extension versions sql to migrate from. This adds unsupported pg_graphql versions (those not compilable with current PostgreSQL) as SQL-only packages, ensuring migration paths exist when upgrading from older PostgreSQL versions.
When upgrading from PostgreSQL 15 to 17, pg_stat_monitor version 1.0 (PG 15-only) cannot be migrated as it uses .sql.in template files that reference MODULE_PATHNAME without proper processing for the target version. This marks version 1.0 as not pg_upgrade compatible and filters it from the version test list, allowing the test to use version 2.1 (which supports both PG 15 and 17) for pg_upgrade validation instead. Version 1.0 remains available for PG 15 installations. Version 2.1 has different schemas on PostgreSQL 15 vs 17 despite sharing the same version number. On PG 15 it uses the older schema with blk_read_time and blk_write_time columns, while on PG 17 it uses a newer schema with shared_blk_read_time, shared_blk_write_time, local_blk_read_time, local_blk_write_time and additional JIT statistics columns. During pg_upgrade from PG 15 to 17, the extension version remains 2.1 without schema migration since no update script is generated. Fresh installations on PG 17 receive the new schema while pg_upgrade retains the old schema, creating a test conflict as both scenarios share the same expected output file. A custom test implementation skips pg_regress validation after pg_upgrade when no update script is generated, since the schema mismatch is expected behavior. This maintains full test coverage for fresh installations through the regular psql_17 check while validating extension version compatibility for pg_upgrade scenarios.
…ib_name parameters
…de compatibility Remove pg_stat_monitor from default extension test list to prevent conflict with the custom pg_stat_monitor.nix test file. The custom test includes special logic to skip pg_regress after pg_upgrade when no update script is generated, which is necessary because version 2.1 has different schemas between PostgreSQL 15 and 17 despite sharing the same version number.
The test was failing because the pg_regress test expects the pgmq extension to be in a fully functional state, but after pg_upgrade the after-create script wasn't being run. This script sets up necessary pgmq configurations that the tests depend on.
pgVersion is a string, so comparison should be string-to-string instead of string-to-integer.
When extensions define shared_preload_libraries as a list (e.g., pg_cron, pg_net), the test code needs to convert it to a comma-separated string for use in pg_upgrade --old-options and --new-options flags. This fix adds proper list-to-string conversion using lib.concatStringsSep.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
ff3e694 to
413e268
Compare
…nputs.nixpkgs.lib.nixos.runTest The pg_stat_monitor test was using self.inputs.nixpkgs.lib.nixos.runTest with hostPkgs parameter, which causes cross-compilation issues on Darwin when building Linux VMs. This change aligns it with all other extension tests by using pkgs.testers.runNixOSTest, which properly handles cross-platform builds.
…esqlWithExtension The pgsodium test uses pkgs.pkgsLinux.buildEnv to build Linux packages, but psql_15 and psql_17 were referencing pkgs.system which could be Darwin. This caused mixed Darwin/Linux binaries in the test environment, leading to "cannot execute binary file" errors on Darwin when trying to run pg_regress. Now consistently uses pkgs.pkgsLinux for all package references.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
d9fde41 to
f47ecd2
Compare
… references The pg_stat_monitor test was mixing pkgs.system and pkgs.stdenv.hostPlatform.system which resolved to Darwin packages on macOS, but the test requires Linux packages for NixOS VMs. Changed to consistently use pkgs.pkgsLinux.stdenv.hostPlatform.system and pkgs.pkgsLinux.buildEnv/makeWrapper throughout, matching the pattern used in pgmq.nix and pgsodium.nix.
yvan-sraka
force-pushed
the
feat/test-pg-upgrade-older-version
branch
from
ea70bdf to
cd83445
Compare
3 participants
Add test to verify that all extension versions from PostgreSQL 15 can successfully upgrade to PostgreSQL 17 using pg_upgrade.
The test now validates:
Summary by CodeRabbit
New Features
Tests
Bug Fixes