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

feat(electric): Add a new write-to-pg mode that applies changes directly to Postgres #698

Merged
merged 22 commits into from
Dec 7, 2023

Conversation

alco
Copy link
Member

@alco alco commented Nov 23, 2023

When Electric is started in this write mode, instead of creating a subscription in Postgres and streaming client writes to it over a logical replication connection, it will use a regular connection to apply client writes as regular DML statements.

The direct_writes mode can be enabled as follows when running Electric from source code:

MIX_ENV=prod \
ELECTRIC_WRITE_TO_PG_MODE=direct_writes \
PG_PROXY_PASSWORD=<pwd> \
DATABASE_URL=<url> \
iex -S mix

Alternatively, you can check out my branch alco/direct-writes-to-pg-dev, start the development database with make start_dev_env and start Electric with:

AUTH_MODE=insecure iex -S mix

Base automatically changed from alco/internal-schema to main November 23, 2023 14:50
@alco alco marked this pull request as draft November 24, 2023 12:52
@KyleAMathews
Copy link
Contributor

Curious what's the goal here? Lower latency? Higher throughput?

@thruflo
Copy link
Contributor

thruflo commented Nov 28, 2023

Curious what's the goal here? Lower latency? Higher throughput?

The key thing this unlocks is being able to run with lower permissions. Currently we use inbound logical replication for writing data from Electric into Postgres. This adds an alternative direct writes mode that uses a standard interactive Postgres client. This reduces the permissions needed and, for example, means we can run on vanilla hosted Supabase.

@alco
Copy link
Member Author

alco commented Nov 28, 2023

Curious what's the goal here? Lower latency? Higher throughput?

Both can be improved later on with this change in place. Direct writes allow us to batch multiple statements into one, keep a cache of prepared statements and generally be more aware of when client updates get committed in Postgres. We also get the freedom to "spread" client updates over a pool of connections, such that multiple updates are committed in parallel, whereas up until now we had to merged them into a single stream of transactions to be fed to Postgres as a stream logical replication messages.

@thruflo
Copy link
Contributor

thruflo commented Nov 28, 2023

Both can be improved later on with this change in place.

Interesting -- I was thinking under the assumption that direct writes would be slower, because you would wait for one transaction to be confirmed before sending the next? Is that true and then is it the case that that therefore reduces throughput but that this reduction may be mitigated and actually outweighed by the performance improvements from the techniques you listed?

Is there something here in knowing when transactions can be written in parallel and is this something we already have enough metadata for?

@KyleAMathews
Copy link
Contributor

🔥 nice set of optimizations that are coming!

@alco alco changed the title poc(electric): Add an "immediate" write mode feat(electric): Add a new inbound replication mode that applies changes directly to Postgres Nov 29, 2023
Copy link
Contributor

@icehaunter icehaunter left a comment

Choose a reason for hiding this comment

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

Looks pretty good! I'm glad this functionality could be achieved in a relatively small code change.

I have multiple questions for particular lines, but also some general questions:

  1. Why are you calling it inbound replication everywhere? We're configuring the system from the POV of Electric, to which streaming to PG is outbound. I found this confusing and would appreciate this changed as I can see myself stumbling over this later on
  2. Why are we keeping direct streaming at all? Perfomance comparison? Direct writes seem like a more generic approach from POV of deployment (also less configuration in general)

Great job all around!

@alco
Copy link
Member Author

alco commented Nov 29, 2023

I have multiple questions for particular lines, but also some general questions:

Hey, thanks for the review! @thruflo can answer those two general questions better than me but I'll give it a go:

  1. You can see from the commit history that I started with ELECTRIC_WRITE_MODE=streaming|immediate. The ELECTRIC_INBOUND_MODE name was James' idea, I believe. It's a reference to the act of applying inbound client updates via logical replication or direct writes to Postgres.
  2. Again, not my idea, but it does let us keep existing users unaffected in case the new mode has edge cases we haven't uncovered yet. The new mode is operationally simpler and doesn't require a superuser role, so we will be making it the default mode soon enough, IMO. Then we'll be able to retire the code that deals with logical replication.

