Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement db-uri-read-replicas #3017

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Implement db-uri-read-replicas #3017

wants to merge 2 commits into from

Conversation

robx
Copy link
Contributor

@robx robx commented Oct 20, 2023

I'm still quite unsure about a bunch of this, but felt I needed to get some version of this out the door. It should work in principle but I have less confidence than I'd like mostly because it's quite awkward to test.

The major things outstanding items (in part perhaps those can be left for an iteration):

  • There's a lot of complexity that comes with having multiple database addresses
    • Instead of one connection that can be up or down, there are multiple. How do we deal with this in terms of considering things healthy? E.g. if one or more read replicas fail, we could degrade smoothly by automatically falling back to the main server.
    • How do we balance connection pooling between different database; what should happen when we have two read replica and one is very slow
      For a start I went with the simplest approaches I could come up with.
  • Testing is hard. I tried a couple of approaches of simulating read-only connections but couldn't make anything work except for a full on replication setup. It would be possible to set this up in integration tests (i.e., I think a variant of withTmpDb that sets up a read replica is doable), but I've shied away from the work so far. It's tempting to spin out the pool implementation to a separately tested package with decent coverage, and leave the postgrest coverage to something minimal.
  • The pooling implementation is rather ad hoc, once we settle on a solution it could be nice to break that out into a package

I'll probably think of more once I let this settle for a bit 😅

From the commit message:

  • takes a list of URIs to use as read replicas
  • queries are routed based on the transaction mode, with read-mode queries going to the read replicas if any are configured
  • we configure one pool per database uri, all with the same configuration
  • which read replica to use is chosen randomly
  • if you want the master to also be used for read-only queries, put it in both db-uri and db-uri-read-replicas

Test coverage is spotty and work-in-progress. It requires a replicating setup which is not automated. With a locally set up read replica, the integration test suite should pass as follows:

PGHOSTREP='/tmp' PGPORTREP=5433 PGHOST='/tmp' PGPORT=5432
postgrest-test-io

You can pass '-k test_readonly' for replication-specific tests.

Comment on lines 182 to 185
create or replace function read_only_session() returns void as $$
begin
set session characteristics as transaction read only;
end; $$ language plpgsql;
Copy link
Member

Choose a reason for hiding this comment

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

Oh, what's this used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's one of my failed attempts at simulating read-only in tests, missed backing it out. I was calling it as db-pre-request, haven't quite worked out why it doesn't work as intendended. Either it doesn't have the effect I understood it should have, or we (directly in postgrest or perhaps via hasql) reset the transaction mode afterwards.

- takes a list of URIs to use as read replicas
- queries are routed based on the transaction mode,
  with read-mode queries going to the read replicas
  if any are configured
- we configure one pool per database uri, all with
  the same configuration
- which read replica to use is chosen randomly
- if you want the master to also be used for read-only
  queries, put it in both db-uri and db-uri-read-replicas

Test coverage is spotty and work-in-progress. It requires a
replicating setup which is not automated. With a locally set
up read replica, the integration test suite should pass as
follows:

PGHOSTREP='/tmp' PGPORTREP=5433 PGHOST='/tmp' PGPORT=5432 \
  postgrest-test-io

You can pass '-k test_readonly' for replication-specific tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants