Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a Postgres database resource and threads database-scoped identifiers/dependencies across replication/sync resources; adds state versioning with a 1.0.0 migration and schematool; refactors DSN/Orchestrator APIs and node/instance helpers; propagates DatabaseName through operations, golden tests, and HostID through resource contexts. Changes
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
a4bfd8b to
0af2711
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
2 times, most recently
from
887b1a8 to
daeb238
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
0af2711 to
9df990e
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
2 times, most recently
from
20b19ea to
b768409
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
ae1a213 to
dc3adc0
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
b768409 to
de8a54c
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
dc3adc0 to
31a709a
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
de8a54c to
e753e90
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
31a709a to
f647f73
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
e753e90 to
e47f053
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
f647f73 to
29af08f
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
server/internal/postgres/roles.go (1)
98-111:⚠️ Potential issue | 🔴 CriticalCopy-paste bug: condition checks wrong role name.
The conditional statement at line 102 checks for
'pgedge_application'but creates'pgedge_application_read_only'at line 105. This is inconsistent and will fail to create the read-only role ifpgedge_applicationalready exists.🐛 Proposed fix
func CreateApplicationReadOnlyRole(opts BuiltinRoleOptions) Statements { statements := Statements{ ConditionalStatement{ If: Query[bool]{ - SQL: "SELECT NOT EXISTS (SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = 'pgedge_application');", + SQL: "SELECT NOT EXISTS (SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = 'pgedge_application_read_only');", }, Then: Statement{ SQL: "CREATE ROLE pgedge_application_read_only WITH NOLOGIN;", }, }, } return statements }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/postgres/roles.go` around lines 98 - 111, The condition in CreateApplicationReadOnlyRole uses the wrong role name; update the Query inside the ConditionalStatement so it checks for NOT EXISTS (... WHERE rolname = 'pgedge_application_read_only') to match the role created by the Statement (pgedge_application_read_only), ensuring the conditional creates the read-only role when it doesn't already exist.server/internal/database/sync_event_resource.go (1)
23-27:⚠️ Potential issue | 🟠 MajorWire
DatabaseNameinto identity and connection selection.Right now this new field is only stored.
Identifier()still keys the resource byproviderNode + subscriberNode, andRefresh()still connects withprovider.Spec.DatabaseName. That means database-scoped sync events will collide for the same node pair and can still run against the wrong database.Suggested fix
-func SyncEventResourceIdentifier(providerNode, subscriberNode string) resource.Identifier { +func SyncEventResourceIdentifier(providerNode, subscriberNode, databaseName string) resource.Identifier { return resource.Identifier{ - ID: providerNode + subscriberNode, + ID: providerNode + subscriberNode + "-" + databaseName, Type: ResourceTypeSyncEvent, } } @@ func (r *SyncEventResource) Identifier() resource.Identifier { - return SyncEventResourceIdentifier(r.ProviderNode, r.SubscriberNode) + return SyncEventResourceIdentifier(r.ProviderNode, r.SubscriberNode, r.DatabaseName) } @@ - providerConn, err := provider.Connection(ctx, rc, provider.Spec.DatabaseName) + providerConn, err := provider.Connection(ctx, rc, r.DatabaseName) if err != nil { - return fmt.Errorf("failed to connect to provider database %q: %w", provider.Spec.DatabaseName, err) + return fmt.Errorf("failed to connect to provider database %q: %w", r.DatabaseName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/sync_event_resource.go` around lines 23 - 27, The resource's identity and connection selection must include the new DatabaseName field: update Identifier() to incorporate DatabaseName (e.g., include r.DatabaseName in the composite key returned so keys are unique per providerNode+subscriberNode+database), and update Refresh() to use the resource's DatabaseName when selecting/creating the provider connection instead of using provider.Spec.DatabaseName (ensure any connection lookup or call uses r.DatabaseName). Also update any equality/hash logic that builds the identity string in Identifier() to reflect the added field so database-scoped sync events no longer collide.server/internal/database/wait_for_sync_event_resource.go (1)
19-24:⚠️ Potential issue | 🟠 Major
DatabaseNamestill isn’t part of this resource’s identity or execution path.The new field is stored on the struct, but the identifier, dependency lookup, and connection target still key off only the node pair. Two databases replicated between the same provider/subscriber nodes will collapse into one
WaitForSyncEventResource, andRefresh()can wait on the wrong database by usingsubscriber.Spec.DatabaseNameinstead ofr.DatabaseName.Also applies to: 44-52, 59-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/wait_for_sync_event_resource.go` around lines 19 - 24, The resource identity and execution must include DatabaseName: change WaitForSyncEventResourceIdentifier to incorporate the database name into the ID (e.g., providerNode + subscriberNode + ":" + databaseName) and update any places that build or compare resource identifiers (including the other functions mentioned around the 44-52 and 59-68 diffs) to use this new identifier format; in Refresh() and any dependency lookup or connection logic replace uses of subscriber.Spec.DatabaseName with r.DatabaseName so the wait targets the resource's own DatabaseName rather than the subscriber spec.server/internal/workflows/plan_restore.go (1)
92-103:⚠️ Potential issue | 🟠 MajorPass node-level restore config to the activity, not the workflow-level config.
The
nodeparameter already carriesnode.RestoreConfig, but line 102 forwards the workflow-levelrestoreConfigtoGetRestoreResourcesInput. Since the activity usesinput.RestoreConfigdirectly (line 57 ofget_restore_resources.go), node-level restore overrides are lost. ChangeRestoreConfig: restoreConfig,toRestoreConfig: node.RestoreConfig,.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/workflows/plan_restore.go` around lines 92 - 103, The workflow currently passes the workflow-level restoreConfig into the activity, causing node-level overrides to be ignored; in the loop that constructs the GetRestoreResourcesInput (when creating in := &activities.GetRestoreResourcesInput{...}) replace RestoreConfig: restoreConfig with RestoreConfig: node.RestoreConfig so the activity (used by GetRestoreResources) receives the node-specific restore settings; locate this in the block that builds nodeRestore (NodeRestoreResources) and the inner loop over node.Instances where in is created using taskID and instance.server/internal/orchestrator/swarm/service_user_role.go (1)
118-155:⚠️ Potential issue | 🔴 CriticalExecute schema and table grants against
r.DatabaseName, not thepostgresdatabase.
connectToPrimary()connects to thepostgresdatabase (line 257), so lines 147–149 grantUSAGEandSELECTon thepublicschema withinpostgres, not withinr.DatabaseName. This leaves the service role withCONNECTpermission onr.DatabaseNamebut no ability to access tables there. Open a second connection targetingr.DatabaseNamefor the schema/table grant statements.Additionally, line 149's
ALTER DEFAULT PRIVILEGESlacksFOR ROLE, so it only affects future objects created by the executing admin role. If the database owner creates tables, the default privileges will not apply to them. AddFOR ROLE <owner>if future owner-created tables should inherit these privileges.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/service_user_role.go` around lines 118 - 155, The schema/table GRANTs are being executed against the connection returned by connectToPrimary (which targets the postgres DB) so USAGE/SELECT/ALTER DEFAULT PRIVILEGES run on postgres instead of r.DatabaseName; open a second connection targeting r.DatabaseName (e.g., add a new conn2 via a connect-to-database call or extend connectToPrimary to accept a database name) and execute the grants variable (the statements that currently build GRANT USAGE/SELECT and ALTER DEFAULT PRIVILEGES) against that connection, ensuring you Close() it; also change the ALTER DEFAULT PRIVILEGES statement to include FOR ROLE <owner> (use the actual database owner variable or query the DB owner) so it affects objects created by the database owner rather than only the admin executing the command.server/internal/database/operations/update_nodes_test.go (1)
153-153:⚠️ Potential issue | 🟡 MinorBug:
t.Name()should betc.namefor sub-test naming.Using
t.Name()returns the parent test function name (TestUpdateNode), not the individual test case name. This means all sub-tests will have the same name, making test output confusing and potentially causing issues with test filtering/reporting.Note: Line 377 in
TestRollingUpdateNodeshas the same issue, while line 593 inTestConcurrentUpdateNodescorrectly usestc.name.Proposed fix
- t.Run(t.Name(), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {And similarly at line 377:
- t.Run(t.Name(), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/update_nodes_test.go` at line 153, Sub-tests inside TestUpdateNode (and the similar case in TestRollingUpdateNodes) are using t.Run(t.Name(), ...) which yields the parent test name for every case; change those calls to use the table case identifier tc.name (i.e., replace t.Run(t.Name(), func... ) with t.Run(tc.name, func... ) ) so each sub-test is uniquely named — verify TestConcurrentUpdateNodes already uses tc.name as reference.server/internal/database/operations/populate_nodes_test.go (1)
166-176:⚠️ Potential issue | 🟡 MinorBug: Test uses
t.Name()instead oftc.namefor subtest naming.Using
t.Name()returns the parent test's name, causing all subtests to have the same name. This makes it harder to identify which test case failed and may cause test filtering issues.🐛 Proposed fix
- t.Run(t.Name(), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/populate_nodes_test.go` around lines 166 - 176, The subtests in populate_nodes_test.go call t.Run(t.Name(), ...) so every case gets the parent test name; change the subtest name to use the test-case identifier (tc.name) in the t.Run call so each subtest is uniquely named, e.g. t.Run(tc.name, func(t *testing.T) { ... }), ensuring you reference the table-driven test variable tc and its name field around the call to operations.PopulateNode.
🧹 Nitpick comments (5)
docker/control-plane-dev/config.json (1)
4-8: Consider if "error" level is appropriate for a dev environment.Setting the
api_serverlog level to "error" will suppress warnings, info, and debug messages. Since this is a development configuration, you may want to use a more verbose level (e.g., "debug" or "info") to aid in troubleshooting and development.If this is intentional to reduce log noise, consider documenting the rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/control-plane-dev/config.json` around lines 4 - 8, The current dev config sets the api_server log level to "error" which will hide warnings/info/debug; update the "logging.component_levels.api_server" value to a more verbose level (e.g., "info" or "debug") in the config and, if keeping "error" intentionally, add a brief comment or documentation note explaining the rationale; locate the "logging" -> "component_levels" -> "api_server" key in the JSON and change the value or add accompanying documentation where config is defined.server/internal/postgres/roles.go (1)
152-162: Consider extracting shared schema logic.
BuiltinRolePrivilegeOptions.Schemas()(lines 157-162) is identical toBuiltinRoleOptions.Schemas()(lines 66-71). Consider extracting an embedded struct or a shared helper to reduce duplication.♻️ Possible refactor using embedded struct
+type SchemaConfig struct { + ExtraSchemas []string +} + +func (s SchemaConfig) Schemas() []string { + var schemas []string + schemas = append(schemas, defaultSchemas...) + schemas = append(schemas, s.ExtraSchemas...) + return schemas +} + type BuiltinRoleOptions struct { - PGVersion string - ExtraSchemas []string -} - -func (o BuiltinRoleOptions) Schemas() []string { - var schemas []string - schemas = append(schemas, defaultSchemas...) - schemas = append(schemas, o.ExtraSchemas...) - return schemas + PGVersion string + SchemaConfig } type BuiltinRolePrivilegeOptions struct { - DBName string - ExtraSchemas []string -} - -func (o BuiltinRolePrivilegeOptions) Schemas() []string { - var schemas []string - schemas = append(schemas, defaultSchemas...) - schemas = append(schemas, o.ExtraSchemas...) - return schemas + DBName string + SchemaConfig }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/postgres/roles.go` around lines 152 - 162, BuiltinRolePrivilegeOptions.Schemas() duplicates BuiltinRoleOptions.Schemas() — extract the shared logic into a single helper or embedded type and have both structs reuse it; for example create a small SchemaOptions struct (holding ExtraSchemas) with a Schemas() method that appends defaultSchemas and ExtraSchemas, or add a package-level function SchemasFor(extra []string) []string that returns append(defaultSchemas, extra...), then replace the duplicate implementations in BuiltinRolePrivilegeOptions and BuiltinRoleOptions to call the shared Schemas implementation and remove the duplicated method bodies.server/internal/database/instance_resource.go (1)
137-143: Consider updating error message for consistency.Similar to the
initializeInstancemethod, theConnectionmethod connects todbName(line 138) but the error message (line 142) referencesr.Spec.DatabaseName, which may not match the actual database being connected to.Proposed fix
conn, err := ConnectToInstance(ctx, &ConnectionOptions{ DSN: r.ConnectionInfo.AdminDSN(dbName), TLS: tlsCfg, }) if err != nil { - return nil, fmt.Errorf("failed to connect to database %q: %w", r.Spec.DatabaseName, err) + return nil, fmt.Errorf("failed to connect to database %q: %w", dbName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/instance_resource.go` around lines 137 - 143, The error message in the Connection method uses r.Spec.DatabaseName while the attempted connection is to the local variable dbName; update the fmt.Errorf call to reference dbName (the actual database passed to ConnectToInstance) for consistency and accurate diagnostics—locate the ConnectToInstance call and the fmt.Errorf that returns "failed to connect to database" and replace the r.Spec.DatabaseName reference with dbName.server/internal/database/operations/helpers_test.go (1)
69-82: Add one fixture that exercises non-nildatabaseDependencies.This helper always passes
nilfor the new constructor argument, so the planner tests in this file still never exercise the extra dependency layer introduced byNewInstanceResources. A regression that dropsPostgresDatabaseordering would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/helpers_test.go` around lines 69 - 82, The test helper always calls NewInstanceResources with a nil third argument, so it never exercises non-nil databaseDependencies; update the fixture(s) that call NewInstanceResources in helpers_test.go to include at least one case where the third parameter is a non-nil dependencies slice (e.g., a realistic databaseDependencies value or a minimal list containing a PostgresDatabase dependency) so tests hit the extra dependency layer introduced by NewInstanceResources (locate calls to NewInstanceResources in this file and add a non-nil dependencies argument to one fixture).server/internal/database/replication_slot_advance_from_cts_resource.go (1)
18-24: Identifier does not includeDatabaseName- potential collision with multi-database support.The
ReplicationSlotAdvanceFromCTSResourceIdentifieronly usesproviderNode + subscriberNode, but the resource now has aDatabaseNamefield. If multiple databases are ever created per node (as mentioned in the PR objectives for future multi-database support), resources for different databases would have colliding identifiers.The PR summary mentions "database name was not incorporated into resource identities yet," so this may be intentional for now, but it's worth tracking for when multi-database support is fully enabled.
Future consideration: Include DatabaseName in identifier
-func ReplicationSlotAdvanceFromCTSResourceIdentifier(providerNode, subscriberNode string) resource.Identifier { +func ReplicationSlotAdvanceFromCTSResourceIdentifier(databaseName, providerNode, subscriberNode string) resource.Identifier { return resource.Identifier{ Type: ResourceTypeReplicationSlotAdvanceFromCTS, - ID: providerNode + subscriberNode, + ID: databaseName + "-" + providerNode + "-" + subscriberNode, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/replication_slot_advance_from_cts_resource.go` around lines 18 - 24, The identifier function ReplicationSlotAdvanceFromCTSResourceIdentifier currently builds IDs from providerNode + subscriberNode which can collide across databases; update this function to incorporate the resource's DatabaseName (e.g., concatenate providerNode, subscriberNode and DatabaseName with a stable separator) so IDs are unique per-database; ensure you use the ResourceTypeReplicationSlotAdvanceFromCTS as before and pick a separator that cannot appear in node or db names (or escape values) so existing consumers of resource.Identifier remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/database/instance_resource.go`:
- Around line 183-189: The error message incorrectly references
r.Spec.DatabaseName even though ConnectToInstance is connecting to the
"postgres" admin DB via r.ConnectionInfo.AdminDSN("postgres"); update the error
to reference the actual target (e.g., "postgres" or derive the admin DB name
from r.ConnectionInfo.AdminDSN call) so the fmt.Errorf in the ConnectToInstance
error branch accurately names the database being connected to and not
r.Spec.DatabaseName.
In `@server/internal/database/lag_tracker_commit_ts_resource.go`:
- Line 29: The resource's DatabaseName field is not being used for identity or
connection, causing different databases on the same node pair to collide; update
the Identifier() method to include DatabaseName in the key (alongside
origin/receiver), update the resource constructor/path so DatabaseName is
threaded into the created lag-tracker resource (replace any use of
instance.Spec.DatabaseName in constructors with the resource's DatabaseName),
and modify Refresh() (and any place that dials/connects, e.g., the code
currently using instance.Spec.DatabaseName) to use r.DatabaseName as the
connection target; ensure the same change is applied where the
constructor/initialization happens (lines around the existing ctor usage noted
at 60-61).
In `@server/internal/database/operations/common.go`:
- Around line 83-86: The error message is misleading: when state.AddResource(db)
fails we're adding a PostgresDatabaseResource (db), so update the fmt.Errorf
message in the failure branch to explicitly reference the
PostgresDatabaseResource or database resource (e.g., "failed to add
PostgresDatabaseResource to state" or "failed to add database resource to
state") so logs correctly reflect the operation performed (locate the call to
state.AddResource(db) and change the error text returned alongside the wrapped
err).
In `@server/internal/database/replication_slot_resource.go`:
- Line 29: The DatabaseName field was added but not threaded into resource
identity and deletion: update the Identifier() method to include the
DatabaseName (so the resource key includes provider, subscriber and
DatabaseName) and update Delete() to use the resource's DatabaseName (not
provider.Spec.DatabaseName) when looking up and deleting the replication
slot/state; ensure any state keys, comparisons and List/Delete calls in
Identifier(), Delete(), and any helper that builds the replication-slot key use
the DatabaseName field so multiple DBs on the same provider/subscriber are kept
distinct.
In `@server/internal/database/spec.go`:
- Around line 587-593: NodeInstances() currently picks the first user with
DBOwner==true from DatabaseUsers (setting DatabaseOwner) without validation,
which makes ownership order-dependent or empty; add validation in
validateDatabaseSpec() (or equivalent service-layer validation) to assert
exactly one DatabaseUsers entry has DBOwner==true and return an error if count
!= 1 so NodeInstances() can assume a single owner; update any callers/tests to
handle the validation error and reference DatabaseUsers, DBOwner, DatabaseOwner,
NodeInstances(), and validateDatabaseSpec() when implementing the check.
In `@server/internal/postgres/dsn.go`:
- Around line 35-66: The iteration over d.Extra in DSN.Fields() makes
DSN.String() nondeterministic; before the for loop that calls addField for each
entry in d.Extra, collect the map keys, sort them (e.g., using sort.Strings),
and then iterate the sorted keys to append fields in a stable order; update
references in DSN.Fields() (and any other place that serializes d.Extra) so
addField is invoked in key-sorted order to ensure deterministic DSN.String()
output.
---
Outside diff comments:
In `@server/internal/database/operations/populate_nodes_test.go`:
- Around line 166-176: The subtests in populate_nodes_test.go call
t.Run(t.Name(), ...) so every case gets the parent test name; change the subtest
name to use the test-case identifier (tc.name) in the t.Run call so each subtest
is uniquely named, e.g. t.Run(tc.name, func(t *testing.T) { ... }), ensuring you
reference the table-driven test variable tc and its name field around the call
to operations.PopulateNode.
In `@server/internal/database/operations/update_nodes_test.go`:
- Line 153: Sub-tests inside TestUpdateNode (and the similar case in
TestRollingUpdateNodes) are using t.Run(t.Name(), ...) which yields the parent
test name for every case; change those calls to use the table case identifier
tc.name (i.e., replace t.Run(t.Name(), func... ) with t.Run(tc.name, func... ) )
so each sub-test is uniquely named — verify TestConcurrentUpdateNodes already
uses tc.name as reference.
In `@server/internal/database/sync_event_resource.go`:
- Around line 23-27: The resource's identity and connection selection must
include the new DatabaseName field: update Identifier() to incorporate
DatabaseName (e.g., include r.DatabaseName in the composite key returned so keys
are unique per providerNode+subscriberNode+database), and update Refresh() to
use the resource's DatabaseName when selecting/creating the provider connection
instead of using provider.Spec.DatabaseName (ensure any connection lookup or
call uses r.DatabaseName). Also update any equality/hash logic that builds the
identity string in Identifier() to reflect the added field so database-scoped
sync events no longer collide.
In `@server/internal/database/wait_for_sync_event_resource.go`:
- Around line 19-24: The resource identity and execution must include
DatabaseName: change WaitForSyncEventResourceIdentifier to incorporate the
database name into the ID (e.g., providerNode + subscriberNode + ":" +
databaseName) and update any places that build or compare resource identifiers
(including the other functions mentioned around the 44-52 and 59-68 diffs) to
use this new identifier format; in Refresh() and any dependency lookup or
connection logic replace uses of subscriber.Spec.DatabaseName with
r.DatabaseName so the wait targets the resource's own DatabaseName rather than
the subscriber spec.
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 118-155: The schema/table GRANTs are being executed against the
connection returned by connectToPrimary (which targets the postgres DB) so
USAGE/SELECT/ALTER DEFAULT PRIVILEGES run on postgres instead of r.DatabaseName;
open a second connection targeting r.DatabaseName (e.g., add a new conn2 via a
connect-to-database call or extend connectToPrimary to accept a database name)
and execute the grants variable (the statements that currently build GRANT
USAGE/SELECT and ALTER DEFAULT PRIVILEGES) against that connection, ensuring you
Close() it; also change the ALTER DEFAULT PRIVILEGES statement to include FOR
ROLE <owner> (use the actual database owner variable or query the DB owner) so
it affects objects created by the database owner rather than only the admin
executing the command.
In `@server/internal/postgres/roles.go`:
- Around line 98-111: The condition in CreateApplicationReadOnlyRole uses the
wrong role name; update the Query inside the ConditionalStatement so it checks
for NOT EXISTS (... WHERE rolname = 'pgedge_application_read_only') to match the
role created by the Statement (pgedge_application_read_only), ensuring the
conditional creates the read-only role when it doesn't already exist.
In `@server/internal/workflows/plan_restore.go`:
- Around line 92-103: The workflow currently passes the workflow-level
restoreConfig into the activity, causing node-level overrides to be ignored; in
the loop that constructs the GetRestoreResourcesInput (when creating in :=
&activities.GetRestoreResourcesInput{...}) replace RestoreConfig: restoreConfig
with RestoreConfig: node.RestoreConfig so the activity (used by
GetRestoreResources) receives the node-specific restore settings; locate this in
the block that builds nodeRestore (NodeRestoreResources) and the inner loop over
node.Instances where in is created using taskID and instance.
---
Nitpick comments:
In `@docker/control-plane-dev/config.json`:
- Around line 4-8: The current dev config sets the api_server log level to
"error" which will hide warnings/info/debug; update the
"logging.component_levels.api_server" value to a more verbose level (e.g.,
"info" or "debug") in the config and, if keeping "error" intentionally, add a
brief comment or documentation note explaining the rationale; locate the
"logging" -> "component_levels" -> "api_server" key in the JSON and change the
value or add accompanying documentation where config is defined.
In `@server/internal/database/instance_resource.go`:
- Around line 137-143: The error message in the Connection method uses
r.Spec.DatabaseName while the attempted connection is to the local variable
dbName; update the fmt.Errorf call to reference dbName (the actual database
passed to ConnectToInstance) for consistency and accurate diagnostics—locate the
ConnectToInstance call and the fmt.Errorf that returns "failed to connect to
database" and replace the r.Spec.DatabaseName reference with dbName.
In `@server/internal/database/operations/helpers_test.go`:
- Around line 69-82: The test helper always calls NewInstanceResources with a
nil third argument, so it never exercises non-nil databaseDependencies; update
the fixture(s) that call NewInstanceResources in helpers_test.go to include at
least one case where the third parameter is a non-nil dependencies slice (e.g.,
a realistic databaseDependencies value or a minimal list containing a
PostgresDatabase dependency) so tests hit the extra dependency layer introduced
by NewInstanceResources (locate calls to NewInstanceResources in this file and
add a non-nil dependencies argument to one fixture).
In `@server/internal/database/replication_slot_advance_from_cts_resource.go`:
- Around line 18-24: The identifier function
ReplicationSlotAdvanceFromCTSResourceIdentifier currently builds IDs from
providerNode + subscriberNode which can collide across databases; update this
function to incorporate the resource's DatabaseName (e.g., concatenate
providerNode, subscriberNode and DatabaseName with a stable separator) so IDs
are unique per-database; ensure you use the
ResourceTypeReplicationSlotAdvanceFromCTS as before and pick a separator that
cannot appear in node or db names (or escape values) so existing consumers of
resource.Identifier remain correct.
In `@server/internal/postgres/roles.go`:
- Around line 152-162: BuiltinRolePrivilegeOptions.Schemas() duplicates
BuiltinRoleOptions.Schemas() — extract the shared logic into a single helper or
embedded type and have both structs reuse it; for example create a small
SchemaOptions struct (holding ExtraSchemas) with a Schemas() method that appends
defaultSchemas and ExtraSchemas, or add a package-level function
SchemasFor(extra []string) []string that returns append(defaultSchemas,
extra...), then replace the duplicate implementations in
BuiltinRolePrivilegeOptions and BuiltinRoleOptions to call the shared Schemas
implementation and remove the duplicated method bodies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a84ae357-8fd4-49e5-877f-563e790fe1cc
📒 Files selected for processing (64)
docker/control-plane-dev/config.jsonserver/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/populate_nodes_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/postgres_database.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/resources.goserver/internal/database/spec.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/monitor/instance_monitor_resource.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/postgres/create_db.goserver/internal/postgres/dsn.goserver/internal/postgres/errors.goserver/internal/postgres/roles.goserver/internal/postgres/roles_test.goserver/internal/resource/event_test.goserver/internal/resource/resource.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/activities/executor.goserver/internal/workflows/common.goserver/internal/workflows/plan_restore.go
💤 Files with no reviewable changes (1)
- server/internal/postgres/roles_test.go
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
29af08f to
bf3dd8b
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
e47f053 to
26aa05d
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
bf3dd8b to
0508e04
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
26aa05d to
a856583
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
0508e04 to
7f89e1e
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
a856583 to
57a6663
Compare
This was referenced Mar 11, 2026
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
7f89e1e to
b6a8fb4
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
57a6663 to
e656f49
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
b6a8fb4 to
53593e9
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
e656f49 to
4e0f475
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/resource-type-dependencies
branch
from
53593e9 to
53394ff
Compare
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
4e0f475 to
3b3cfbb
Compare
Base automatically changed from
feat/PLAT-417/resource-type-dependencies
to
main
March 13, 2026 13:12
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
3b3cfbb to
42e1c4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/postgres/roles.go (1)
98-111:⚠️ Potential issue | 🟠 MajorPre-existing bug: incorrect role check in
CreateApplicationReadOnlyRole.Line 102 checks for the existence of
pgedge_applicationbut the function createspgedge_application_read_onlyon line 105. This causes the read-only role creation to depend incorrectly on whether the application role exists.This is a pre-existing issue not introduced by this PR, but should be tracked for a follow-up fix.
🐛 Proposed fix
func CreateApplicationReadOnlyRole(opts BuiltinRoleOptions) Statements { statements := Statements{ ConditionalStatement{ If: Query[bool]{ - SQL: "SELECT NOT EXISTS (SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = 'pgedge_application');", + SQL: "SELECT NOT EXISTS (SELECT 1 FROM pg_catalog.pg_roles WHERE rolname = 'pgedge_application_read_only');", }, Then: Statement{ SQL: "CREATE ROLE pgedge_application_read_only WITH NOLOGIN;", }, }, }Do you want me to open a new issue to track this bug fix?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/postgres/roles.go` around lines 98 - 111, The existence check in CreateApplicationReadOnlyRole is querying for 'pgedge_application' but the function creates 'pgedge_application_read_only', so change the Query SQL in CreateApplicationReadOnlyRole to check for NOT EXISTS ... WHERE rolname = 'pgedge_application_read_only' (so the ConditionalStatement's If Query matches the actual role created) and ensure the Then Statement remains CREATE ROLE pgedge_application_read_only WITH NOLOGIN; to avoid the creation depending on the wrong role name.
♻️ Duplicate comments (3)
server/internal/database/spec.go (1)
587-593:⚠️ Potential issue | 🟠 MajorEnforce exactly one
DBOwnerbefore assigningDatabaseOwner.Current logic picks the first owner and silently accepts zero/multiple owners, making ownership order-dependent or empty.
Suggested fix
- var owner string + var owner string + ownerCount := 0 for _, user := range s.DatabaseUsers { if user.DBOwner { owner = user.Username - break + ownerCount++ } } + if ownerCount != 1 { + return nil, fmt.Errorf("expected exactly one database owner, found %d", ownerCount) + }Also applies to: 645-646
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec.go` around lines 587 - 593, The loop that sets owner by taking the first s.DatabaseUsers with DBOwner is unsafe; instead, count and collect DBOwner entries from s.DatabaseUsers, if count != 1 return or propagate an error (or fail validation) and only then set the DatabaseOwner field to that sole user.Username; apply the same exact validation/assignment change for the other occurrence that currently picks an owner (the second block using DBOwner at the later assignment).server/internal/database/operations/common.go (1)
83-85:⚠️ Potential issue | 🟡 MinorName the failing resource correctly here.
state.AddResource(db)is adding aPostgresDatabaseResource, so the current message sends debugging toward the node path instead of the database path.📝 Proposed fix
err := state.AddResource(db) if err != nil { - return nil, fmt.Errorf("failed to add node resources to state: %w", err) + return nil, fmt.Errorf("failed to add database resource to state: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/common.go` around lines 83 - 85, The error message after calling state.AddResource(db) incorrectly references a node resource; update the error to name the actual resource being added (a PostgresDatabaseResource) so debugging points to the database path. Locate the state.AddResource(db) call and change the fmt.Errorf text from "failed to add node resources to state: %w" to something like "failed to add database resource to state: %w" (optionally include db identifier if available) to clearly reference the PostgresDatabaseResource being added.server/internal/database/instance_resource.go (1)
137-143:⚠️ Potential issue | 🟡 MinorUse the requested
dbNamein the connect error.
Connection()accepts the target database as a parameter, but the error still reportsr.Spec.DatabaseName. Calls like Line 178 now connect to"postgres", so this message is misleading on admin-connection failures.📝 Proposed fix
conn, err := ConnectToInstance(ctx, &ConnectionOptions{ DSN: r.ConnectionInfo.AdminDSN(dbName), TLS: tlsCfg, }) if err != nil { - return nil, fmt.Errorf("failed to connect to database %q: %w", r.Spec.DatabaseName, err) + return nil, fmt.Errorf("failed to connect to database %q: %w", dbName, err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/instance_resource.go` around lines 137 - 143, The error message when ConnectToInstance fails uses r.Spec.DatabaseName but should reference the actual target database parameter dbName; update the fmt.Errorf call in the Connection() function (the block that calls ConnectToInstance with ConnectionOptions and r.ConnectionInfo.AdminDSN(dbName)) to interpolate dbName instead of r.Spec.DatabaseName so the error accurately reports the admin DB being connected to.
🧹 Nitpick comments (3)
server/internal/postgres/roles.go (1)
152-162: Consider extracting duplicateSchemas()logic.
BuiltinRolePrivilegeOptions.Schemas()duplicatesBuiltinRoleOptions.Schemas()(lines 66-71). Could embed a shared base or extract a helper.♻️ Optional: Extract shared helper
+func combineSchemas(extraSchemas []string) []string { + var schemas []string + schemas = append(schemas, defaultSchemas...) + schemas = append(schemas, extraSchemas...) + return schemas +} + func (o BuiltinRoleOptions) Schemas() []string { - var schemas []string - schemas = append(schemas, defaultSchemas...) - schemas = append(schemas, o.ExtraSchemas...) - return schemas + return combineSchemas(o.ExtraSchemas) } ... func (o BuiltinRolePrivilegeOptions) Schemas() []string { - var schemas []string - schemas = append(schemas, defaultSchemas...) - schemas = append(schemas, o.ExtraSchemas...) - return schemas + return combineSchemas(o.ExtraSchemas) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/postgres/roles.go` around lines 152 - 162, The Schemas() implementation in BuiltinRolePrivilegeOptions duplicates BuiltinRoleOptions.Schemas(); extract the shared logic into a single helper (e.g., a package-level function like combinedSchemas(extra []string) or a small embedded struct with Schemas()) that appends defaultSchemas and the provided extra list, then have both BuiltinRolePrivilegeOptions.Schemas() and BuiltinRoleOptions.Schemas() call that helper; reference the existing symbols BuiltinRolePrivilegeOptions.Schemas, BuiltinRoleOptions.Schemas, and defaultSchemas when making the change.server/internal/database/lag_tracker_commit_ts_resource.go (1)
38-43: Constructor does not initializeDatabaseName.The
NewLagTrackerCommitTimestampResourceconstructor doesn't accept or set theDatabaseNamefield, while the resource now depends on it forDependencies(). Callers must manually setDatabaseNameafter construction. Consider either addingdatabaseNameas a constructor parameter or documenting that callers must set it explicitly.🔧 Optional: Add DatabaseName to constructor
-func NewLagTrackerCommitTimestampResource(originNode, receiverNode string) *LagTrackerCommitTimestampResource { +func NewLagTrackerCommitTimestampResource(databaseName, originNode, receiverNode string) *LagTrackerCommitTimestampResource { return &LagTrackerCommitTimestampResource{ + DatabaseName: databaseName, OriginNode: originNode, ReceiverNode: receiverNode, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/lag_tracker_commit_ts_resource.go` around lines 38 - 43, The constructor NewLagTrackerCommitTimestampResource currently doesn't set the DatabaseName field but Dependencies() relies on it; modify NewLagTrackerCommitTimestampResource to accept a databaseName string parameter and assign it to the returned LagTrackerCommitTimestampResource's DatabaseName field, and update all callers to pass the database name; alternatively (if you prefer minimal changes) document and enforce via comment that callers must set DatabaseName immediately after construction, but the preferred fix is adding the databaseName parameter and assigning it inside NewLagTrackerCommitTimestampResource so Dependencies() always has a valid DatabaseName.server/internal/database/replication_slot_advance_from_cts_resource.go (1)
85-89:Create()usesprovider.Spec.DatabaseNameinstead ofr.DatabaseName.While the resource now has a
DatabaseNamefield and uses it inDependencies(), theCreate()method connects and operates usingprovider.Spec.DatabaseName(lines 85, 95, 109, 119, 136). Under the current single-database-per-control-plane contract this is safe, but for consistency with theReplicationSlotResource.Delete()fix, consider usingr.DatabaseName.♻️ Suggested fix for consistency
conn, err := provider.Connection(ctx, rc, provider.Spec.DatabaseName) + // Consider using r.DatabaseName for consistency: + // conn, err := provider.Connection(ctx, rc, r.DatabaseName)Apply similar changes to lines 95, 109, 119, and 136.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/replication_slot_advance_from_cts_resource.go` around lines 85 - 89, The Create() method is still using provider.Spec.DatabaseName when opening connections and operating on the DB; update Create() to use the resource's r.DatabaseName (the same field used in Dependencies()) for all connection and operation calls to match the Delete() fix and ensure consistency—replace occurrences of provider.Spec.DatabaseName with r.DatabaseName in the Create() method (affecting the connection call and subsequent statements that reference the DB name), and leave Dependencies() and ReplicationSlotResource.Delete() unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/api/apiv1/validate_test.go`:
- Around line 773-774: The test's expected error message prefixes are wrong:
replace the "users[...]" prefixes in the assertions with "database_users[...]"
to match the validator output; update the two strings currently written as
"users[1]: usernames must be unique within a database" and "users[3]: cannot
have multiple users with db_owner = true" to "database_users[1]: usernames must
be unique within a database" and "database_users[3]: cannot have multiple users
with db_owner = true" so the assertions in validate_test.go (the expected error
messages array) match the validator's actual keys.
In `@server/internal/api/apiv1/validate.go`:
- Around line 360-368: The code currently checks hasOwner for every user and
appends a duplicate-owner error before verifying the current user's DbOwner
flag; update the condition so the duplicate-owner error is only raised when the
current user actually has DbOwner set to true. Concretely, modify the block that
constructs the error (the one using hasOwner, newValidationError, errs) to only
execute when user.DbOwner != nil && *user.DbOwner && hasOwner (or move that
block inside the subsequent if that sets hasOwner), leaving
seenNames.Add(user.Username) and the hasOwner = true assignment intact.
In `@server/internal/database/postgres_database.go`:
- Around line 28-35: The JSON tag for the HasRestoreConfig field in the
PostgresDatabaseResource struct is misspelled as `json:"hast_restore_config"`;
update the struct definition so the HasRestoreConfig field uses the correct tag
`json:"has_restore_config"` (fix the typo in the tag for the HasRestoreConfig
field) to avoid baking the incorrect key into persisted resource payloads and
snapshots.
In `@server/internal/database/subscription_resource.go`:
- Around line 25-31: SubscriptionResource must remain backward-compatible: keep
Identifier() as-is (old resource key) and ensure deserialized payloads without
database_name or with ExtraDependencies stored under dependent_subscriptions
still work by (1) adding a fallback in Dependencies() and providerDSN() to use
r.Subscriber.Spec.DatabaseName when r.DatabaseName == "" and (2) preserving or
adding compatibility for ExtraDependencies by accepting the legacy JSON key
"dependent_subscriptions" (either via struct tag or UnmarshalJSON) so
ExtraDependencies isn't lost after reload; update SubscriptionResource (fields
DatabaseName, ExtraDependencies), Dependencies(), and providerDSN() accordingly.
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 759-761: Update the error message to refer to the Instance
resource instead of a node: when the call to
resource.FromContext[*database.InstanceResource](rc,
database.InstanceResourceIdentifier(fromInstanceID)) fails, change the
fmt.Errorf message from "failed to get node resource: %w" to something like
"failed to get instance resource: %w" so logs correctly identify the missing
InstanceResource (referencing resource.FromContext, database.InstanceResource,
and InstanceResourceIdentifier to locate the call).
In `@server/internal/postgres/create_db.go`:
- Around line 563-576: StartRepairModeTxn opens a transaction with conn.Begin
but returns on subsequent setup failures without closing it; before returning
after errors from IsSpockEnabled().Scalar or EnableRepairMode().Exec, call
tx.Rollback(ctx) (and ignore or log its error) to ensure the begun transaction
is cleaned up; update the error return paths in StartRepairModeTxn to perform a
rollback on tx prior to returning the wrapped error so the connection isn't left
with an open transaction.
---
Outside diff comments:
In `@server/internal/postgres/roles.go`:
- Around line 98-111: The existence check in CreateApplicationReadOnlyRole is
querying for 'pgedge_application' but the function creates
'pgedge_application_read_only', so change the Query SQL in
CreateApplicationReadOnlyRole to check for NOT EXISTS ... WHERE rolname =
'pgedge_application_read_only' (so the ConditionalStatement's If Query matches
the actual role created) and ensure the Then Statement remains CREATE ROLE
pgedge_application_read_only WITH NOLOGIN; to avoid the creation depending on
the wrong role name.
---
Duplicate comments:
In `@server/internal/database/instance_resource.go`:
- Around line 137-143: The error message when ConnectToInstance fails uses
r.Spec.DatabaseName but should reference the actual target database parameter
dbName; update the fmt.Errorf call in the Connection() function (the block that
calls ConnectToInstance with ConnectionOptions and
r.ConnectionInfo.AdminDSN(dbName)) to interpolate dbName instead of
r.Spec.DatabaseName so the error accurately reports the admin DB being connected
to.
In `@server/internal/database/operations/common.go`:
- Around line 83-85: The error message after calling state.AddResource(db)
incorrectly references a node resource; update the error to name the actual
resource being added (a PostgresDatabaseResource) so debugging points to the
database path. Locate the state.AddResource(db) call and change the fmt.Errorf
text from "failed to add node resources to state: %w" to something like "failed
to add database resource to state: %w" (optionally include db identifier if
available) to clearly reference the PostgresDatabaseResource being added.
In `@server/internal/database/spec.go`:
- Around line 587-593: The loop that sets owner by taking the first
s.DatabaseUsers with DBOwner is unsafe; instead, count and collect DBOwner
entries from s.DatabaseUsers, if count != 1 return or propagate an error (or
fail validation) and only then set the DatabaseOwner field to that sole
user.Username; apply the same exact validation/assignment change for the other
occurrence that currently picks an owner (the second block using DBOwner at the
later assignment).
---
Nitpick comments:
In `@server/internal/database/lag_tracker_commit_ts_resource.go`:
- Around line 38-43: The constructor NewLagTrackerCommitTimestampResource
currently doesn't set the DatabaseName field but Dependencies() relies on it;
modify NewLagTrackerCommitTimestampResource to accept a databaseName string
parameter and assign it to the returned LagTrackerCommitTimestampResource's
DatabaseName field, and update all callers to pass the database name;
alternatively (if you prefer minimal changes) document and enforce via comment
that callers must set DatabaseName immediately after construction, but the
preferred fix is adding the databaseName parameter and assigning it inside
NewLagTrackerCommitTimestampResource so Dependencies() always has a valid
DatabaseName.
In `@server/internal/database/replication_slot_advance_from_cts_resource.go`:
- Around line 85-89: The Create() method is still using
provider.Spec.DatabaseName when opening connections and operating on the DB;
update Create() to use the resource's r.DatabaseName (the same field used in
Dependencies()) for all connection and operation calls to match the Delete() fix
and ensure consistency—replace occurrences of provider.Spec.DatabaseName with
r.DatabaseName in the Create() method (affecting the connection call and
subsequent statements that reference the DB name), and leave Dependencies() and
ReplicationSlotResource.Delete() unchanged.
In `@server/internal/postgres/roles.go`:
- Around line 152-162: The Schemas() implementation in
BuiltinRolePrivilegeOptions duplicates BuiltinRoleOptions.Schemas(); extract the
shared logic into a single helper (e.g., a package-level function like
combinedSchemas(extra []string) or a small embedded struct with Schemas()) that
appends defaultSchemas and the provided extra list, then have both
BuiltinRolePrivilegeOptions.Schemas() and BuiltinRoleOptions.Schemas() call that
helper; reference the existing symbols BuiltinRolePrivilegeOptions.Schemas,
BuiltinRoleOptions.Schemas, and defaultSchemas when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36c456ed-9b71-48d1-a8f2-550e9af59c31
📒 Files selected for processing (66)
docker/control-plane-dev/config.jsonserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/populate_nodes_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/postgres_database.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/resources.goserver/internal/database/spec.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/monitor/instance_monitor_resource.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/postgres/create_db.goserver/internal/postgres/dsn.goserver/internal/postgres/errors.goserver/internal/postgres/roles.goserver/internal/postgres/roles_test.goserver/internal/resource/event_test.goserver/internal/resource/resource.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/activities/executor.goserver/internal/workflows/common.goserver/internal/workflows/plan_restore.go
💤 Files with no reviewable changes (1)
- server/internal/postgres/roles_test.go
✅ Files skipped from review due to trivial changes (1)
- server/internal/database/sync_event_resource.go
🚧 Files skipped from review as they are similar to previous changes (18)
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.json
- server/internal/database/operations/add_nodes.go
- server/internal/workflows/activities/executor.go
- server/internal/monitor/instance_monitor_resource.go
- server/internal/database/operations/helpers_test.go
- server/internal/database/operations/update_nodes.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.json
- server/internal/workflows/plan_restore.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.json
- server/internal/database/operations/end.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.json
- server/internal/database/operations/restore_database.go
- server/internal/resource/resource.go
- server/internal/database/resources.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.json
- server/internal/resource/event_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.json
mmols
reviewed
Mar 19, 2026
| if d.SSLRootCert != "" { | ||
| fields = append(fields, fmt.Sprintf("sslrootcert=%s", d.SSLRootCert)) | ||
| } | ||
| if d.ApplicationName != "" { |
Member
There was a problem hiding this comment.
I see we set this for AdminDSN, should we set it for other DSNs so we can differentiate things in pg_stat_activity and other views?
Member
Author
There was a problem hiding this comment.
Yeah, I think that's a good idea for anything that doesn't already set one. As of this PR, every Control Plane and (almost) every Spock connection has an application name set.
Every connection originating from the Control Plane uses AdminDSN (although I'll need to keep rebasing and catching up with the stuff going into main), so every one of our connections has application_name=control-plane. Spock is the only other consumer of this output, and it sets its own application name for each component, like: spock supervisor, spock manager 16429, and spock_failover_slots worker. I do notice that it doesn't set one for the logical replication worker, but there might be a good reason for that.
mmols
reviewed
Mar 19, 2026
|
|
||
| func (r *ServiceUserRole) TypeDependencies() []resource.Type { | ||
| return nil | ||
| return []resource.Type{database.ResourceTypePostgresDatabase} |
Member
There was a problem hiding this comment.
Does this block on all the database for all nodes? or just the current node?
Member
Author
There was a problem hiding this comment.
This makes it so the service user role isn't created until every node's database is ready. That's functionally equivalent to how the service deployment is already written, but I was thinking about when we incorporate service deployments into the resource model a little better.
That said, a few things have changed since I first wrote this - including the multi-host connection strings. Looking at this resource's current state in main, I think it will break when someone adds a node. We'll need to change its identifier to be per-node rather than per-service, and then, instead of this TypeDependencies() change, we'll list that node's PostgresDatabase in Dependencies.
Member
There was a problem hiding this comment.
I ran into an issue while testing the multi-host connection strings where the service role didn't land on all of the nodes, which sounds related to what you are describing (its creation may have fired too early...).
@rshoemaker is looking at that as a follow-up. (tagging you here so you see this).
Member
Author
There was a problem hiding this comment.
Ah yeah, that makes sense. I had suggested some changes to this resource in a different PR, but I didn't register that we were connecting to "postgres" rather than the user's database, so Spock isn't there to replicate the user to every node.
Even so, relying on replication is the wrong approach because it can break when someone adds a node. So, the right way to do it is to create the user on each node. In that approach, we can either connect to "postgres" like we are now or connect to the user's database and enable repair mode. I like that connecting to "postgres" is less code, so I'd stick with that.
Contributor
There was a problem hiding this comment.
Thanks - given the scope of the fix, I opened a separate Jira ticket for this bug rather than roll it into my PR for the other RO/RW service account changes.
mmols
reviewed
Mar 19, 2026
Member
mmols
left a comment
mmols
left a comment
There was a problem hiding this comment.
Everything seems sensible here - I mainly had questions and not any comments for concern. I think we should probably hold this back a bit, since you mentioned you were contemplating any required state migration. if that's not relevant for this one, LMK and we can work to merge it.
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
2 times, most recently
from
d62db24 to
761cc61
Compare
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/database/lag_tracker_commit_ts_resource.go (1)
38-43:⚠️ Potential issue | 🟡 MinorConstructor does not initialize
DatabaseNamefield.The
NewLagTrackerCommitTimestampResourceconstructor doesn't accept or setDatabaseName, but the struct field andIdentifier()method expect it. While the constructor is currently unused in the codebase (struct literals are preferred), this represents an incomplete API contract that could lead to bugs if used.Proposed fix
-func NewLagTrackerCommitTimestampResource(originNode, receiverNode string) *LagTrackerCommitTimestampResource { +func NewLagTrackerCommitTimestampResource(originNode, receiverNode, databaseName string) *LagTrackerCommitTimestampResource { return &LagTrackerCommitTimestampResource{ OriginNode: originNode, ReceiverNode: receiverNode, + DatabaseName: databaseName, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/lag_tracker_commit_ts_resource.go` around lines 38 - 43, The constructor NewLagTrackerCommitTimestampResource does not initialize the DatabaseName field used by LagTrackerCommitTimestampResource.Identifier(), so change the constructor to accept a databaseName parameter and set DatabaseName on the returned struct (i.e., set DatabaseName: databaseName in the returned LagTrackerCommitTimestampResource), and update any call sites to pass the DB name; reference: NewLagTrackerCommitTimestampResource, LagTrackerCommitTimestampResource, and Identifier().
♻️ Duplicate comments (2)
server/internal/database/spec.go (1)
604-610:⚠️ Potential issue | 🟠 MajorValidate exactly one
DBOwnerbefore assigningDatabaseOwner.Line 604 currently picks the first owner and Line 662 propagates it to every node. If no owner exists,
DatabaseOwneris empty; if multiple exist, result is input-order dependent. This should fail fast before building node instances.Proposed fix
func (s *Spec) NodeInstances() ([]*NodeInstances, error) { specVersion, err := ds.NewPgEdgeVersion(s.PostgresVersion, s.SpockVersion) if err != nil { return nil, fmt.Errorf("failed to parse version from spec: %w", err) } - var owner string + var owner string + ownerCount := 0 for _, user := range s.DatabaseUsers { if user.DBOwner { owner = user.Username - break + ownerCount++ } } + if ownerCount != 1 { + return nil, fmt.Errorf("expected exactly 1 db owner, got %d", ownerCount) + }Also applies to: 662-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec.go` around lines 604 - 610, Before constructing nodes, validate that exactly one user in s.DatabaseUsers has DBOwner set: iterate s.DatabaseUsers, count users with DBOwner and capture the Username; if count != 1 return an error (or fail fast) instead of using the first match. Replace the current first-match logic that assigns owner and later sets DatabaseOwner on nodes with this strict check and use the captured single owner Username when populating DatabaseOwner for node instances.server/internal/database/instance_resource.go (1)
122-145:⚠️ Potential issue | 🟡 MinorReport the database actually passed into
Connection().
ConnectToInstanceusesAdminDSN(dbName), but the error path still reportsr.Spec.DatabaseName. Calls that intentionally use"postgres"will log the wrong target and make connection failures harder to debug.Suggested fix
- return nil, fmt.Errorf("failed to connect to database %q: %w", r.Spec.DatabaseName, err) + return nil, fmt.Errorf("failed to connect to database %q: %w", dbName, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/instance_resource.go` around lines 122 - 145, The error message in Connection() incorrectly reports r.Spec.DatabaseName instead of the actual dbName passed in; update the ConnectToInstance error wrap to include the dbName parameter (the value used in r.ConnectionInfo.AdminDSN(dbName)) so failures from ConnectToInstance/ConnectToInstance(...) show the real target database, referencing the Connection function, AdminDSN(dbName), and r.Spec.DatabaseName for context.
🧹 Nitpick comments (4)
server/internal/resource/resource.go (1)
165-177: Include the failing element in the joined error.Every appended error currently has the same wrapper text, so once
errors.Joinreturns you can't tell which variadic resource failed to serialize. Adding the slice index or identifier would make startup/migration failures much easier to localize.💡 Suggested tweak
func ToResourceDataSlice(resources ...Resource) ([]*ResourceData, error) { var errs []error data := make([]*ResourceData, len(resources)) for i, res := range resources { d, err := ToResourceData(res) if err != nil { - errs = append(errs, fmt.Errorf("failed to convert resource to resource data: %w", err)) + errs = append(errs, fmt.Errorf("resource[%d]: %w", i, err)) + continue } data[i] = d }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/resource.go` around lines 165 - 177, ToResourceDataSlice: the wrapper error added when ToResourceData(res) fails lacks context about which input failed; change the error construction where errs is appended (inside the loop in ToResourceDataSlice, referencing ToResourceData and ResourceData) to include the failing element index and/or a stable identifier from res (e.g., res.ID or res.Name if available, otherwise use the loop index i) in the fmt.Errorf message so the joined error reports which resource failed to serialize.server/internal/resource/migrations/schematool/resolver_test.go (1)
5-55: Optional: extract resolver setup helper in tests.Line 6, Line 20, and Line 40 repeat the same
openRepoAtRef+newResolversetup. A tinymustResolver(t)helper would reduce boilerplate and keep tests focused on assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/migrations/schematool/resolver_test.go` around lines 5 - 55, Create a small test helper mustResolver(t *testing.T) that calls openRepoAtRef("../../../../", "HEAD") and newResolver(tree), calls t.Fatalf on any error, and returns the constructed *resolver; then replace the repeated setup in TestResolverModulePath, TestResolverIsInRepo, and TestResolverPackageIndex with a single call to mustResolver(t). Reference openRepoAtRef, newResolver, and the new helper mustResolver to locate where to change the repeated code.server/internal/database/wait_for_sync_event_resource.go (1)
44-50: Keep the polled database aligned with the new database-scoped identifier.
Identifier()andDependencies()now scope this resource byr.DatabaseName, butRefresh()still openssubscriber.Spec.DatabaseName. Using the resource field here keeps the sync-event lookup and the database being polled in lockstep.Suggested tweak
- subscriberConn, err := subscriber.Connection(ctx, rc, subscriber.Spec.DatabaseName) + subscriberConn, err := subscriber.Connection(ctx, rc, r.DatabaseName) if err != nil { - return fmt.Errorf("failed to connect to subscriber database %q: %w", subscriber.Spec.DatabaseName, err) + return fmt.Errorf("failed to connect to subscriber database %q: %w", r.DatabaseName, err) }Also applies to: 59-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/wait_for_sync_event_resource.go` around lines 44 - 50, The resource's Identifier() and Dependencies() were changed to use r.DatabaseName but Refresh() still opens the database using subscriber.Spec.DatabaseName; update WaitForSyncEventResource.Refresh to use r.DatabaseName when opening/looking up the subscriber database so the polled DB matches the resource identifier (also apply the same replacement for other occurrences in the Refresh block and the related logic between lines 59-76), ensuring SyncEventResourceIdentifier/WaitForSyncEventResourceIdentifier lookups and any database polling use r.DatabaseName rather than subscriber.Spec.DatabaseName.server/internal/resource/service.go (1)
58-68: Keepdatabase_idon the follow-up migration logs.These branches fall back to
s.logger, so the scopeddatabase_idfield disappears right where upgrade persistence and contention are logged. Reusing the per-state logger here would make migration issues much easier to correlate.As per coding guidelines, "Use structured JSON logging with zerolog throughout the codebase, with pretty-printing enabled in dev mode."
Also applies to: 88-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/service.go` around lines 58 - 68, The follow-up logging uses s.logger which drops the per-state scoped database_id field; replace these calls to s.logger.Debug()/Warn()/Info() with the already-created per-state scoped logger (the scoped logger used when operating on the state) so the database_id remains in logs, and keep the existing fields (e.g. Stringer("to_version", stored.State.Version)); do the same change for the other similar branch around lines 88-90 (replace s.logger.* with the per-state scoped logger and preserve the error and Stringer calls).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/database/operations/common.go`:
- Around line 66-79: When adding monitor.InstanceMonitorResource in the loop
(see state.AddResource(&monitor.InstanceMonitorResource{...})), also add that
monitor resource's identifier to db.ExtraDependencies so the Postgres database
resource waits for instance monitors; i.e., append the monitor resource's
Identifier (the same unique id produced for InstanceMonitorResource) to
db.ExtraDependencies in addition to the existing instance.DatabaseDependencies
handling and keep the state.AddResource call as-is so the monitor is registered
in state.
In `@server/internal/database/service.go`:
- Around line 333-344: The function GetStoredDatabaseState returns
ErrInstanceNotFound when s.store.Database.GetByKey(databaseID).Exec(ctx) yields
storage.ErrNotFound; change that to return ErrDatabaseNotFound instead. Locate
GetStoredDatabaseState and replace the ErrInstanceNotFound return with
ErrDatabaseNotFound while keeping the DatabaseStateUnknown sentinel and the
other error wrapping (fmt.Errorf) intact so only the not-found branch is
updated.
In `@server/internal/host/host.go`:
- Around line 174-176: The loop over hosts calls h.DefaultPgEdgeVersion.String()
without guarding against a nil DefaultPgEdgeVersion; add a nil check before
invoking String() (or skip/handle that host) to avoid a panic. Update the loop
that references hosts and DefaultPgEdgeVersion (where defaultVersions.Add(...)
is called) to test if h.DefaultPgEdgeVersion == nil and either continue, set a
sensible fallback, or log an error, then only call .String() when non-nil; keep
the subsequent supported := ds.NewSet[string]() logic unchanged.
In `@server/internal/monitor/instance_monitor.go`:
- Around line 63-66: When m.dbSvc.GetStoredDatabaseState(ctx, m.databaseID)
returns an error, we must call updateInstanceErrStatus to persist the monitor
error before returning; update the block in instance_monitor.go so that on error
you call m.updateInstanceErrStatus(ctx, m.databaseID, err) (or the exact
signature used) to record the failure and then return the wrapped error from
GetStoredDatabaseState; this ensures stale instance status/error fields are not
left behind.
In `@server/internal/postgres/dsn.go`:
- Around line 39-49: The addField function currently writes raw values into
libpq keyword/value pairs, which breaks DSNs when values contain whitespace,
single quotes, or backslashes; update addField (used by String()) to escape DSN
values per libpq rules before building "key=value" (e.g., wrap in single quotes
and escape internal single quotes and backslashes) so the resulting string
parses correctly with pgx.ParseConfig(); locate addField and modify its value
handling to produce properly quoted/escaped values rather than writing value raw
into fields.
In `@server/internal/resource/migrations/1_0_0.go`:
- Around line 161-182: The loop in migrateLagTrackerResources (and the similar
blocks at the other locations) erroneously ranges over new.ExtraDependencies
(which is empty) and writes into extraDeps by index; change these to iterate
old.ExtraDependencies, create extraDeps with capacity len(old.ExtraDependencies)
and use append to populate it (do not assign by index), update
new.ExtraDependencies to the marshaled form of extraDeps after the rewrite, and
move the json.Marshal(new) call to after you've rebuilt new.ExtraDependencies;
reference symbols: migrateLagTrackerResources, old.ExtraDependencies,
new.ExtraDependencies, extraDeps, and v1_0_0.WaitForSyncEventResourceIdentifier
for locating the code to change.
In
`@server/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.json`:
- Around line 144-203: The fixture is missing the migrated n3 entries, causing
dangling dependencies; add a new "n3" object under "database.node" mirroring the
structure of "n1"/"n2" (set identifier.id "n3", attributes.name "n3",
instance_ids ["instance-3"], primary_instance_id "instance-3", dependencies
pointing to database.instance id "instance-3", resource_version "1",
pending_deletion false, etc.), and also add matching entries under
"database.postgres_database" for "n3:test" (or whatever identifier other
sections reference) so the later references to instance-3 and n3:test resolve.
Ensure the new objects use the same keys and shape as the existing nodes
(needs_recreate, executor, identifier, attributes, dependencies,
type_dependencies, diff_ignore, resource_version, pending_deletion, error).
In `@server/internal/resource/migrations/schematool/inliner.go`:
- Around line 62-66: When returning fallback qualified type text (e.g.,
returning alias + "." + typeName in the block that calls
in.resolver.isInRepo/isExcludedFromInlining and in the similar fallback in
exprString), ensure the corresponding import is recorded into neededImports
before returning (use the package index from
in.resolver.packageIndexFor(importPath) or add the import entry for the alias),
or instead return an explicit error to fail fast for unsupported nodes; update
the code paths that return a qualified string fallback to either push the import
into neededImports (referencing in.resolver, packageIndexFor, alias, typeName,
and neededImports) or return an error so generated schemas remain compilable.
In `@server/internal/resource/migrations/schematool/output.go`:
- Around line 34-43: The merging of imports into mergedImports can silently
overwrite existing aliases when adding info.ResourcePkgAlias/info.FuncImports
inside the loop over types; update the merge logic in that block (the
mergedImports map population that references mergedImports, imports, types,
nt.IdentifierInfo, info.ResourcePkgAlias, info.ResourcePkgPath, and
info.FuncImports) to detect alias collisions: if an alias already exists with a
different path, generate a unique alias (e.g., append a numeric suffix until the
alias is free) and insert the mapping under that new alias (or alternatively
return/report an error), and consistently use that resolved alias for any
subsequent usage so generated code stays compilable.
In `@server/internal/resource/migrations/schematool/parser.go`:
- Around line 54-72: The importer collection currently writes all file imports
into a package-wide idx.Imports and uses path.Base(importPath) as the default
alias, which loses file-scoped context and miscomputes aliases for versioned
module paths; change the code that iterates f.Imports to build a per-file import
map (e.g., local variable fileImports map[string]string) instead of writing
directly to idx.Imports, populate alias using imp.Name when present (skipping
"_" and ".") and otherwise compute the default alias by stripping a trailing /vN
version segment from importPath (e.g., remove a final "/v" + digits) then taking
path.Base on the result, and finally store that per-file map into the parser
index structure (e.g., idx.FileImports[f.Name] or similar) so later selector
resolution uses file-scoped imports rather than a single pkg-wide idx.Imports.
In `@server/internal/resource/migrations/schematool/resolver.go`:
- Around line 88-119: The bug is that pkgDirFromImport only trims
r.modulePath+"/" and leaves importPath==r.modulePath unchanged, causing
packageIndexFor to try indexing the full module path; update pkgDirFromImport to
explicitly handle the module root (when importPath == r.modulePath) and return
the repo-root directory token (e.g. "." or an empty string consistent with how
indexPackageFromTree expects it), ensuring packageIndexFor and the r.cache use
that same token; reference functions: isInRepo, pkgDirFromImport,
packageIndexFor, indexPackageFromTree and the r.cache map when making this
change.
---
Outside diff comments:
In `@server/internal/database/lag_tracker_commit_ts_resource.go`:
- Around line 38-43: The constructor NewLagTrackerCommitTimestampResource does
not initialize the DatabaseName field used by
LagTrackerCommitTimestampResource.Identifier(), so change the constructor to
accept a databaseName parameter and set DatabaseName on the returned struct
(i.e., set DatabaseName: databaseName in the returned
LagTrackerCommitTimestampResource), and update any call sites to pass the DB
name; reference: NewLagTrackerCommitTimestampResource,
LagTrackerCommitTimestampResource, and Identifier().
---
Duplicate comments:
In `@server/internal/database/instance_resource.go`:
- Around line 122-145: The error message in Connection() incorrectly reports
r.Spec.DatabaseName instead of the actual dbName passed in; update the
ConnectToInstance error wrap to include the dbName parameter (the value used in
r.ConnectionInfo.AdminDSN(dbName)) so failures from
ConnectToInstance/ConnectToInstance(...) show the real target database,
referencing the Connection function, AdminDSN(dbName), and r.Spec.DatabaseName
for context.
In `@server/internal/database/spec.go`:
- Around line 604-610: Before constructing nodes, validate that exactly one user
in s.DatabaseUsers has DBOwner set: iterate s.DatabaseUsers, count users with
DBOwner and capture the Username; if count != 1 return an error (or fail fast)
instead of using the first match. Replace the current first-match logic that
assigns owner and later sets DatabaseOwner on nodes with this strict check and
use the captured single owner Username when populating DatabaseOwner for node
instances.
---
Nitpick comments:
In `@server/internal/database/wait_for_sync_event_resource.go`:
- Around line 44-50: The resource's Identifier() and Dependencies() were changed
to use r.DatabaseName but Refresh() still opens the database using
subscriber.Spec.DatabaseName; update WaitForSyncEventResource.Refresh to use
r.DatabaseName when opening/looking up the subscriber database so the polled DB
matches the resource identifier (also apply the same replacement for other
occurrences in the Refresh block and the related logic between lines 59-76),
ensuring SyncEventResourceIdentifier/WaitForSyncEventResourceIdentifier lookups
and any database polling use r.DatabaseName rather than
subscriber.Spec.DatabaseName.
In `@server/internal/resource/migrations/schematool/resolver_test.go`:
- Around line 5-55: Create a small test helper mustResolver(t *testing.T) that
calls openRepoAtRef("../../../../", "HEAD") and newResolver(tree), calls
t.Fatalf on any error, and returns the constructed *resolver; then replace the
repeated setup in TestResolverModulePath, TestResolverIsInRepo, and
TestResolverPackageIndex with a single call to mustResolver(t). Reference
openRepoAtRef, newResolver, and the new helper mustResolver to locate where to
change the repeated code.
In `@server/internal/resource/resource.go`:
- Around line 165-177: ToResourceDataSlice: the wrapper error added when
ToResourceData(res) fails lacks context about which input failed; change the
error construction where errs is appended (inside the loop in
ToResourceDataSlice, referencing ToResourceData and ResourceData) to include the
failing element index and/or a stable identifier from res (e.g., res.ID or
res.Name if available, otherwise use the loop index i) in the fmt.Errorf message
so the joined error reports which resource failed to serialize.
In `@server/internal/resource/service.go`:
- Around line 58-68: The follow-up logging uses s.logger which drops the
per-state scoped database_id field; replace these calls to
s.logger.Debug()/Warn()/Info() with the already-created per-state scoped logger
(the scoped logger used when operating on the state) so the database_id remains
in logs, and keep the existing fields (e.g. Stringer("to_version",
stored.State.Version)); do the same change for the other similar branch around
lines 88-90 (replace s.logger.* with the per-state scoped logger and preserve
the error and Stringer calls).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 710c2cf5-efc5-4866-b8ae-58edc335f6b8
⛔ Files ignored due to path filters (5)
api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**server/internal/resource/migrations/schematool/go.sumis excluded by!**/*.sum
📒 Files selected for processing (119)
api/apiv1/design/database.goclient/enums.godocker/control-plane-dev/config.jsonserver/cmd/root.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/app/app.goserver/internal/database/database.goserver/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/populate_nodes_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/postgres_database.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/resources.goserver/internal/database/service.goserver/internal/database/service_instance.goserver/internal/database/spec.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/ds/versions.goserver/internal/ds/versions_test.goserver/internal/host/host.goserver/internal/host/host_store.goserver/internal/host/host_test.goserver/internal/host/versions_test.goserver/internal/monitor/instance_monitor.goserver/internal/monitor/instance_monitor_resource.goserver/internal/orchestrator/swarm/images.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/postgres/create_db.goserver/internal/postgres/dsn.goserver/internal/postgres/errors.goserver/internal/postgres/roles.goserver/internal/postgres/roles_test.goserver/internal/resource/event_test.goserver/internal/resource/migrations/1_0_0.goserver/internal/resource/migrations/1_0_0_test.goserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.jsonserver/internal/resource/migrations/main_test.goserver/internal/resource/migrations/provide.goserver/internal/resource/migrations/schemas/v0_0_0/database.goserver/internal/resource/migrations/schemas/v1_0_0/database.goserver/internal/resource/migrations/schematool/.gitignoreserver/internal/resource/migrations/schematool/README.mdserver/internal/resource/migrations/schematool/git.goserver/internal/resource/migrations/schematool/git_test.goserver/internal/resource/migrations/schematool/go.modserver/internal/resource/migrations/schematool/identifier.goserver/internal/resource/migrations/schematool/identifier_test.goserver/internal/resource/migrations/schematool/inliner.goserver/internal/resource/migrations/schematool/inliner_test.goserver/internal/resource/migrations/schematool/integration_test.goserver/internal/resource/migrations/schematool/main.goserver/internal/resource/migrations/schematool/output.goserver/internal/resource/migrations/schematool/output_test.goserver/internal/resource/migrations/schematool/parser.goserver/internal/resource/migrations/schematool/parser_test.goserver/internal/resource/migrations/schematool/resolver.goserver/internal/resource/migrations/schematool/resolver_test.goserver/internal/resource/provide.goserver/internal/resource/resource.goserver/internal/resource/service.goserver/internal/resource/state.goserver/internal/resource/state_store.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/activities/executor.goserver/internal/workflows/activities/plan_refresh.goserver/internal/workflows/common.goserver/internal/workflows/plan_restore.goserver/internal/workflows/plan_update.go
💤 Files with no reviewable changes (4)
- server/internal/orchestrator/swarm/service_user_role.go
- api/apiv1/design/database.go
- server/internal/host/versions_test.go
- server/internal/postgres/roles_test.go
✅ Files skipped from review due to trivial changes (20)
- docker/control-plane-dev/config.json
- server/internal/resource/migrations/schematool/.gitignore
- server/internal/database/resources.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.json
- server/internal/database/service_instance.go
- server/internal/database/replication_slot_create_resource.go
- client/enums.go
- server/internal/database/switchover_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.json
- server/internal/workflows/activities/executor.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.json
- server/internal/resource/migrations/schematool/README.md
- server/internal/database/operations/update_nodes.go
- server/internal/database/operations/add_nodes.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.json
- server/internal/monitor/instance_monitor_resource.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.json
- server/internal/resource/migrations/schematool/go.mod
🚧 Files skipped from review as they are similar to previous changes (23)
- server/internal/database/operations/helpers_test.go
- server/internal/resource/event_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.json
- server/internal/postgres/errors.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.json
- server/internal/database/operations/add_nodes_test.go
- server/internal/workflows/plan_restore.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.json
- server/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.json
- server/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.json
- server/internal/api/apiv1/validate_test.go
- server/internal/database/replication_slot_advance_from_cts_resource.go
- server/internal/database/operations/restore_database_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.json
- server/internal/database/operations/populate_nodes_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.json
- server/internal/database/replication_slot_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.json
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
4 times, most recently
from
019f9bb to
cc66bd1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docker/control-plane-dev/Dockerfile (1)
1-10:⚠️ Potential issue | 🟠 MajorRun the container as a non-root user.
The image still runs as root because there is no
USERinstruction. Please drop privileges to reduce blast radius if the container is compromised.🔧 Suggested hardening patch
FROM golang:1.25.8 ENV CGO_ENABLED=0 RUN go install github.com/go-delve/delve/cmd/dlv@latest COPY ./entrypoint.sh /entrypoint.sh COPY ./control-plane /control-plane + +# Create and use an unprivileged runtime user +RUN useradd --create-home --uid 10001 appuser \ + && chown appuser:appuser /entrypoint.sh /control-plane +USER appuser ENTRYPOINT [ "/entrypoint.sh" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/control-plane-dev/Dockerfile` around lines 1 - 10, The Dockerfile runs as root—add a non-root user and switch to it: create a user (e.g. appuser) and group, chown the copied artifacts (/control-plane and /entrypoint.sh) to that user, ensure the entrypoint is executable, and add a USER instruction (USER appuser or USER <uid>:<gid>) before ENTRYPOINT so the container drops privileges; reference the Dockerfile COPY lines and the ENTRYPOINT to locate where to set ownership and the USER instruction.server/internal/database/operations/add_nodes_test.go (2)
316-317:⚠️ Potential issue | 🟡 MinorSame bug: use
tc.nameinstead oft.Name()for subtest naming.🐛 Proposed fix
- t.Run(t.Name(), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/add_nodes_test.go` around lines 316 - 317, In the subtest invocation using t.Run currently passing t.Name(), switch to using the table-case identifier (tc.name) so each subtest is named after the table entry; update the t.Run call inside the loop that iterates over tc (replace t.Name() with tc.name) to avoid duplicate/incorrect subtest names and ensure unique, descriptive test names.
129-129:⚠️ Potential issue | 🟡 MinorBug:
t.Name()returns parent test name, not subtest name.Using
t.Name()instead oftc.namecauses all subtests to share the same name (the parent test name), making test output confusing and potentially hiding failures.🐛 Proposed fix
- t.Run(t.Name(), func(t *testing.T) { + t.Run(tc.name, func(t *testing.T) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/add_nodes_test.go` at line 129, The subtests are being named with t.Name() (the parent test) instead of the individual case name; replace t.Run(t.Name(), ...) with t.Run(tc.name, ...) and ensure the loop variable is captured by value before creating the closure (e.g., add "tc := tc" or "name := tc.name" immediately inside the loop) so the subtest uses the correct tc.name for each iteration when calling t.Run and inside the func(t *testing.T).server/internal/database/operations/populate_nodes_test.go (1)
95-109:⚠️ Potential issue | 🟡 MinorMake these expectations assert the node-scoped database name, not the instance fixture's default.
These three cases still build
ReplicationSlotCreateResource/ReplicationSlotCreateResourceIdentifier(...)frominstance*.DatabaseName(). IfmakeInstance()does not already return"test", the expected state is wrong; if it does, the test still won't catch a regression wherePopulateNode(s)falls back to the instance database name instead ofNodeResources.DatabaseName.🧪 Suggested assertion fix
- &database.ReplicationSlotCreateResource{ - DatabaseName: instance1.DatabaseName(), + &database.ReplicationSlotCreateResource{ + DatabaseName: "test", SubscriberNode: "n3", ProviderNode: "n2", }, ... database.ReplicationSlotCreateResourceIdentifier( - instance1.DatabaseName(), + "test", "n2", "n3", ),Apply the same replacement in the analogous expectation blocks below.
Also applies to: 285-299, 420-440
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/populate_nodes_test.go` around lines 95 - 109, The test expectations currently use instance*.DatabaseName() when constructing ReplicationSlotCreateResource and ReplicationSlotCreateResourceIdentifier; change those to use the node-scoped name from the NodeResources struct used in the test (e.g., NodeResources.DatabaseName()) so the assertions expect the node-specific DB name rather than the instance fixture default; update all occurrences (including the blocks around ReplicationSlotCreateResource, ReplicationSlotCreateResourceIdentifier, and the analogous blocks at the other locations) so PopulateNode(s) is validated against NodeResources.DatabaseName rather than instance*.DatabaseName().
♻️ Duplicate comments (3)
server/internal/database/service.go (1)
333-344:⚠️ Potential issue | 🟡 MinorMinor: Inconsistent error message.
Line 340 returns an error message saying "failed to get stored instance" but this method retrieves database state, not instance state.
📝 Proposed fix
func (s *Service) GetStoredDatabaseState(ctx context.Context, databaseID string) (DatabaseState, error) { stored, err := s.store.Database. GetByKey(databaseID). Exec(ctx) if errors.Is(err, storage.ErrNotFound) { return DatabaseStateUnknown, ErrDatabaseNotFound } else if err != nil { - return DatabaseStateUnknown, fmt.Errorf("failed to get stored instance: %w", err) + return DatabaseStateUnknown, fmt.Errorf("failed to get stored database: %w", err) } return stored.State, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/service.go` around lines 333 - 344, The error message in GetStoredDatabaseState is misleading ("failed to get stored instance"); update the fmt.Errorf call inside Service.GetStoredDatabaseState to use a message that reflects retrieving the database state (for example "failed to get stored database state") while preserving the wrapped original error (using %w) and leaving the rest of the logic unchanged.server/internal/database/spec.go (1)
604-610:⚠️ Potential issue | 🟠 MajorReject specs with zero or multiple
DBOwnerusers.This still picks the first matching owner and silently accepts the zero-owner case. With
DatabaseOwnernow propagated into planning, ownership becomes input-order dependent, and Line 662 will stamp an empty or ambiguous owner onto everyNodeInstances.Proposed fix
- var owner string - for _, user := range s.DatabaseUsers { - if user.DBOwner { - owner = user.Username - break - } - } + var owners []string + for _, user := range s.DatabaseUsers { + if user.DBOwner { + owners = append(owners, user.Username) + } + } + if len(owners) != 1 { + return nil, fmt.Errorf("expected exactly one database owner, got %d", len(owners)) + } + owner := owners[0]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec.go` around lines 604 - 610, The loop that assigns owner from s.DatabaseUsers silently accepts zero or multiple DBOwner flags; change it to scan s.DatabaseUsers, collect all users with DBOwner==true (e.g., into a slice), and if the count != 1 return a validation error (reject the spec) describing either "no DBOwner found" or "multiple DBOwner users found" and include the offending usernames; only when exactly one owner is present set owner = that username and proceed to stamp NodeInstances. Reference the s.DatabaseUsers slice, the DBOwner field, the owner variable, and the subsequent NodeInstances stamping logic when making this change.server/internal/resource/migrations/1_0_0.go (1)
155-185:⚠️ Potential issue | 🟠 MajorMarshal the lag-tracker attributes after rebuilding
ExtraDependencies.
attrsis encoded beforenew.ExtraDependenciesis populated, so the stored JSON drops those extra dependencies even though the top-levelDependenciesslice includes them. That leaves the migrated attributes out of sync with the resource graph.🛠️ Minimal fix
- attrs, err := json.Marshal(new) - if err != nil { - return fmt.Errorf("failed to marshal new lag tracker resource: %w", err) - } extraDeps := make([]resource.Identifier, 0, len(old.ExtraDependencies)) for i, dep := range old.ExtraDependencies { if dep.Type != v0_0_0.ResourceTypeWaitForSyncEvent.String() { return fmt.Errorf("unexpected lag tracker extra dependency type: type=%s, id=%s", dep.Type, dep.ID) } @@ Type: extraDeps[i].Type.String(), }) } + attrs, err := json.Marshal(new) + if err != nil { + return fmt.Errorf("failed to marshal new lag tracker resource: %w", err) + } adds = append(adds, &resource.ResourceData{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/migrations/1_0_0.go` around lines 155 - 185, The attributes JSON is marshaled into attrs before new.ExtraDependencies is populated, causing the stored attributes to miss those dependencies; move the json.Marshal call for v1_0_0.LagTrackerCommitTimestampResource (attrs) to after the loop that builds new.ExtraDependencies (the loop using v0_0_0.WaitForSyncEventResourceIdentifier and appending v1_0_0.WaitForSyncEventResourceIdentifier), so that attrs reflects the rebuilt ExtraDependencies, then use that attrs when constructing the resource with v1_0_0.LagTrackerCommitTSIdentifier.
🧹 Nitpick comments (2)
server/internal/postgres/roles.go (1)
61-71: DropExtraSchemasfromBuiltinRoleOptions, or make the split explicit.After this refactor, the builders in this file only create roles, but
BuiltinRoleOptionsstill exposesExtraSchemasandSchemas(). That makes it easy for a caller to keep passing schema config here and get a silent no-op instead of remembering to also callGrantBuiltinRolePrivileges().Also applies to: 73-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/postgres/roles.go` around lines 61 - 71, BuiltinRoleOptions currently carries ExtraSchemas and a Schemas() method even though the builders in this file only create roles (not grant schema privileges), which leads to silent no-ops; either remove the ExtraSchemas field and Schemas() method from BuiltinRoleOptions (and update callers to pass schema lists explicitly to GrantBuiltinRolePrivileges()) or split the API by creating two distinct types (e.g., BuiltinRoleCreationOptions without ExtraSchemas and BuiltinRoleGrantOptions with ExtraSchemas/Schemas()) and update the role builder functions to accept only the creation options while GrantBuiltinRolePrivileges accepts the grant options; update all references to BuiltinRoleOptions, BuiltinRoleOptions.Schemas(), and usages where ExtraSchemas was passed so callers explicitly call GrantBuiltinRolePrivileges with the schema list.server/internal/database/operations/add_nodes_test.go (1)
157-161: MissingDatabaseNameinTestAddNodestest inputs.The
TestAddNodecases were updated to includeDatabaseName: "test", butTestAddNodescases are missing this field. IfDatabaseNameis now required for proper resource identity, these test cases should be updated for consistency.♻️ Proposed fix for first test case (apply similarly to others)
{ // This should look identical to the AddNode output. name: "one node with one instance", input: []*operations.NodeResources{ { + DatabaseName: "test", NodeName: "n1", InstanceResources: []*database.InstanceResources{n1Instance1}, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/operations/add_nodes_test.go` around lines 157 - 161, TestAddNodes test cases are missing the required DatabaseName on NodeResources (TestAddNode was updated to include DatabaseName: "test"); update each input in TestAddNodes to set DatabaseName: "test" on the operations.NodeResources entries (e.g., the first case where NodeName: "n1" and InstanceResources: []*database.InstanceResources{n1Instance1}) so resource identity matches the updated behavior used by TestAddNode and the code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/resource/migrations/1_0_0.go`:
- Around line 116-137: The migrated Postgres database resources created in
addDatabaseResources lack the prerequisite edges carried in fresh state
construction, so update addDatabaseResources to populate
v1_0_0.PostgresDatabaseResource.ExtraDependencies with the same dependency
identifiers used by NodeResources.databaseResourceState (i.e., include the
instance's prerequisite role/database identifiers), and ensure the
resource.ResourceData Dependencies/Attributes reflect those ExtraDependencies
when calling state.Add (references: addDatabaseResources,
v1_0_0.PostgresDatabaseResource, PostgresDatabaseResourceIdentifier,
NodeResources.databaseResourceState, resource.ResourceData).
In `@server/internal/resource/state.go`:
- Around line 27-35: Clone() and NewState() currently drop or share the
State.Version pointer causing cloned states to lose version info and NewState to
share CurrentVersion; update Clone() to copy the Version pointer/value into the
cloned State (preserve State.Version when creating the new State in Clone) and
change NewState() to allocate a new Version value copied from CurrentVersion
instead of assigning the pointer directly; ensure ValidateVersion() will then
read the preserved version and avoid ErrStateNeedsUpgrade/persisting null.
---
Outside diff comments:
In `@docker/control-plane-dev/Dockerfile`:
- Around line 1-10: The Dockerfile runs as root—add a non-root user and switch
to it: create a user (e.g. appuser) and group, chown the copied artifacts
(/control-plane and /entrypoint.sh) to that user, ensure the entrypoint is
executable, and add a USER instruction (USER appuser or USER <uid>:<gid>) before
ENTRYPOINT so the container drops privileges; reference the Dockerfile COPY
lines and the ENTRYPOINT to locate where to set ownership and the USER
instruction.
In `@server/internal/database/operations/add_nodes_test.go`:
- Around line 316-317: In the subtest invocation using t.Run currently passing
t.Name(), switch to using the table-case identifier (tc.name) so each subtest is
named after the table entry; update the t.Run call inside the loop that iterates
over tc (replace t.Name() with tc.name) to avoid duplicate/incorrect subtest
names and ensure unique, descriptive test names.
- Line 129: The subtests are being named with t.Name() (the parent test) instead
of the individual case name; replace t.Run(t.Name(), ...) with t.Run(tc.name,
...) and ensure the loop variable is captured by value before creating the
closure (e.g., add "tc := tc" or "name := tc.name" immediately inside the loop)
so the subtest uses the correct tc.name for each iteration when calling t.Run
and inside the func(t *testing.T).
In `@server/internal/database/operations/populate_nodes_test.go`:
- Around line 95-109: The test expectations currently use
instance*.DatabaseName() when constructing ReplicationSlotCreateResource and
ReplicationSlotCreateResourceIdentifier; change those to use the node-scoped
name from the NodeResources struct used in the test (e.g.,
NodeResources.DatabaseName()) so the assertions expect the node-specific DB name
rather than the instance fixture default; update all occurrences (including the
blocks around ReplicationSlotCreateResource,
ReplicationSlotCreateResourceIdentifier, and the analogous blocks at the other
locations) so PopulateNode(s) is validated against NodeResources.DatabaseName
rather than instance*.DatabaseName().
---
Duplicate comments:
In `@server/internal/database/service.go`:
- Around line 333-344: The error message in GetStoredDatabaseState is misleading
("failed to get stored instance"); update the fmt.Errorf call inside
Service.GetStoredDatabaseState to use a message that reflects retrieving the
database state (for example "failed to get stored database state") while
preserving the wrapped original error (using %w) and leaving the rest of the
logic unchanged.
In `@server/internal/database/spec.go`:
- Around line 604-610: The loop that assigns owner from s.DatabaseUsers silently
accepts zero or multiple DBOwner flags; change it to scan s.DatabaseUsers,
collect all users with DBOwner==true (e.g., into a slice), and if the count != 1
return a validation error (reject the spec) describing either "no DBOwner found"
or "multiple DBOwner users found" and include the offending usernames; only when
exactly one owner is present set owner = that username and proceed to stamp
NodeInstances. Reference the s.DatabaseUsers slice, the DBOwner field, the owner
variable, and the subsequent NodeInstances stamping logic when making this
change.
In `@server/internal/resource/migrations/1_0_0.go`:
- Around line 155-185: The attributes JSON is marshaled into attrs before
new.ExtraDependencies is populated, causing the stored attributes to miss those
dependencies; move the json.Marshal call for
v1_0_0.LagTrackerCommitTimestampResource (attrs) to after the loop that builds
new.ExtraDependencies (the loop using v0_0_0.WaitForSyncEventResourceIdentifier
and appending v1_0_0.WaitForSyncEventResourceIdentifier), so that attrs reflects
the rebuilt ExtraDependencies, then use that attrs when constructing the
resource with v1_0_0.LagTrackerCommitTSIdentifier.
---
Nitpick comments:
In `@server/internal/database/operations/add_nodes_test.go`:
- Around line 157-161: TestAddNodes test cases are missing the required
DatabaseName on NodeResources (TestAddNode was updated to include DatabaseName:
"test"); update each input in TestAddNodes to set DatabaseName: "test" on the
operations.NodeResources entries (e.g., the first case where NodeName: "n1" and
InstanceResources: []*database.InstanceResources{n1Instance1}) so resource
identity matches the updated behavior used by TestAddNode and the code under
test.
In `@server/internal/postgres/roles.go`:
- Around line 61-71: BuiltinRoleOptions currently carries ExtraSchemas and a
Schemas() method even though the builders in this file only create roles (not
grant schema privileges), which leads to silent no-ops; either remove the
ExtraSchemas field and Schemas() method from BuiltinRoleOptions (and update
callers to pass schema lists explicitly to GrantBuiltinRolePrivileges()) or
split the API by creating two distinct types (e.g., BuiltinRoleCreationOptions
without ExtraSchemas and BuiltinRoleGrantOptions with ExtraSchemas/Schemas())
and update the role builder functions to accept only the creation options while
GrantBuiltinRolePrivileges accepts the grant options; update all references to
BuiltinRoleOptions, BuiltinRoleOptions.Schemas(), and usages where ExtraSchemas
was passed so callers explicitly call GrantBuiltinRolePrivileges with the schema
list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faf996fb-8b12-4c45-b861-ff88313d7d1b
⛔ Files ignored due to path filters (5)
api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**server/internal/resource/migrations/schematool/go.sumis excluded by!**/*.sum
📒 Files selected for processing (122)
.circleci/config.ymlapi/apiv1/design/database.goclient/enums.godocker/control-plane-dev/Dockerfiledocker/control-plane-dev/config.jsongo.modserver/cmd/root.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/app/app.goserver/internal/database/database.goserver/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/populate_nodes_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/postgres_database.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/resources.goserver/internal/database/service.goserver/internal/database/service_instance.goserver/internal/database/spec.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/ds/versions.goserver/internal/ds/versions_test.goserver/internal/host/host.goserver/internal/host/host_store.goserver/internal/host/host_test.goserver/internal/host/versions_test.goserver/internal/monitor/instance_monitor.goserver/internal/monitor/instance_monitor_resource.goserver/internal/orchestrator/swarm/images.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/postgres/create_db.goserver/internal/postgres/dsn.goserver/internal/postgres/errors.goserver/internal/postgres/roles.goserver/internal/postgres/roles_test.goserver/internal/resource/event_test.goserver/internal/resource/migrations/1_0_0.goserver/internal/resource/migrations/1_0_0_test.goserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.jsonserver/internal/resource/migrations/main_test.goserver/internal/resource/migrations/provide.goserver/internal/resource/migrations/schemas/v0_0_0/database.goserver/internal/resource/migrations/schemas/v1_0_0/database.goserver/internal/resource/migrations/schematool/.gitignoreserver/internal/resource/migrations/schematool/README.mdserver/internal/resource/migrations/schematool/git.goserver/internal/resource/migrations/schematool/git_test.goserver/internal/resource/migrations/schematool/go.modserver/internal/resource/migrations/schematool/identifier.goserver/internal/resource/migrations/schematool/identifier_test.goserver/internal/resource/migrations/schematool/inliner.goserver/internal/resource/migrations/schematool/inliner_test.goserver/internal/resource/migrations/schematool/integration_test.goserver/internal/resource/migrations/schematool/main.goserver/internal/resource/migrations/schematool/output.goserver/internal/resource/migrations/schematool/output_test.goserver/internal/resource/migrations/schematool/parser.goserver/internal/resource/migrations/schematool/parser_test.goserver/internal/resource/migrations/schematool/resolver.goserver/internal/resource/migrations/schematool/resolver_test.goserver/internal/resource/provide.goserver/internal/resource/resource.goserver/internal/resource/service.goserver/internal/resource/state.goserver/internal/resource/state_store.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/activities/executor.goserver/internal/workflows/activities/plan_refresh.goserver/internal/workflows/common.goserver/internal/workflows/plan_restore.goserver/internal/workflows/plan_update.go
💤 Files with no reviewable changes (3)
- api/apiv1/design/database.go
- server/internal/postgres/roles_test.go
- server/internal/host/versions_test.go
✅ Files skipped from review due to trivial changes (32)
- .circleci/config.yml
- docker/control-plane-dev/config.json
- go.mod
- server/internal/database/resources.go
- client/enums.go
- server/internal/resource/migrations/schematool/.gitignore
- server/internal/monitor/instance_monitor_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.json
- server/internal/api/apiv1/validate_test.go
- server/internal/workflows/activities/plan_refresh.go
- server/internal/workflows/activities/executor.go
- server/internal/postgres/errors.go
- server/internal/host/host_test.go
- server/internal/resource/migrations/schematool/README.md
- server/internal/resource/migrations/provide.go
- server/internal/resource/migrations/schematool/git_test.go
- server/internal/host/host_store.go
- server/internal/resource/migrations/schematool/go.mod
- server/internal/resource/migrations/schematool/integration_test.go
- server/internal/resource/migrations/1_0_0_test.go
- server/internal/resource/migrations/schematool/git.go
- server/internal/resource/migrations/schematool/resolver_test.go
- server/internal/database/operations/add_nodes.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.json
- server/internal/resource/migrations/schematool/output.go
- server/internal/ds/versions_test.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.json
- server/internal/resource/migrations/schemas/v1_0_0/database.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json
- server/internal/resource/migrations/schemas/v0_0_0/database.go
🚧 Files skipped from review as they are similar to previous changes (38)
- server/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.json
- server/internal/resource/event_test.go
- server/internal/database/switchover_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.json
- server/internal/database/replication_slot_create_resource.go
- server/internal/api/apiv1/post_init_handlers.go
- server/internal/workflows/activities/apply_event.go
- server/internal/workflows/plan_restore.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.json
- server/internal/database/operations/helpers_test.go
- server/internal/workflows/plan_update.go
- server/internal/database/operations/end.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.json
- server/internal/orchestrator/swarm/service_images_test.go
- server/internal/resource/migrations/main_test.go
- server/internal/resource/resource.go
- server/internal/database/operations/update_nodes_test.go
- server/internal/resource/state_store.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.json
- server/internal/orchestrator/swarm/service_images.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.json
- server/internal/database/wait_for_sync_event_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.json
- server/internal/database/replication_slot_advance_from_cts_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.json
- server/internal/database/operations/update_nodes.go
- server/internal/resource/migrations/schematool/parser.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.json
- server/internal/database/instance_resource.go
- server/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.json
- server/internal/orchestrator/swarm/service_user_role.go
- server/internal/database/postgres_database.go
- server/internal/resource/migrations/schematool/resolver.go
- server/internal/postgres/dsn.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker/control-plane-dev/Dockerfile (1)
1-10:⚠️ Potential issue | 🟠 MajorRun the container as a non-root user.
docker/control-plane-dev/Dockerfilecurrently runs as root (noUSERinstruction), which violates the DS-0002 hardening baseline and increases blast radius on compromise.🔒 Proposed hardening patch
FROM golang:1.25.8 ENV CGO_ENABLED=0 RUN go install github.com/go-delve/delve/cmd/dlv@latest COPY ./entrypoint.sh /entrypoint.sh COPY ./control-plane /control-plane + +RUN groupadd -r app && useradd -r -g app -m app \ + && chown app:app /entrypoint.sh /control-plane \ + && chmod 0755 /entrypoint.sh + +USER app ENTRYPOINT [ "/entrypoint.sh" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/control-plane-dev/Dockerfile` around lines 1 - 10, The Dockerfile currently runs as root; create a non-root user and group (e.g., "app" or "controlplane"), chown the copied files and any required runtime directories to that user, and add a USER instruction before the ENTRYPOINT to switch to the non-root account; adjust references to files created by COPY (like /entrypoint.sh and /control-plane) to ensure they are owned/executable by the new user and any RUN steps that install tools (dlv) do not rely on root-only runtime permissions.
♻️ Duplicate comments (3)
server/internal/database/service.go (1)
333-344:⚠️ Potential issue | 🟡 MinorMinor inconsistency in error message.
Line 340 wraps the error with "failed to get stored instance" but this method retrieves a database state. The error mapping to
ErrDatabaseNotFoundon line 338 is correct, but the wrap message should match.📝 Proposed fix
if errors.Is(err, storage.ErrNotFound) { return DatabaseStateUnknown, ErrDatabaseNotFound } else if err != nil { - return DatabaseStateUnknown, fmt.Errorf("failed to get stored instance: %w", err) + return DatabaseStateUnknown, fmt.Errorf("failed to get stored database: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/service.go` around lines 333 - 344, The error wrap message in GetStoredDatabaseState is inconsistent: when s.store.Database.GetByKey(databaseID).Exec(ctx) returns a non-notfound error, update the fmt.Errorf wrap from "failed to get stored instance: %w" to a database-specific message such as "failed to get stored database: %w" (keeping the same return types DatabaseStateUnknown and ErrDatabaseNotFound handling intact) so the error text matches the method purpose.server/internal/database/spec.go (1)
604-610:⚠️ Potential issue | 🟠 MajorStill reject zero or multiple
DBOwnerusers before stampingDatabaseOwner.This still picks the first
DBOwner == trueuser and falls back to""when none exist. That makes ownership input-order dependent, and Line 662 now propagates the wrong/empty owner to every node. Please fail whenlen(owners) != 1instead of selecting the first match.🐛 Proposed fix
- var owner string - for _, user := range s.DatabaseUsers { - if user.DBOwner { - owner = user.Username - break - } - } + owners := make([]string, 0, 1) + for _, user := range s.DatabaseUsers { + if user.DBOwner { + owners = append(owners, user.Username) + } + } + if len(owners) != 1 { + return nil, fmt.Errorf("expected exactly one database owner, got %d", len(owners)) + } + owner := owners[0]Also applies to: 661-663
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/database/spec.go` around lines 604 - 610, The current loop that picks the first user with DBOwner true (iterating s.DatabaseUsers and setting owner) is order-dependent and allows zero or multiple owners; change it to collect all usernames where user.DBOwner is true into an owners slice, then validate that len(owners) == 1 and return/fail if not; only when exactly one owner is found, set DatabaseOwner (the variable currently named owner) to owners[0] before stamping DatabaseOwner on nodes. Ensure this validation replaces the existing loop logic that assigns owner and prevents propagating an empty or ambiguous owner.server/internal/resource/state.go (1)
111-123:⚠️ Potential issue | 🟠 Major
Clone()does not preserveState.Version.The
Clone()method copiesResourcesbut omitsVersion. A cloned state will haveVersion: nil, which may causeValidateVersion()to treat it as version0.0.0and returnErrStateNeedsUpgradeunexpectedly.Proposed fix
func (s *State) Clone() *State { resources := make(map[Type]map[string]*ResourceData, len(s.Resources)) for t, byID := range s.Resources { resources[t] = make(map[string]*ResourceData, len(byID)) for i, resource := range byID { resources[t][i] = resource.Clone() } } + var version *ds.Version + if s.Version != nil { + version = s.Version.Clone() + } + return &State{ + Version: version, Resources: resources, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/state.go` around lines 111 - 123, State.Clone currently copies only Resources and omits the State.Version, causing clones to have a nil version and triggering ValidateVersion to treat it as 0.0.0; update State.Clone (the Clone method on type State) to preserve the original version by setting the returned State's Version to the source's Version—if Version is a pointer to a mutable struct, perform a deep copy (e.g., use a Version.Clone() method or allocate a new value and copy fields) instead of sharing the pointer so mutated versions don't affect the original.
🧹 Nitpick comments (2)
server/internal/resource/service.go (1)
153-165: Variable shadowing causes migrations to never execute.The loop variable
mshadows the receiverm *StateMigrations, som.Run(state)on line 161 calls the current migration'sRunmethod, which is correct. However, this shadowing is confusing and error-prone. Consider renaming the loop variable for clarity.Actually, on closer inspection, the code is functionally correct since
m.Run(state)on line 161 correctly calls theStateMigration.Run()method on the loop variable. The shadowing doesn't cause a bug here, but it reduces readability.♻️ Suggested improvement for clarity
func (m *StateMigrations) Run(state *State) error { - for _, m := range m.migrations { - version := m.Version() + for _, migration := range m.migrations { + version := migration.Version() if state.Version.Compare(version) >= 0 { // Skip migrations for older or current versions continue } - if err := m.Run(state); err != nil { + if err := migration.Run(state); err != nil { return fmt.Errorf("failed to upgrade database state to version '%s': %w", version, err) } state.Version = version } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/resource/service.go` around lines 153 - 165, The loop variable `m` shadows the receiver `m *StateMigrations`, harming readability; rename the loop variable (e.g., to `mig` or `migration`) in the Run method of StateMigrations that iterates over m.migrations so references like migration.Version(), migration.Run(state) and assigning state.Version = version remain unambiguous while preserving existing logic and error handling.server/internal/database/postgres_database.go (1)
237-263: Retry logic for flaky rename operation.The retry logic (3 attempts, 500ms backoff) is reasonable for handling transient connection issues during database rename. However, the retry will also retry on permanent errors (e.g., permission denied, database doesn't exist).
Consider whether permanent errors should be distinguished from transient ones to fail fast. That said, given the controlled environment and limited retry count, this is acceptable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/database/sync_event_resource.go`:
- Around line 15-18: The resource is database-scoped but the provider connection
still uses provider.Spec.DatabaseName; replace that with the resource instance's
DatabaseName so the sync query uses r.DatabaseName. Find the code that
constructs the provider connection (the call that currently passes
provider.Spec.DatabaseName) and change it to use r.DatabaseName instead, and
update any other occurrences (around the blocks referenced at lines 23 and
42–48) so the identifier created by SyncEventResourceIdentifier and the sync
query consistently use r.DatabaseName rather than provider.Spec.DatabaseName.
In `@server/internal/resource/service.go`:
- Line 38: The comment "Errors in this process at startup. are considered
non-fatal" contains an extra period; edit that comment to read "Errors in this
process at startup are considered non-fatal" (replace the misplaced period).
Locate the exact comment string in server/internal/resource/service.go and
update it accordingly, then save and run gofmt/lint to ensure comment formatting
is clean.
---
Outside diff comments:
In `@docker/control-plane-dev/Dockerfile`:
- Around line 1-10: The Dockerfile currently runs as root; create a non-root
user and group (e.g., "app" or "controlplane"), chown the copied files and any
required runtime directories to that user, and add a USER instruction before the
ENTRYPOINT to switch to the non-root account; adjust references to files created
by COPY (like /entrypoint.sh and /control-plane) to ensure they are
owned/executable by the new user and any RUN steps that install tools (dlv) do
not rely on root-only runtime permissions.
---
Duplicate comments:
In `@server/internal/database/service.go`:
- Around line 333-344: The error wrap message in GetStoredDatabaseState is
inconsistent: when s.store.Database.GetByKey(databaseID).Exec(ctx) returns a
non-notfound error, update the fmt.Errorf wrap from "failed to get stored
instance: %w" to a database-specific message such as "failed to get stored
database: %w" (keeping the same return types DatabaseStateUnknown and
ErrDatabaseNotFound handling intact) so the error text matches the method
purpose.
In `@server/internal/database/spec.go`:
- Around line 604-610: The current loop that picks the first user with DBOwner
true (iterating s.DatabaseUsers and setting owner) is order-dependent and allows
zero or multiple owners; change it to collect all usernames where user.DBOwner
is true into an owners slice, then validate that len(owners) == 1 and
return/fail if not; only when exactly one owner is found, set DatabaseOwner (the
variable currently named owner) to owners[0] before stamping DatabaseOwner on
nodes. Ensure this validation replaces the existing loop logic that assigns
owner and prevents propagating an empty or ambiguous owner.
In `@server/internal/resource/state.go`:
- Around line 111-123: State.Clone currently copies only Resources and omits the
State.Version, causing clones to have a nil version and triggering
ValidateVersion to treat it as 0.0.0; update State.Clone (the Clone method on
type State) to preserve the original version by setting the returned State's
Version to the source's Version—if Version is a pointer to a mutable struct,
perform a deep copy (e.g., use a Version.Clone() method or allocate a new value
and copy fields) instead of sharing the pointer so mutated versions don't affect
the original.
---
Nitpick comments:
In `@server/internal/resource/service.go`:
- Around line 153-165: The loop variable `m` shadows the receiver `m
*StateMigrations`, harming readability; rename the loop variable (e.g., to `mig`
or `migration`) in the Run method of StateMigrations that iterates over
m.migrations so references like migration.Version(), migration.Run(state) and
assigning state.Version = version remain unambiguous while preserving existing
logic and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b12db8e2-3372-4879-ab33-886509e7131b
⛔ Files ignored due to path filters (5)
api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**server/internal/resource/migrations/schematool/go.sumis excluded by!**/*.sum
📒 Files selected for processing (122)
.circleci/config.ymlapi/apiv1/design/database.goclient/enums.godocker/control-plane-dev/Dockerfiledocker/control-plane-dev/config.jsongo.modserver/cmd/root.goserver/internal/api/apiv1/post_init_handlers.goserver/internal/api/apiv1/validate.goserver/internal/api/apiv1/validate_test.goserver/internal/app/app.goserver/internal/database/database.goserver/internal/database/instance_resource.goserver/internal/database/lag_tracker_commit_ts_resource.goserver/internal/database/node_resource.goserver/internal/database/operations/add_nodes.goserver/internal/database/operations/add_nodes_test.goserver/internal/database/operations/common.goserver/internal/database/operations/end.goserver/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.jsonserver/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_rolling.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/remove_two_nodes_from_three_node_database.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.jsonserver/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.jsonserver/internal/database/operations/helpers_test.goserver/internal/database/operations/populate_nodes.goserver/internal/database/operations/populate_nodes_test.goserver/internal/database/operations/restore_database.goserver/internal/database/operations/restore_database_test.goserver/internal/database/operations/update_database_test.goserver/internal/database/operations/update_nodes.goserver/internal/database/operations/update_nodes_test.goserver/internal/database/orchestrator.goserver/internal/database/postgres_database.goserver/internal/database/replication_slot_advance_from_cts_resource.goserver/internal/database/replication_slot_create_resource.goserver/internal/database/replication_slot_resource.goserver/internal/database/resources.goserver/internal/database/service.goserver/internal/database/service_instance.goserver/internal/database/spec.goserver/internal/database/subscription_resource.goserver/internal/database/switchover_resource.goserver/internal/database/sync_event_resource.goserver/internal/database/wait_for_sync_event_resource.goserver/internal/ds/versions.goserver/internal/ds/versions_test.goserver/internal/host/host.goserver/internal/host/host_store.goserver/internal/host/host_test.goserver/internal/host/versions_test.goserver/internal/monitor/instance_monitor.goserver/internal/monitor/instance_monitor_resource.goserver/internal/orchestrator/swarm/images.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_images_test.goserver/internal/orchestrator/swarm/service_user_role.goserver/internal/postgres/create_db.goserver/internal/postgres/dsn.goserver/internal/postgres/errors.goserver/internal/postgres/roles.goserver/internal/postgres/roles_test.goserver/internal/resource/event_test.goserver/internal/resource/migrations/1_0_0.goserver/internal/resource/migrations/1_0_0_test.goserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.jsonserver/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.jsonserver/internal/resource/migrations/main_test.goserver/internal/resource/migrations/provide.goserver/internal/resource/migrations/schemas/v0_0_0/database.goserver/internal/resource/migrations/schemas/v1_0_0/database.goserver/internal/resource/migrations/schematool/.gitignoreserver/internal/resource/migrations/schematool/README.mdserver/internal/resource/migrations/schematool/git.goserver/internal/resource/migrations/schematool/git_test.goserver/internal/resource/migrations/schematool/go.modserver/internal/resource/migrations/schematool/identifier.goserver/internal/resource/migrations/schematool/identifier_test.goserver/internal/resource/migrations/schematool/inliner.goserver/internal/resource/migrations/schematool/inliner_test.goserver/internal/resource/migrations/schematool/integration_test.goserver/internal/resource/migrations/schematool/main.goserver/internal/resource/migrations/schematool/output.goserver/internal/resource/migrations/schematool/output_test.goserver/internal/resource/migrations/schematool/parser.goserver/internal/resource/migrations/schematool/parser_test.goserver/internal/resource/migrations/schematool/resolver.goserver/internal/resource/migrations/schematool/resolver_test.goserver/internal/resource/provide.goserver/internal/resource/resource.goserver/internal/resource/service.goserver/internal/resource/state.goserver/internal/resource/state_store.goserver/internal/workflows/activities/apply_event.goserver/internal/workflows/activities/executor.goserver/internal/workflows/activities/plan_refresh.goserver/internal/workflows/common.goserver/internal/workflows/plan_restore.goserver/internal/workflows/plan_update.go
💤 Files with no reviewable changes (4)
- server/internal/orchestrator/swarm/service_user_role.go
- server/internal/postgres/roles_test.go
- api/apiv1/design/database.go
- server/internal/host/versions_test.go
✅ Files skipped from review due to trivial changes (35)
- server/internal/resource/event_test.go
- server/internal/monitor/instance_monitor_resource.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/empty.json
- docker/control-plane-dev/config.json
- go.mod
- .circleci/config.yml
- server/internal/resource/migrations/schematool/.gitignore
- server/internal/workflows/activities/executor.go
- server/internal/database/operations/restore_database_test.go
- server/internal/resource/migrations/schematool/go.mod
- server/internal/api/apiv1/validate_test.go
- server/internal/resource/migrations/schematool/git_test.go
- server/internal/host/host_store.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/no_nodes.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/with_restore_config.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/single_node_with_replicas.json
- server/internal/resource/migrations/schematool/resolver_test.go
- server/internal/resource/migrations/schematool/README.md
- server/internal/resource/migrations/schematool/output_test.go
- server/internal/resource/migrations/schematool/parser_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/adding_a_replica.json
- server/internal/resource/migrations/schematool/git.go
- server/internal/orchestrator/swarm/images.go
- server/internal/host/host_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/multiple_nodes_from_empty.json
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/three_nodes.json
- server/internal/ds/versions_test.go
- server/internal/resource/migrations/golden_test/TestVersion_1_0_0/populate_n3_with_n1_source.json
- server/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db.json
- server/internal/database/replication_slot_advance_from_cts_resource.go
- server/internal/resource/migrations/schematool/identifier_test.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_from_empty.json
- server/internal/resource/migrations/1_0_0_test.go
- server/internal/resource/migrations/schemas/v0_0_0/database.go
- server/internal/resource/migrations/schemas/v1_0_0/database.go
🚧 Files skipped from review as they are similar to previous changes (43)
- server/internal/database/resources.go
- server/internal/postgres/errors.go
- server/cmd/root.go
- server/internal/api/apiv1/post_init_handlers.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/forced_update.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_one_node_from_three_node_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/single_node_with_service_from_empty.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/remove_node.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/delete_two_node_database.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/update_database_node_with_unchanged_service.json
- client/enums.go
- server/internal/database/operations/end.go
- server/internal/monitor/instance_monitor.go
- server/internal/resource/migrations/provide.go
- server/internal/resource/resource.go
- server/internal/resource/state_store.go
- server/internal/resource/migrations/main_test.go
- server/internal/workflows/activities/apply_event.go
- server/internal/workflows/activities/plan_refresh.go
- server/internal/database/operations/add_nodes_test.go
- server/internal/workflows/plan_update.go
- server/internal/database/database.go
- server/internal/database/replication_slot_create_resource.go
- server/internal/resource/provide.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_an_instance_dependency_with_forced_update.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_default_source_node.json
- server/internal/ds/versions.go
- server/internal/database/wait_for_sync_event_resource.go
- server/internal/orchestrator/swarm/service_images.go
- server/internal/orchestrator/swarm/orchestrator.go
- server/internal/resource/migrations/schematool/integration_test.go
- server/internal/database/node_resource.go
- server/internal/database/operations/golden_test/TestUpdateDatabase/adding_multiple_replicas_concurrent.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/one_node_to_two_nodes_with_populate_and_replica.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/add_update_and_remove_node.json
- server/internal/resource/migrations/schematool/resolver.go
- server/internal/database/operations/golden_test/TestRestoreDatabase/single_node_restore_in_two-node_db_with_replica.json
- server/internal/database/operations/golden_test/TestUpdateDatabase/two_nodes_to_three_nodes_with_populate.json
- server/internal/resource/migrations/schematool/inliner_test.go
- server/internal/resource/migrations/schematool/inliner.go
- server/internal/database/operations/golden_test/TestRestoreDatabase/restore_all_nodes_in_two-node_db.json
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
cc66bd1 to
0777377
Compare
Adds a new `PostgresDatabaseResource` that shifts the Postgres database creation logic to happen later in the database creation process. Now, the `InstanceResource` is only responsible for creating the database users, and the `PostgresDatabase` resource is responsible for creating the Postgres database, granting role privileges to the new database, and initializing the Spock node. This change is necessary because we need to use `pg_service.conf` for SystemD support, and we need that file to exist when we create the Spock node. This change shifts the Spock node initialization until after all of the instances are created and all nodes are ready, so we're able to construct the DSNs for all nodes before we initialize Spock. This change has an added benefit that it opens up the possibility of support for multiple Postgres databases per Control Plane database since we're able to make multiple databases/spock nodes per `NodeResource`/Patroni cluster. I've added the `DatabaseName` field to most, if not all, of the resources that would need to change to accommodate this. A subsequent commit will add a migration system that will safely upgrade existing databases with these new changes. PLAT-417
Moves the version types to the ds package so that they can be used in more places without creating a dependency on the `host` package. We'll rename the `ds` package to something less specific, like `types` in a future commit.
Adds a CLI tool, called `schematool`, that takes a git ref, a package path, and a list of type names, and outputs the type definition with inlined structs for properties. This output is necessary for writing safe migrations that will be unaffected by future code changes.
Adds a system to version resource states and to migrate them to newer versions. These migrations run when the server starts as well as when they're retrieved from storage in order to handle cases where different server versions are started simultaneously.
jason-lynch
force-pushed
the
feat/PLAT-417/postgres-database-resource
branch
from
0777377 to
ce0a342
Compare
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
3 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.
Summary
Adds a new
PostgresDatabaseResourcethat shifts the Postgres database creation logic to happen later in the database creation process. Now, theInstanceResourceis only responsible for creating the database users, and thePostgresDatabaseresource is responsible for creating the Postgres database, granting role privileges to the new database, and initializing the Spock node.This change is necessary because we need to use
pg_service.conffor SystemD support, and we need that file to exist when we create the Spock node. This change shifts the Spock node initialization until after all of the instances are created and all nodes are ready, so we're able to construct the DSNs for all nodes before we initialize Spock.This change has an added benefit that it opens up the possibility of support for multiple Postgres databases per Control Plane database since we're able to make multiple databases/spock nodes per
NodeResource/Patroni cluster. I've added theDatabaseNamefield to most, if not all, of the resources that would need to change to accommodate this.Testing
There should be no difference in user-facing functionality.
PLAT-417