@thruflo
Copy link
Contributor

thruflo commented Nov 30, 2023

Yup, as @alco says.

Inbound derives from @balegas who I remember referenced MySQL naming. Which is database centric. Where @icehaunter is understandably thinking from Electric’s POV.

I discussed with @balegas because I wanted us to be intentional about the naming and I didn’t think “immediate” made sense. If there’s a better term than inbound great. But I’d lean towards the Postgres-centric view if discussing inbound vs outbound.

@icehaunter
Copy link
Contributor

@thruflo @balegas @alco I would like to argue against using "inbound" here - we're configuring electric with this, not the Postgres, nor the entire stack. I believe it's bound to lead to more confusion, especially explicitly prefixed with ELECTRIC_. Trying to "hide" Electric from the configuration seems like a decision that would lead to operational confusion and thus complexity in deployment. "Inbound" here is an ambiguous word. In my head it's definitely "outbound", but if that's also ambiguous, then maybe very explicitly ELECTRIC_WRITE_TO_PG_MODE?

@thruflo
Copy link
Contributor

thruflo commented Nov 30, 2023

I'm good with ELECTRIC_WRITE_TO_PG_MODE.

@balegas
Copy link
Contributor

balegas commented Nov 30, 2023

I'm happy with any decision.
With regards to supporting both modes, let's keep things around until we learn more.

@alco
Copy link
Member Author

alco commented Nov 30, 2023

Both can be improved later on with this change in place.

Interesting -- I was thinking under the assumption that direct writes would be slower, because you would wait for one transaction to be confirmed before sending the next? Is that true and then is it the case that that therefore reduces throughput but that this reduction may be mitigated and actually outweighed by the performance improvements from the techniques you listed?

I believe that providing direct write performance comparable to logical replication is an easy baseline to achieve. Postgres supports query pipelining which means that even if we feed a single stream of changes, we don't have to wait for Postgres to response to every change before sending the next one. This already looks similar to the logical replication approach. On top of that we can add caching of prepared statements which implies encoding parameters using the binary format. This will result in fewer bytes sent over the wire and lower latency.

Is there something here in knowing when transactions can be written in parallel and is this something we already have enough metadata for?

I may be speaking out of ignorance, but I think we have enough room for parallelization. At a very basic level, updates to disjoint sets of tables can be parallized. But even if two updates would conflict with each other, Postgres's MVCC should already be providing deterministic ordering of such concurrent updates, and our conflict-resolution logic will keep things consistent.

@alco alco changed the title feat(electric): Add a new inbound replication mode that applies changes directly to Postgres feat(electric): Add a new write-to-pg mode that applies changes directly to Postgres Dec 2, 2023
The upsert_acknowledged_client_lsn trigger won't fire in the immediate
write mode.
…ic writes

This gives us control over which triggers fire regardless of whether
writes are streamed to Postgres over a logical replication connection or
applied directly as DML statements.
Depending on whether proxy_opts are passed to it, we either
open a regular connection to the singleton test db (current behaviour
without proxy) or create a new temporary database and open a proxied
connection to it.

One of the reasons being the latest internal migration failing with a
SyntaxError when applied via the proxy.
Using conditionals inside a loop may look weird but it actually provides
more straightforward control flow compared to the previous version.
This follows the same approach that was implemented earlier in the conflict resolution trigger migration
…untime

This is necessary for the new trigger templates to override the old ones in
databases that already have the conflict resolution trigger migration applied.
@alco alco marked this pull request as ready for review December 6, 2023 22:20
This change removes the word "error" from the logs which was causing E2E
tests to fail.
@alco
Copy link
Member Author

alco commented Dec 7, 2023

Note: I haven't implemented any specific error handling for the case where a client's update is rejected by Postgres for any reason. Opened VAX-1416 to address that at a later time.

@icehaunter icehaunter merged commit 2f3feff into main Dec 7, 2023
5 checks passed
@icehaunter icehaunter deleted the alco/direct-writes-to-pg branch December 7, 2023 14:41
alco added a commit that referenced this pull request Dec 9, 2023
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.

5 participants