feat: test extensions with OrioleDB #1889
Conversation
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
72f463c to
3fb7c96
Compare
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughAdds OrioleDB-17 variants and wiring across Nix test harnesses: new public binding and specialisation for orioledb-17, adjusted packaging and majorVersion resolution, OrioleDB-specific init/migrate/systemd handling, extended http/default test scripts, and a new index_advisor NixOS runTest exercising 15 → 17 → orioledb-17 upgrade and tests. (39 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Tester as Test harness
participant VM as NixOS VM
participant Systemd as systemd
participant Postgres as postgresql service
participant HTTPMock as http-mock-server
participant Psql as psql client
Tester->>VM: start runTest (http / index_advisor)
VM->>Systemd: start required unit files and services
Systemd->>Postgres: start configured postgres instance (15 / 17 / orioledb-17)
Systemd->>HTTPMock: start http-mock-server
Tester->>Psql: run init scripts, create extension, run pg_regress
Psql->>Postgres: execute SQL for tests
alt Upgrade path
Tester->>Systemd: trigger migrate unit (reinit datadir or switch config)
Systemd->>Postgres: stop old, start new postgres variant
Tester->>Psql: validate extension presence and run regress
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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/tests/http.nix`:
- Around line 275-280: After switching to OrioleDB in the subtest that runs
server.succeed("${orioledb17-configuration}/bin/switch-to-configuration test
>&2"), call test.create_schema() before running the extension query;
specifically, add a call to test.create_schema() after the switch and before
retrieving installed_extensions (the variable set by test.run_sql("""SELECT
extname FROM pg_extension WHERE extname = 'orioledb';""")), so the schema is
recreated in the reinitialized data directory and the subsequent assertion that
"orioledb" is in installed_extensions will be valid.
In `@nix/ext/tests/index_advisor.nix`:
- Around line 210-211: PostgresExtensionTest is instantiated with only four
arguments in index_advisor.nix causing implicit defaults (support_upgrade=True,
schema="public", lib_name=None) and missing the explicit create_schema() call
used elsewhere; make the usage consistent by either passing all seven parameters
to PostgresExtensionTest (including explicit support_upgrade, schema, lib_name)
to mirror default.nix, or keep four parameters but add an explicit call to
create_schema() on the returned instance (the object from PostgresExtensionTest)
and document why defaults are acceptable; reference the PostgresExtensionTest
constructor and the create_schema() method when applying the change.
🧹 Nitpick comments (7)
nix/ext/tests/index_advisor.nix (2)
166-175: Consider adding a safety check beforerm -rf.The
rm -rfoperation deletes the data directory without verifying thatnewPostgresql.psqlSchemais non-empty. While this is intentional for reinitializing OrioleDB, an empty or unexpected value could cause unintended deletion.♻️ Proposed safety check
'' set -x systemctl cat postgresql.service + if [[ -z "${newPostgresql.psqlSchema}" ]]; then + echo "Error: psqlSchema is empty, refusing to rm -rf" + exit 1 + fi rm -rf ${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema} '';
259-262: FIXME acknowledged - pg_regress tests failing with OrioleDB.The commented-out pg_regress test for OrioleDB with the FIXME note is acceptable for tracking a known limitation. Consider creating an issue to track resolution.
Would you like me to open an issue to track fixing the pg_regress tests with OrioleDB?
nix/ext/tests/default.nix (3)
55-59: Inconsistent package system references.Line 55 uses
pkgs.stdenv.hostPlatform.system, line 56 usespkgs.system, and lines 57-59 usepkgs.stdenv.hostPlatform.system. For consistency and cross-compilation awareness, prefer using the same reference throughout.♻️ Proposed fix for consistency
psql_15 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_15; - psql_17 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_17; + psql_17 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_17; orioledb_17 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17;
148-162: Inconsistent system reference in orioledb17 specialisation.Line 151 uses
pkgs.systemwhile the rest of the file predominantly usespkgs.stdenv.hostPlatform.system. Apply the same reference consistently.♻️ Proposed fix
specialisation.orioledb17.configuration = { services.postgresql = { - package = lib.mkForce (postgresqlWithExtension self.packages.${pkgs.system}.postgresql_orioledb-17); + package = lib.mkForce (postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17); settings = lib.mkForce (
184-192: Add safety check beforerm -rfand fix inconsistent system reference.Same concerns as index_advisor.nix: the
rm -rflacks a guard against an emptypsqlSchema, and line 186 usespkgs.systeminconsistently.♻️ Proposed fix
script = let - newPostgresql = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_orioledb-17; + newPostgresql = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17; in '' set -x systemctl cat postgresql.service + if [[ -z "${newPostgresql.psqlSchema}" ]]; then + echo "Error: psqlSchema is empty, refusing to rm -rf" + exit 1 + fi rm -rf ${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema} '';nix/ext/tests/http.nix (2)
47-49: Inconsistent package system references.Lines 47-49 use
pkgs.system, while line 72 and lines 106, 122-127 usepkgs.stdenv.hostPlatform.system. For consistency across the file, use the same reference.♻️ Proposed fix for consistency
- psql_15 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_15; - psql_17 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_17; - orioledb_17 = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_orioledb-17; + psql_15 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_15; + psql_17 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_17; + orioledb_17 = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17;
188-196: Add safety check beforerm -rfand fix inconsistent system reference.Line 190 uses
pkgs.systemwhile most of the file usespkgs.stdenv.hostPlatform.system. Therm -rfalso lacks a guard.♻️ Proposed fix
script = let - newPostgresql = postgresqlWithExtension self.packages.${pkgs.system}.postgresql_orioledb-17; + newPostgresql = postgresqlWithExtension self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17; in '' set -x systemctl cat postgresql.service + if [[ -z "${newPostgresql.psqlSchema}" ]]; then + echo "Error: psqlSchema is empty, refusing to rm -rf" + exit 1 + fi rm -rf ${builtins.dirOf config.services.postgresql.dataDir}/${newPostgresql.psqlSchema} '';
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
3fb7c96 to
6adac53
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/tests/index_advisor.nix`:
- Around line 259-267: After switching to OrioleDB in the "switch to orioledb
17" subtest (the block invoking
"${orioledb17-configuration}/bin/switch-to-configuration test >&2" and checking
pg_extension), add a call to test.create_schema() immediately after the switch
and before running extension assertions so the schema is recreated after the
data directory reinitialization; this mirrors the behavior in default.nix and
http.nix and ensures the subsequent installed_extensions check and
test.check_upgrade_path("orioledb-17") operate against a proper schema.
🧹 Nitpick comments (3)
nix/ext/tests/index_advisor.nix (2)
269-271: Consider tracking this FIXME for pg_regress with orioledb.The commented-out pg_regress test indicates a known issue. Consider creating a tracking issue to ensure this gets resolved.
Would you like me to help create an issue to track enabling pg_regress tests for index_advisor with orioledb?
45-47: Inconsistent platform reference style.The file uses
pkgs.systemin several places (lines 7, 22, 45-47) butpkgs.stdenv.hostPlatform.systemat line 170. While these are typically equivalent, consider using a consistent style throughout for clarity. The other test files (default.nix,http.nix) usepkgs.stdenv.hostPlatform.systemconsistently.♻️ Proposed fix for consistency
- newPostgresql = - postgresqlWithExtension - self.packages.${pkgs.stdenv.hostPlatform.system}.postgresql_orioledb-17; + newPostgresql = + postgresqlWithExtension + self.packages.${pkgs.system}.postgresql_orioledb-17;Or alternatively, update lines 7, 22, 45-47 to use
pkgs.stdenv.hostPlatform.systemfor consistency with other test files.Also applies to: 170-170
nix/ext/tests/http.nix (1)
244-244: DifferentPostgresExtensionTestinstantiation pattern.This file instantiates
PostgresExtensionTestwith 4 parameters, relying on defaults forsupport_upgrade,schema, andlib_name:test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory)While
default.nix(line 241) andindex_advisor.nix(line 219) explicitly provide all 7 parameters. This may be intentional if http extension tests work fine with defaults, but consider aligning for consistency if the defaults aren't specifically required.
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
6adac53 to
14f608a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/tests/index_advisor.nix`:
- Around line 232-234: The test uses pname when building the background-worker
.so path which fails if the shared library name differs; change the lookup to
use lib_name instead of pname in the test invocation so the .so path is
Path("${psql_15}/lib/${lib_name}.so"); update the call to
test.check_switch_extension_with_background_worker to pass that corrected Path
(reference ext_has_background_worker, subtest and
test.check_switch_extension_with_background_worker to locate the code).
🧹 Nitpick comments (1)
nix/ext/tests/http.nix (1)
222-259: AlignPostgresExtensionTestinputs with extension metadata.For consistency with other harnesses and to avoid mismatches if schema/lib names differ, consider passing
ext_schema/lib_name, creating the schema upfront, and usinglib_namefor the background-worker path.♻️ Suggested alignment
extension_name = "${pname}" + support_upgrade = True + ext_schema = "${(installedExtension "15").defaultSchema or "public"}" + lib_name = "${(installedExtension "15").libName or pname}" ext_has_background_worker = ${ if (installedExtension "15") ? hasBackgroundWorker then "True" else "False" } sql_test_directory = Path("${../../tests}") pg_regress_test_name = "${(installedExtension "15").pgRegressTestName or pname}" ${builtins.readFile ./lib.py} start_all() server.wait_for_unit("multi-user.target") server.wait_for_unit("postgresql.service") - test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory) + test = PostgresExtensionTest(server, extension_name, versions, sql_test_directory, support_upgrade, ext_schema, lib_name) + test.create_schema() if ext_has_background_worker: with subtest("Test switch_${pname}_version"): - test.check_switch_extension_with_background_worker(Path("${psql_15}/lib/${pname}.so"), "15") + test.check_switch_extension_with_background_worker(Path("${psql_15}/lib/${lib_name}.so"), "15")
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
14f608a to
b9db725
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@nix/ext/tests/index_advisor.nix`:
- Around line 270-272: Replace the FIXME comment by either (a) adding a tracking
issue link or (b) adding a temporary skip that documents the rationale;
specifically update the commented-out subtest block referencing subtest("Check
pg_regress with orioledb 17 after installing the last version") and the call to
test.check_pg_regress(Path("${orioledb_17}/lib/pgxs/src/test/regress/pg_regress"),
"orioledb-17", pg_regress_test_name) so it either logs a URL/issue ID (e.g.
“Tracking: <issue-url>”) or wraps the subtest in a skip with a clear reason
(e.g. skip="ortoiledb pg_regress failing on CI - see <issue-id>") so the missing
coverage remains visible and traceable.
🧹 Nitpick comments (1)
nix/ext/tests/default.nix (1)
154-163: Consider usingorioledb-17defaultSettings in the OrioleDB specialization.Right now the OrioleDB config pulls defaults from
"17". If the OrioleDB-17 extension package ever diverges, those defaults won’t apply. Using"orioledb-17"keeps the specialization aligned with the actual variant (or document that they’re guaranteed identical).♻️ Suggested adjustment
- settings = lib.mkForce ( - ((installedExtension "17").defaultSettings or { }) + settings = lib.mkForce ( + ((installedExtension "orioledb-17").defaultSettings or { }) // { jit = "off"; shared_preload_libraries = [ "orioledb" ] - ++ (lib.toList ((installedExtension "17").defaultSettings.shared_preload_libraries or [ ])); + ++ (lib.toList ((installedExtension "orioledb-17").defaultSettings.shared_preload_libraries or [ ])); default_table_access_method = "orioledb"; } );
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
b9db725 to
c2fa76f
Compare
|
Don't forget to add the test on OrioleDB on other extension once we have merged #1906 |
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
c2fa76f to
a3dffe5
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/tests/default.nix`:
- Around line 154-166: The code uses installedExtension "orioledb-17" in the
settings block; change both occurrences to installedExtension "17" so the lookup
uses the PostgreSQL version key (matching http.nix/index_advisor.nix). Update
the two places referencing (installedExtension "orioledb-17").defaultSettings
and (installedExtension "orioledb-17").defaultSettings.shared_preload_libraries
to use installedExtension "17" instead, keeping the rest of the lib.mkForce /
defaultSettings merging logic unchanged.
In `@nix/ext/tests/index_advisor.nix`:
- Around line 268-275: Replace the placeholder tracking URL
"https://github.com/supabase/postgres/issues/TBD" in the OrioleDB pg_regress
note: either create a real GitHub issue and update that URL to the new issue
link, or remove the URL entirely if you opt not to file an issue; ensure the
surrounding comment (the TODO and the commented subtest block referencing "Check
pg_regress with orioledb 17...") remains intact and only the placeholder URL or
its enclosing line is modified.
a3dffe5 to
7574614
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
7574614 to
8e464bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@nix/ext/tests/default.nix`:
- Around line 35-36: The lookup uses pkgs.system for the OrioleDB package while
the rest of the file uses pkgs.stdenv.hostPlatform.system, which can cause
cross-compilation mismatches; update the expression that builds
self.legacyPackages.${pkgs.system}.psql_orioledb-17.exts.orioledb to use
pkgs.stdenv.hostPlatform.system instead (i.e.
self.legacyPackages.${pkgs.stdenv.hostPlatform.system}.psql_orioledb-17.exts.orioledb)
so it matches the other package lookups and avoids host/target inconsistency.
In `@nix/ext/tests/http.nix`:
- Around line 72-93: Add the missing settings line to the PG15
services.postgresql block so it matches other test files: inside the same block
that references postgresql_15 (the services.postgresql configuration that
includes package = postgresqlWithExtension ... postgresql_15 and initialScript),
add a settings assignment (an empty object when no defaults are provided) named
settings so the block follows the same pattern used by default.nix and the other
tests.
🧹 Nitpick comments (1)
nix/ext/tests/default.nix (1)
302-327: Don’t gate OrioleDB upgrade path on run_pg_regress.
Right now the switch + upgrade checks run only whenrun_pg_regressis true, so extensions with regress disabled (e.g., pg_repack) skip the OrioleDB path entirely. Consider separating the switch/upgrade flow from the pg_regress-only checks.🛠️ Suggested restructuring
- ${ - if run_pg_regress then - '' - ... - with subtest("switch to orioledb 17"): - server.succeed( - "${orioledb17-configuration}/bin/switch-to-configuration test >&2" - ) - installed_extensions=test.run_sql("""SELECT extname FROM pg_extension WHERE extname = 'orioledb';""") - assert "orioledb" in installed_extensions - test.create_schema() - - with subtest("Check upgrade path with orioledb 17"): - test.check_upgrade_path("orioledb-17") - - with subtest("Check pg_regress with orioledb 17 after installing the last version"): - test.check_pg_regress(Path("${orioledb_17}/lib/pgxs/src/test/regress/pg_regress"), "orioledb-17", pg_regress_test_name) - '' - else - "" - } + ${ + if support_upgrade then + '' + with subtest("switch to orioledb 17"): + server.succeed( + "${orioledb17-configuration}/bin/switch-to-configuration test >&2" + ) + installed_extensions=test.run_sql("""SELECT extname FROM pg_extension WHERE extname = 'orioledb';""") + assert "orioledb" in installed_extensions + test.create_schema() + + with subtest("Check upgrade path with orioledb 17"): + test.check_upgrade_path("orioledb-17") + '' + else + "" + } + + ${ + if run_pg_regress then + '' + with subtest("Check pg_regress with orioledb 17 after installing the last version"): + test.check_pg_regress(Path("${orioledb_17}/lib/pgxs/src/test/regress/pg_regress"), "orioledb-17", pg_regress_test_name) + '' + else + "" + }
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@nix/ext/tests/default.nix`:
- Around line 34-36: Replace the incorrect list concatenation using '+' with the
Nix list concatenation operator '++' for the expression that adds the optional
OrioleDB extension: update the expression containing lib.optional,
postgresql.isOrioleDB, and
self.legacyPackages.${pkgs.stdenv.hostPlatform.system}.psql_orioledb-17.exts.orioledb
to use '++' to concatenate the list produced by lib.optional into the
surrounding list.
In `@nix/ext/tests/http.nix`:
- Around line 22-24: Replace the inconsistent platform lookup in the optional
OrioleDB extension expression: change the reference
self.legacyPackages.${pkgs.system}.psql_orioledb-17.exts.orioledb to use
pkgs.stdenv.hostPlatform.system (i.e.
self.legacyPackages.${pkgs.stdenv.hostPlatform.system}.psql_orioledb-17.exts.orioledb)
so it matches the other package lookups and avoids cross-compilation mismatches.
- Around line 152-165: In the specialisation.orioledb17.configuration block,
replace the use of pkgs.system in the package lookup (the expression
self.packages.${pkgs.system}.postgresql_orioledb-17) with
pkgs.stdenv.hostPlatform.system to match the migration script and ensure
consistent platform selection; keep the rest of the package selection and
settings logic unchanged so installedExtension "17" and the
settings/jit/shared_preload_libraries/default_table_access_method references
continue to work as before.
Make sure that pg_regress and upgrade paths work correctly when using OrioleDB
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
5d19cd7 to
d81c602
Compare
The index_advisor and http extension tests were missing PostgreSQL authentication configuration causing test failures with 'Peer authentication failed'. OrioleDB tests also failed because schemas were not recreated after database reinitialization. This addresses all PR review comments including proper background worker library path resolution, standardized test patterns, safety checks for data directory deletion, enhanced documentation for test limitations, and version-specific OrioleDB configuration. The changes ensure extension tests work consistently across PostgreSQL 15, 17, and OrioleDB variants with proper authentication, schema management, and cross-compilation support.
yvan-sraka
force-pushed
the
feat/ext-test-orioledb
branch
from
d81c602 to
d107e46
Compare
Make sure that pg_regress and upgrade paths work correctly when using OrioleDB
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.