Skip to content

feat: add proxy topology mode for SPQR-compatible routing - #33

Merged
jhoncool merged 6 commits into
gravity-ui:mainfrom
Denchick:feat/proxy-topology-mode
Aug 11, 2026
Merged

jhoncool merged 6 commits into
gravity-ui:mainfrom
Denchick:feat/proxy-topology-mode

Conversation

@Denchick

Copy link
Copy Markdown
Contributor

Problem

PostgresKit currently assumes that every connection string represents an individual node in a PostgreSQL primary/replica cluster.

To determine each node's role, it periodically executes:

SELECT pg_is_in_recovery();

A false result identifies a primary, while true identifies a replica.

This assumption does not work for SPQR. The supplied endpoints represent equivalent SPQR router instances rather than individual PostgreSQL nodes. A router can accept both read and write queries and forward them to the appropriate backend.

SPQR routers report themselves as primary for pg_is_in_recovery(). Therefore, when several router endpoints are configured, PostgresKit classifies all of them as primaries and may report:

Multiple primary connections detected, something is wrong

or:

No alive replica available, using master for read

Having multiple endpoints classified as “masters” is normal for an SPQR deployment: these are equivalent router instances, not multiple writable PostgreSQL primary nodes. The warnings are produced by PostgresKit because its primary/replica topology model does not match the actual SPQR topology.

Suppressing status logs does not solve the problem because it only hides the warnings while leaving the incorrect routing model in place.

Solution

This PR adds an optional dispatcher topology mode:

initDB({
  connectionString: process.env.POSTGRES_DSN_LIST,
  dispatcherOptions: {
    topologyMode: 'proxy',
  },
});

In proxy mode, PostgresKit:

  • uses SELECT 1 instead of pg_is_in_recovery() for health checks;
  • treats every healthy endpoint as eligible for both db.primary and db.replica;
  • selects the healthy endpoint with the lowest latest health-check latency;
  • excludes unhealthy endpoints;
  • does not produce primary/replica topology warnings;
  • preserves the existing unavailable-database error when no healthy endpoints remain.

Backward compatibility

primary-replica remains the default topology mode. When topologyMode is omitted, the existing health checks, routing behavior, warnings, and errors remain unchanged.

Testing

Added tests covering:

  • existing primary/replica behavior in the default mode;
  • role-agnostic proxy health checks;
  • fastest healthy proxy selection for both db.primary and db.replica;
  • unhealthy endpoint exclusion;
  • behavior when all proxy endpoints are unavailable;
  • absence of primary/replica topology warnings in proxy mode.

@jhoncool

Copy link
Copy Markdown
Collaborator

@Denchick Thanks for the PR. A few things to address before merge.

1. The new tests never run

jest/unit.config.js is added, but it isn't referenced by any npm script or CI job. npm test resolves to jest -c ./jest/jest.config.js, whose rootDir is ../examples/demo/ and whose testMatch is ['**/examples/demo/build/tests/**/(*.)+(test|spec).[tj]s']. jest -c ./jest/jest.config.js --listTests does not include tests/dispatcher.test.js — so the whole test section of this PR is dead weight and will not catch a regression. The tests do pass when invoked directly (npx jest -c ./jest/unit.config.js → 3/3), they're just unreachable.

Suggested wiring:

"test:unit": "jest -c ./jest/unit.config.js",
"test": "npm run test:prepare && npm run test:unit && npm run test:run"

2. The "no topology warnings" assertion is vacuous

tests/dispatcher.test.js:97-102:

expect(loggedErrorMessages(logger)).not.toEqual(
  expect.arrayContaining([
    'Multiple primary connections detected, something is wrong',
    'No alive replica available, using master for read',
  ]),
);

arrayContaining([A, B]) matches only when both elements are present, so under .not the assertion passes whenever at least one is missing. I verified this: expect([A]).not.toEqual(expect.arrayContaining([A, B])) is green even though A was logged. Since this test is the only guard for the PR's core claim ("does not produce primary/replica topology warnings"), it currently guards nothing. expect.not.arrayContaining has the same "not all" semantics and is not the fix either.

Rather than matching on English sentences, please attach a stable code to these two log records and assert on the code. Matching on prose is brittle — any copy-editing of the message silently disables the test — and a stable code is useful for consumers too, who can key alerting off it instead of grepping log text. PDError already carries an optional code, and the internal error wrapper (lib/dispatcher.ts:83-89) already forwards the Error instance as the second positional argument to the user's logger, so no signature or type changes are needed.

lib/dispatcher.ts:146-153:

if (primaryConnections.length > 1) {
  const error = new PDError('Multiple primary connections detected, something is wrong');
  error.code = 'ERR_DB_MULTIPLE_PRIMARIES';
  this.logger.error({
    message: error.message,
    error,
    data: {primaryConnections: primaryConnections.map((c) => c.host)},
  });
}

lib/dispatcher.ts:177-179:

if (this.connections.length > 1) {
  const error = new PDError('No alive replica available, using master for read');
  error.code = 'ERR_DB_NO_ALIVE_REPLICA';
  this.logger.error({message: error.message, error});
}

Then in the test:

const loggedErrorCodes = (logger) => logger.error.mock.calls.map(([, error]) => error.code);

// ...
const codes = loggedErrorCodes(logger);
expect(codes).not.toContain('ERR_DB_MULTIPLE_PRIMARIES');
expect(codes).not.toContain('ERR_DB_NO_ALIVE_REPLICA');

3. Minor: primary: false pollutes the status log in proxy mode

performCheckupQuery returns a hardcoded {pingOk: true, primary: false} (lib/dispatcher.ts:227), so the periodic Database current status record (lib/dispatcher.ts:201-213) reports every host as primary: false forever. For a mode whose stated purpose is to stop producing misleading topology output, that's an odd leftover — an operator reading the logs sees a cluster with no primary at all. Consider omitting the primary field from the status log in proxy mode and logging topologyMode instead, so the log is self-describing.

4. Minor: extract an isProxyMode getter

The literal comparison this.options.topologyMode === 'proxy' is repeated at three call sites (lib/dispatcher.ts:141, :167, :224). A private get isProxyMode() would read better and keeps the check in one place if a third mode ever shows up.

@Denchick

Denchick commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@jhoncool, thanks for the review. I’ve addressed all four issues.

For #2, I fixed the assertion by checking each warning separately, but left error codes out to keep this PR focused on proxy topology and avoid changing the existing logging API. I can add stable error codes in a separate PR.

@jhoncool

Copy link
Copy Markdown
Collaborator

For #2, I fixed the assertion by checking each warning separately, but left error codes out to keep this PR focused on proxy topology and avoid changing the existing logging API. I can add stable error codes in a separate PR.

Got it, thanks! For now, let's add positive assertions for both messages:

expect(errorMessages).toContain('Multiple primary connections detected, something is wrong');
expect(errorMessages).toContain('No alive replica available, using master for read');

This way we explicitly verify the expected warnings are triggered. We can swap to error codes later in a follow-up PR.

@jhoncool
jhoncool merged commit 267d953 into gravity-ui:main Aug 11, 2026
2 checks passed
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.

2 participants