test: support per-suite databases on external postgres#1931
Merged
Conversation
Benchstat (RLS)Base:
|
| Benchmark | Base | Head | Change | p-value |
|---|---|---|---|---|
RLS/Sample-15000/catalog_changes/Without_RLS-4 |
3.436m | 3.788m | +10.22% 🔴 | 0.002 |
RLS/Sample-15000/config_changes/Without_RLS-4 |
3.446m | 3.795m | +10.13% 🔴 | 0.002 |
RLS/Sample-15000/config_names/Without_RLS-4 |
9.146m | 9.631m | +5.30% 🔴 | 0.002 |
RLS/Sample-15000/config_summary/Without_RLS-4 |
43.60m | 45.42m | +4.19% | 0.002 |
RLS/Sample-15000/config_types/With_RLS-4 |
85.52m | 86.82m | +1.51% | 0.002 |
RLS/Sample-15000/config_detail/With_RLS-4 |
84.21m | 85.19m | +1.16% | 0.002 |
RLS/Sample-15000/config_types/Without_RLS-4 |
2.627m | 2.656m | +1.09% | 0.026 |
✅ 2 improvement(s)
| Benchmark | Base | Head | Change | p-value |
|---|---|---|---|---|
RLS/Sample-15000/configs/With_RLS-4 |
86.12m | 84.31m | -2.11% | 0.002 |
RLS/Sample-15000/catalog_changes/With_RLS-4 |
89.59m | 88.42m | -1.31% | 0.041 |
Failed: 3 benchmark(s) regressed by more than 5%:
RLS/Sample-15000/catalog_changes/Without_RLS-4: 3.436m -> 3.788m (+10.22%)
RLS/Sample-15000/config_changes/Without_RLS-4: 3.446m -> 3.795m (+10.13%)
RLS/Sample-15000/config_names/Without_RLS-4: 9.146m -> 9.631m (+5.30%)
Full benchstat output
goos: linux
goarch: amd64
pkg: github.com/flanksource/duty/bench
cpu: AMD EPYC 9V74 80-Core Processor
│ bench-base.txt │ bench-head.txt │
│ sec/op │ sec/op vs base │
RLS/Sample-15000/catalog_changes/Without_RLS-4 3.436m ± 1% 3.788m ± 9% +10.22% (p=0.002 n=6)
RLS/Sample-15000/catalog_changes/With_RLS-4 89.59m ± 2% 88.42m ± 0% -1.31% (p=0.041 n=6)
RLS/Sample-15000/config_changes/Without_RLS-4 3.446m ± 1% 3.795m ± 17% +10.13% (p=0.002 n=6)
RLS/Sample-15000/config_changes/With_RLS-4 88.40m ± 0% 88.67m ± 1% ~ (p=0.065 n=6)
RLS/Sample-15000/config_detail/Without_RLS-4 2.675m ± 1% 2.693m ± 2% ~ (p=0.394 n=6)
RLS/Sample-15000/config_detail/With_RLS-4 84.21m ± 0% 85.19m ± 0% +1.16% (p=0.002 n=6)
RLS/Sample-15000/config_names/Without_RLS-4 9.146m ± 4% 9.631m ± 1% +5.30% (p=0.002 n=6)
RLS/Sample-15000/config_names/With_RLS-4 84.84m ± 2% 85.34m ± 0% ~ (p=0.394 n=6)
RLS/Sample-15000/config_summary/Without_RLS-4 43.60m ± 1% 45.42m ± 2% +4.19% (p=0.002 n=6)
RLS/Sample-15000/config_summary/With_RLS-4 499.8m ± 1% 499.1m ± 2% ~ (p=0.937 n=6)
RLS/Sample-15000/configs/Without_RLS-4 5.042m ± 1% 5.604m ± 11% ~ (p=0.132 n=6)
RLS/Sample-15000/configs/With_RLS-4 86.12m ± 0% 84.31m ± 1% -2.11% (p=0.002 n=6)
RLS/Sample-15000/analysis_types/Without_RLS-4 2.626m ± 0% 2.619m ± 2% ~ (p=0.589 n=6)
RLS/Sample-15000/analysis_types/With_RLS-4 2.624m ± 3% 2.627m ± 1% ~ (p=0.818 n=6)
RLS/Sample-15000/analyzer_types/Without_RLS-4 2.497m ± 1% 2.499m ± 2% ~ (p=0.937 n=6)
RLS/Sample-15000/analyzer_types/With_RLS-4 2.516m ± 2% 2.504m ± 1% ~ (p=0.093 n=6)
RLS/Sample-15000/change_types/Without_RLS-4 3.451m ± 1% 3.432m ± 1% ~ (p=0.240 n=6)
RLS/Sample-15000/change_types/With_RLS-4 3.515m ± 1% 3.487m ± 3% ~ (p=0.310 n=6)
RLS/Sample-15000/config_classes/Without_RLS-4 2.194m ± 3% 2.191m ± 1% ~ (p=0.589 n=6)
RLS/Sample-15000/config_classes/With_RLS-4 85.13m ± 1% 85.05m ± 0% ~ (p=0.485 n=6)
RLS/Sample-15000/config_types/Without_RLS-4 2.627m ± 1% 2.656m ± 2% +1.09% (p=0.026 n=6)
RLS/Sample-15000/config_types/With_RLS-4 85.52m ± 0% 86.82m ± 1% +1.51% (p=0.002 n=6)
geomean 13.04m 13.27m +1.78%
Benchstat (Other)Base: ✅ 3 improvement(s)
Full benchstat output |
Contributor
WalkthroughThe test setup code now generates unique random database names (incorporating parallel-process ID, timestamp, and random bytes) instead of deterministic ones. Database operations use quoted identifiers and safer SQL syntax with Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Duty tests can use DUTY_DB_URL to run against an always-on Postgres server, but create mode reused predictable database names and rewrote URLs with string replacement. Generate a unique database name for each suite process, build per-database URLs with net/url, and quote database identifiers for create/drop operations. Embedded Postgres behavior remains unchanged.
moshloop
force-pushed
the
test/external-postgres-per-suite-db
branch
from
534fd2d to
05ce240
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/setup/common.go`:
- Around line 132-133: The error message currently returns the raw connection
string (variable "connection") which may contain credentials; change the error
to avoid leaking secrets by reporting only the scheme (u.Scheme) or a redacted
URL instead. Update the branch that checks u.Scheme in tests/setup/common.go so
the fmt.Errorf does not interpolate "connection" but instead uses u.Scheme (or a
function that redacts userinfo from the parsed URL) to produce the error text,
e.g. "unsupported postgres connection scheme: %s" or "unsupported postgres
connection URL (redacted): %s" using the sanitized value.
- Around line 229-245: The shutdown hook uses the package-global postgresDBUrl
which is later reassigned, so capture the admin URL and any DB-specific
identifiers into invocation-local variables (e.g., localPostgresDBUrl and
localQuotedDBName) before calling execPostgres and before registering
shutdown.AddHookWithPriority; update all execPostgres calls and the hook closure
to use these local variables (referencing postgresDBUrl, PgUrl, quoteIdentifier,
execPostgres, and shutdown.AddHookWithPriority) so each SetupDB invocation
cleans up its own database instead of the possibly-reassigned global.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f00ffc4-09dc-44c7-a4a0-e509f775b38b
📒 Files selected for processing (1)
tests/setup/common.go
Comment on lines
+132
to
+133
| if u.Scheme != "postgres" && u.Scheme != "postgresql" { | ||
| return "", fmt.Errorf("unsupported postgres connection string: %s", connection) |
Contributor
There was a problem hiding this comment.
Don’t include the raw DSN in this error.
connection can contain credentials from DUTY_DB_URL, so this bubbles secrets into test/CI logs on a bad scheme. Report the scheme (or a redacted URL) instead.
Suggested fix
if u.Scheme != "postgres" && u.Scheme != "postgresql" {
- return "", fmt.Errorf("unsupported postgres connection string: %s", connection)
+ return "", fmt.Errorf("unsupported postgres connection scheme %q", u.Scheme)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if u.Scheme != "postgres" && u.Scheme != "postgresql" { | |
| return "", fmt.Errorf("unsupported postgres connection string: %s", connection) | |
| if u.Scheme != "postgres" && u.Scheme != "postgresql" { | |
| return "", fmt.Errorf("unsupported postgres connection scheme %q", u.Scheme) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setup/common.go` around lines 132 - 133, The error message currently
returns the raw connection string (variable "connection") which may contain
credentials; change the error to avoid leaking secrets by reporting only the
scheme (u.Scheme) or a redacted URL instead. Update the branch that checks
u.Scheme in tests/setup/common.go so the fmt.Errorf does not interpolate
"connection" but instead uses u.Scheme (or a function that redacts userinfo from
the parsed URL) to produce the error text, e.g. "unsupported postgres connection
scheme: %s" or "unsupported postgres connection URL (redacted): %s" using the
sanitized value.
Comment on lines
229
to
+245
| postgresDBUrl = url | ||
| dbName = fmt.Sprintf("duty_gingko%d", port) | ||
| PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1) | ||
| _ = execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) | ||
| if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName); err != nil { | ||
| dbName = randomDatabaseName("duty_test") | ||
|
|
||
| var err error | ||
| PgUrl, err = databaseURL(url, dbName) | ||
| if err != nil { | ||
| return context.Context{}, err | ||
| } | ||
|
|
||
| quotedDBName := quoteIdentifier(dbName) | ||
| _ = execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)) | ||
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil { | ||
| return context.Context{}, fmt.Errorf("cannot create %s: %v", dbName, err) | ||
| } | ||
|
|
||
| shutdown.AddHookWithPriority("remove postgres db", shutdown.PriorityCritical, func() { | ||
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", dbName)); err != nil { | ||
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)); err != nil { |
Contributor
There was a problem hiding this comment.
Capture the admin URL locally before registering cleanup.
The shutdown hook reads the package-global postgresDBUrl, which gets reassigned on every SetupDB call. If this runs more than once in a process, earlier hooks can clean up against the wrong Postgres instance. Use an invocation-local variable for the create/drop path and the hook.
Suggested fix
- postgresDBUrl = url
+ adminURL := url
+ postgresDBUrl = adminURL
dbName = randomDatabaseName("duty_test")
var err error
PgUrl, err = databaseURL(url, dbName)
if err != nil {
return context.Context{}, err
}
quotedDBName := quoteIdentifier(dbName)
- _ = execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName))
- if err := execPostgres(postgresDBUrl, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil {
+ _ = execPostgres(adminURL, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName))
+ if err := execPostgres(adminURL, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil {
return context.Context{}, fmt.Errorf("cannot create %s: %v", dbName, err)
}
shutdown.AddHookWithPriority("remove postgres db", shutdown.PriorityCritical, func() {
- if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)); err != nil {
+ if err := execPostgres(adminURL, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)); err != nil {
logger.Errorf("execPostgres: %v", err)
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| postgresDBUrl = url | |
| dbName = fmt.Sprintf("duty_gingko%d", port) | |
| PgUrl = strings.Replace(url, "/postgres", "/"+dbName, 1) | |
| _ = execPostgres(postgresDBUrl, "DROP DATABASE "+dbName) | |
| if err := execPostgres(postgresDBUrl, "CREATE DATABASE "+dbName); err != nil { | |
| dbName = randomDatabaseName("duty_test") | |
| var err error | |
| PgUrl, err = databaseURL(url, dbName) | |
| if err != nil { | |
| return context.Context{}, err | |
| } | |
| quotedDBName := quoteIdentifier(dbName) | |
| _ = execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)) | |
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil { | |
| return context.Context{}, fmt.Errorf("cannot create %s: %v", dbName, err) | |
| } | |
| shutdown.AddHookWithPriority("remove postgres db", shutdown.PriorityCritical, func() { | |
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE %s (FORCE)", dbName)); err != nil { | |
| if err := execPostgres(postgresDBUrl, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)); err != nil { | |
| adminURL := url | |
| postgresDBUrl = adminURL | |
| dbName = randomDatabaseName("duty_test") | |
| var err error | |
| PgUrl, err = databaseURL(url, dbName) | |
| if err != nil { | |
| return context.Context{}, err | |
| } | |
| quotedDBName := quoteIdentifier(dbName) | |
| _ = execPostgres(adminURL, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)) | |
| if err := execPostgres(adminURL, fmt.Sprintf("CREATE DATABASE %s", quotedDBName)); err != nil { | |
| return context.Context{}, fmt.Errorf("cannot create %s: %v", dbName, err) | |
| } | |
| shutdown.AddHookWithPriority("remove postgres db", shutdown.PriorityCritical, func() { | |
| if err := execPostgres(adminURL, fmt.Sprintf("DROP DATABASE IF EXISTS %s (FORCE)", quotedDBName)); err != nil { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/setup/common.go` around lines 229 - 245, The shutdown hook uses the
package-global postgresDBUrl which is later reassigned, so capture the admin URL
and any DB-specific identifiers into invocation-local variables (e.g.,
localPostgresDBUrl and localQuotedDBName) before calling execPostgres and before
registering shutdown.AddHookWithPriority; update all execPostgres calls and the
hook closure to use these local variables (referencing postgresDBUrl, PgUrl,
quoteIdentifier, execPostgres, and shutdown.AddHookWithPriority) so each SetupDB
invocation cleans up its own database instead of the possibly-reassigned global.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Duty tests can run against an always-on Postgres server via DUTY_DB_URL, but create mode reused predictable database names and rewrote URLs with string replacement.
Generate a unique database name for each suite process so multiple suites can share the same Postgres server safely. Build per-database URLs with net/url and quote database identifiers for create/drop operations.
This keeps embedded Postgres behavior unchanged while enabling faster local test runs against a long-lived Postgres server.
Summary by CodeRabbit