Skip to content

fix: use safe port range for local cluster postgres instances#353

Merged
rafael merged 10 commits intomultigres:mainfrom
ixchio:fix/local-cluster-port-assignment
Dec 23, 2025
Merged

fix: use safe port range for local cluster postgres instances#353
rafael merged 10 commits intomultigres:mainfrom
ixchio:fix/local-cluster-port-assignment

Conversation

@ixchio
Copy link
Contributor

@ixchio ixchio commented Dec 14, 2025

Fixes #351

Summary

This PR addresses two issues when bringing up a local cluster:

  1. Backend Postgres port conflict: Postgres instances now use ports 25432-25434 instead of 5432-5434, avoiding conflicts with local Postgres installations.

  2. Inconsistent Multigateway ports: Multigateway PG ports for zones 2 and 3 now correctly use 15433 and 15434 instead of incorrectly using 5433 and 5434.

Changes

File Change
ports/ports.go Added DefaultLocalPostgresPort = 25432
config.go Updated default config to use new port constants
local.go Updated fallback default
pgctld.go Updated fallback default
config_test.go Added TestDefaultConfigPorts to prevent regression

Port Assignments After This Fix

Zone Backend Postgres Multigateway PG
Zone 1 25432 15432
Zone 2 25433 15433
Zone 3 25434 15434

Testing

  • All existing tests pass
  • Added new test TestDefaultConfigPorts to verify correct port assignments

Fixes multigres#351

This change addresses two issues when bringing up a local cluster:

1. Backend Postgres instances now use ports 25432-25434 instead of
   5432-5434, avoiding conflicts with local Postgres installations.

2. Multigateway PG ports for zones 2 and 3 now correctly use 15433
   and 15434 instead of incorrectly using 5433 and 5434.

Changes:
- Added DefaultLocalPostgresPort (25432) constant to ports.go
- Updated DefaultConfig in config.go to use the new port constants
- Updated fallback defaults in local.go and pgctld.go
- Added TestDefaultConfigPorts test to prevent regression

Signed-off-by: amankumarpandeyin <[email protected]>
@ixchio ixchio force-pushed the fix/local-cluster-port-assignment branch from b048282 to 6a3da61 Compare December 15, 2025 14:15
@sougou sougou requested a review from rafael December 16, 2025 10:23
@coveralls
Copy link

coveralls commented Dec 17, 2025

Pull Request Test Coverage Report for Build 20451734589

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 3 files are covered.
  • 433 unchanged lines in 26 files lost coverage.
  • Overall coverage decreased (-0.3%) to 55.398%

Files with Coverage Reduction New Missed Lines %
go/common/topoclient/multipooler.go 1 86.1%
go/common/topoclient/memorytopo/file.go 2 77.73%
go/provisioner/local/local.go 2 77.12%
go/multigateway/planner/planner.go 3 81.08%
go/multiorch/recovery/actions/appoint_leader.go 3 11.27%
go/pgprotocol/client/conn.go 3 69.57%
go/provisioner/local/state.go 3 77.5%
go/multiorch/recovery/discovery.go 4 91.12%
go/pb/query/query.pb.go 4 13.68%
go/pgprotocol/client/startup.go 4 44.98%
Totals Coverage Status
Change from base Build 20444393454: -0.3%
Covered Lines: 42714
Relevant Lines: 77104

💛 - Coveralls

@ixchio ixchio force-pushed the fix/local-cluster-port-assignment branch from 0cb54bf to 1dc1637 Compare December 18, 2025 12:54
The second break attempt was racing with multiorch's recovery cycle,
causing intermittent failures. The first break/verify cycle already
proves the mechanism works; the second only needs to verify
repeatability of the fix.

Signed-off-by: amankumarpandeyin <[email protected]>
@rafael
Copy link
Collaborator

rafael commented Dec 22, 2025

This change looks good to me. Could you fix the DCO so we can merge?

…n_Standby_Fails test

The test doesn't need replication configured - it only validates that
ConfigureSynchronousReplication fails on a standby node. Without this
option, cleanup would timeout trying to restore replication state.

Signed-off-by: amankumarpandeyin <[email protected]>
@ixchio ixchio force-pushed the fix/local-cluster-port-assignment branch from 3f97522 to 1243835 Compare December 23, 2025 04:50
@ixchio
Copy link
Contributor Author

ixchio commented Dec 23, 2025

This change looks good to me. Could you fix the DCO so we can merge?

@rafael Done.......I’ve fixed the DCO and updated the commits.
Thanks for the review

Copy link
Collaborator

@rafael rafael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rafael rafael merged commit a470fc8 into multigres:main Dec 23, 2025
11 checks passed
@ixchio
Copy link
Contributor Author

ixchio commented Dec 23, 2025

All set. DCO fixed, commits updated, and tests are green.
Thanks @rafael for the careful review and quick turnaround

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Issues bringing up local cluster

3 participants