Postgres DB autocreation #806
Conversation
WalkthroughAdds a Postgres auto-create option: new config field and env var, wires the flag into store initialization, updates OpenDb signature and Postgres helpers to detect a missing database and create it before reconnecting, and updates tests, dev env, and README. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Cfg as Config
participant Svc as Service
participant Utils as PostgresUtils
participant DB as Postgres
App->>Cfg: LoadConfig()
Cfg-->>App: Config{PostgresAutoCreateDB}
App->>Svc: Init stores with [dsn, autoCreate]
Svc->>Utils: OpenDb(dsn, autoCreate)
Utils->>DB: PingContext()
alt DB exists
DB-->>Utils: OK
Utils-->>Svc: *sql.DB
else DB missing and autoCreate=true
DB-->>Utils: error (3D000)
Utils->>Utils: createDB(dsn)
Utils->>DB: connect to server (no db) -> CREATE DATABASE <name>
DB-->>Utils: Created
Utils->>DB: PingContext() retry
DB-->>Utils: OK
Utils-->>Svc: *sql.DB
else DB missing and autoCreate=false
DB-->>Utils: error (3D000)
Utils-->>Svc: error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/infrastructure/db/service.go (1)
135-200: Use the event-store config when opening the event DBIn the postgres branch you’re validating and unpacking
config.DataStoreConfig. That breaks setups where the event store uses its own DSN/flag (and even rejects mixed backends like event-store postgres + data-store sqlite). Switch the length/type checks toconfig.EventStoreConfig, and adjust the error text accordingly.- if len(config.DataStoreConfig) != 2 { - return nil, fmt.Errorf("invalid data store config for postgres") + if len(config.EventStoreConfig) != 2 { + return nil, fmt.Errorf("invalid event store config for postgres") } - dsn, ok := config.DataStoreConfig[0].(string) + dsn, ok := config.EventStoreConfig[0].(string) if !ok { return nil, fmt.Errorf("invalid DSN for postgres") } - autoCreate, ok := config.DataStoreConfig[1].(bool) + autoCreate, ok := config.EventStoreConfig[1].(bool) if !ok { return nil, fmt.Errorf("invalid autocreate flag for postgres") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
README.md(1 hunks)internal/config/config.go(5 hunks)internal/infrastructure/db/postgres/utils.go(1 hunks)internal/infrastructure/db/service.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/infrastructure/db/service.go (1)
internal/infrastructure/db/postgres/utils.go (1)
OpenDb(26-39)
qustavo
force-pushed
the
pg_db_autocreate
branch
2 times, most recently
from
f75f3ec to
1c440e0
Compare
|
@qustavo sorry to get back to you so late. I tested your changes and i got an error at startup:
Please sync your branch with master, then you can test by running Consider that, at the moment, the e2e tests on CI run against arkd configured with an sqlite db and a kv event db, therefore they are not reliable for your changes even if everything is green. We planned for the next week to change the setup for the testing env and move to an arkd configured with pg db and event db. |
|
@altafan thanks for taking the time to review this. Could you provide your |
|
i ran arkd with |
This adds the ARKD_PG_DB_AUTOCREATE flag to toggle auto db creation on postgres. Database autocreation will only be enabled when this flag is set to true or 1.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Gustavo Chaín <me@qustavo.cc>
As suggested by coderabbit
qustavo
force-pushed
the
pg_db_autocreate
branch
from
a2ff2a0 to
4326b37
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/infrastructure/db/postgres/utils.go (2)
23-65: OpenDb/connectDB flow is sound; fix small comment typoThe lazy
sql.Open+PingContext+ conditional auto-create flow looks correct, and the recursion withautoCreate=falseavoids infinite loops. The only nit is the inline comment on Line 56 still referring topingDBinstead ofconnectDB, which could confuse future readers.You can update it as:
- // Recursively call pingDB now that the DB exists but set autoCreate false to avoid + // Recursively call connectDB now that the DB exists but set autoCreate false to avoid
67-107: Handle concurrent auto-creation when the database already exists
createDBcorrectly derives the DB name from the URL and usespq.QuoteIdentifierto build a safeCREATE DATABASEstatement. One edge case: if multiple arkd instances start at the same time withautoCreate=true, they can race to create the same DB. The loser will see a “database already exists” error and fail startup even though the target DB is now available.To make this more robust, treat the Postgres “duplicate database” error (code
42P04) as a non-fatal success:- if _, err := rootDB.ExecContext(ctx, query); err != nil { - return err - } + if _, err := rootDB.ExecContext(ctx, query); err != nil { + var dbErr *pq.Error + if errors.As(err, &dbErr) && dbErr.Code == "42P04" { + log.Infof("Postgres database %s already exists, continuing startup", dbName) + return nil + } + return err + }This keeps behavior strict for other errors but avoids unnecessary failures when another instance has just created the DB.
internal/config/config.go (1)
71-78: PostgresAutoCreateDB wiring from env → config → repoManager looks correctThe
PostgresAutoCreateDBflag is consistently defined, read fromPG_DB_AUTOCREATE, and propagated into both the event and main Postgres store configs. This keeps auto-create behavior explicitly opt‑in while reusing a single flag for both stores, which is a reasonable tradeoff; no functional issues spotted here.Also applies to: 155-169, 337-401, 644-667
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.md(1 hunks)envs/arkd.dev.env(1 hunks)internal/config/config.go(5 hunks)internal/infrastructure/db/postgres/utils.go(1 hunks)internal/infrastructure/db/service.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/infrastructure/db/service.go
- README.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-08T11:50:25.008Z
Learnt from: altafan
Repo: arkade-os/arkd PR: 733
File: cmd/arkd/commands.go:601-606
Timestamp: 2025-10-08T11:50:25.008Z
Learning: In the market hour configuration (cmd/arkd/commands.go and related files), when roundMinParticipantsCount or roundMaxParticipantsCount is set to 0, the system uses default values from environment variables ARKD_ROUN_MIN/MAX_PARTICIPANTS_COUNT. Therefore, it's acceptable that these values cannot be explicitly cleared back to 0 after being set to a non-zero value.
Applied to files:
envs/arkd.dev.env
⏰ 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). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
🔇 Additional comments (1)
envs/arkd.dev.env (1)
13-13: New ARKD_PG_DB_AUTOCREATE flag in dev env looks consistentThe variable name matches the PG_DB_AUTOCREATE key and enables auto-create only for this dev profile; no issues from a config perspective.
|
Thanks for reviewing @altafan. Turns that in postgres if you do not specify a database name in the DSN, you will by default connect to a database named after the user name that you are connecting as, hence the |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/infrastructure/db/postgres/utils.go (3)
23-39: OpenDb: auto-create wiring looks good; consider error wrapping with%wThe new
autoCreateflag and delegation toconnectDBare clear and preserve existing behavior whenautoCreateis false. To keep the underlying error chain available to callers, consider wrapping with%winstead of%vin bothfmt.Errorfcalls:- return nil, fmt.Errorf("failed to open postgres db: %v", err) + return nil, fmt.Errorf("failed to open postgres db: %w", err) - return nil, fmt.Errorf("unable to establish connection with db: %v", err) + return nil, fmt.Errorf("unable to establish connection with db: %w", err)This lets higher layers still inspect driver/SQL errors via
errors.Is/errors.As.
41-65: connectDB recursion and comments: behavior is fine, small docstring nitThe
connectDBlogic (ping → detect missing DB → auto-create → recurse withautoCreate=false) is straightforward and bounded (at most one recursive call), which is good.Minor polish: the inline comment at Line 56 still mentions
pingDB, but the function is namedconnectDB. Updating that comment will reduce confusion for future readers:- // Recursively call pingDB now that the DB exists but set autoCreate false to avoid - // unlikely but possible infinite recursion. + // Recursively call connectDB now that the DB exists but set autoCreate=false to avoid + // unlikely but possible infinite recursion.
67-109: Based on my verification:
PostgreSQL SQLSTATE 42P04 is correct: This is the confirmed error code for
duplicate_databasein all current PostgreSQL versions.Race condition scenario is real: arkd with
autoCreate=trueon multiple concurrent instances can indeed race to create the same database, causing the second instance to hit a duplicate_database error.Error handling pattern is idiomatic: The suggested use of
errors.As(err, &dbErr)followed bydbErr.Code == "42P04"is the standard Go 1.13+ pattern for extracting and checking PostgreSQL driver-specific errors from lib/pq.Mitigation is sound: Treating the duplicate_database error as non-fatal and allowing the process to continue is a pragmatic and correct approach to make database creation idempotent.
The review comment's technical suggestion is accurate and would genuinely improve resilience. All error codes, patterns, and reasoning check out.
createDB: good quoting/guards; consider handling concurrent creators gracefully
The
createDBhelper is on the right track:
- It correctly restricts auto-create to URL-style DSNs.
- It validates that a non-empty DB name is present.
- It now uses
pq.QuoteIdentifier(dbName)in theCREATE DATABASEstatement, which avoids injection and handles special characters.One robustness gap: if multiple arkd instances start with
autoCreate=true, they can race. A second instance may hit the 3D000 "DB does not exist" error, callcreateDB, and then seeCREATE DATABASEfail because another instance created the DB first. In that case you can safely treat the error as non-fatal and proceed toconnectDBagain.A targeted tweak inside
createDBwould make this resilient:- if _, err := rootDB.ExecContext(ctx, query); err != nil { - return err - } + if _, err := rootDB.ExecContext(ctx, query); err != nil { + // If another instance created the DB in the meantime, treat it as success. + var dbErr *pq.Error + if errors.As(err, &dbErr) && dbErr.Code == "42P04" { // duplicate_database + log.Infof("Database %s already exists, continuing startup", dbName) + return nil + } + return err + }This way auto-creation becomes idempotent across multiple concurrent processes, avoiding a hard startup failure when a race occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/infrastructure/db/postgres/utils.go(1 hunks)
⏰ 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). (3)
- GitHub Check: Build and Scan
- GitHub Check: integration tests
- GitHub Check: unit tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/infrastructure/db/service_test.go (1)
173-174: Configuration updated correctly to match new API signature.The addition of the boolean flag aligns with the updated
OpenDb(dsn, autoCreate)signature. However, the test only exercises the auto-create disabled (false) path.Consider adding a test case that verifies the auto-create functionality when set to
true:{ name: "repo_manager_with_postgres_stores", config: db.ServiceConfig{ EventStoreType: "postgres", DataStoreType: "postgres", EventStoreConfig: []interface{}{pgEventDns, false}, DataStoreConfig: []interface{}{pgDns, false}, }, }, + { + name: "repo_manager_with_postgres_stores_autocreate", + config: db.ServiceConfig{ + EventStoreType: "postgres", + DataStoreType: "postgres", + EventStoreConfig: []interface{}{pgEventDns, true}, + DataStoreConfig: []interface{}{pgDns, true}, + }, + },Note: The new test case would need databases that don't exist initially to properly test the auto-create feature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/infrastructure/db/service_test.go(1 hunks)
⏰ 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). (3)
- GitHub Check: Build and Scan
- GitHub Check: unit tests
- GitHub Check: integration tests
|
I apologize again for the slow review process, but thank you for your contribution @qustavo! |
2 participants
Although some advice against allowing programs to auto-create databases (due to security policies and separation of concerns) it is also convenient to have the ability to do so, this can come handy for testing environments.
Based on the above this PR introduces the
ARKD_PG_DB_AUTOCREATEflag that toggles database creation when it does not exist.Closes #754
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.