Proposal: Activity to fetch config from the DB #3589

alexstoick · 2025-10-10T16:56:13Z

This uses a new helper in internal.FetchConfigFromDB to fetch a fully hydrated protos.FlowConnectionConfigs. When passing this config to Temporal we strip the tableMappings array element which can cause it to go over the 2MB limit.

This contains a subset of the changes which were originally proposed in: #3407

This uses a new helper in `internal.FetchConfigFromDB` to fetch a fully hydrated `protos.FlowConnectionConfigs`. When passing this config to Temporal we strip the `tableMappings` array element which can cause it to go over the 2MB limit. This contains a subset of the changes which were originally proposed in: PeerDB-io#3407

ilidemi · 2025-10-11T08:44:42Z

Some ideas on how to tighten this up:

Remove the field from FlowConnectionConfigs (mark as reserved) to rip off the band-aid and have the type system working with us. For table additions pass the mappings as a separate arg.
Add a table_mappings table to catalog, write there on flow creation in handler.go, add a migration after pause like in cdc_flow.go lines 375-390 from Remove TableNameSchemaMapping from temporal state #2090 which pulled off a similar thing. Instead of fetching config from DB, fetch table mappings from DB.
Preserve replayability, which can be done by adding aversion bigint field that increments within a flow, referencing the value from FlowConnectionConfigs and making the mappings table be append-only.

Would be nice to consider how to not to trade off too much observability that Temporal is providing - right now for workflows with 1-10 tables it's convenient to see it right there in the execution. PeerDB UI and raw catalog access is not easily available in ClickPipes environment. Maybe log up to 100 tables (along with count and version) every time they're getting fetched?

alexstoick · 2025-10-13T10:53:44Z

@ilidemi - Thanks for the feedback!

I hesitated in removing the field - as it does feel like a nuclear option - and I was worried about the migration path for running envs. I'll go ahead and implement your suggestion for the new table & taking a similar approach to #2090

In my previous PR: #3407 I was able to fully remove the options argument being passed which also reduced the size of the blob passed around - what are your thoughts on that? The options didn't serve any purpose once I was fetching the TableMappings and SrcTableIdNameMapping from the DB.

For adding/removing table mappings - I think we can either do:

a different activity
pass as an additional param

My only concern with changing the signature of this is again dealing with running systems and queued jobs - not sure how well Temporal behaves when you change the signature of the job!

ilidemi

Options argument - moving SrcTableIdNameMapping to DB too makes sense to me, and use a version instead (or -1/nil). Rest of fields change with config/state separation, and although config is not fully immutable, merging the two would require more thinking (if it's a good idea) and would be too much scope for this PR. If it's easy enough and useful to do, we can do it separately.

Changing activity signatures - the only fully supported upgrade route for PeerDB is to pause CDC mirrors (and wait for setup/snapshot/drop to complete), then upgrade, then resume. It is nice when a surgical change is possible for running snapshots but we're not shooting for that even with smaller changes. We can just treat them as functions (besides the codepath from the start of CDCFlowWorkflow to the pause loop) and not worry about the rest. Adding an argument is good.

ilidemi · 2025-10-15T23:45:51Z

nexus/catalog/migrations/V49__table_mappings_table.sql

@@ -0,0 +1,6 @@
+CREATE TABLE IF NOT EXISTS table_mappings (
+    flow_name varchar(255) not null,Expand commentComment on line R2ResolvedCode has comments. Press enter to view.
+    version int32 not null default 1,


bigint, in case someone spams the API endpoint for updating tables so we don't even have to think through overflows

ilidemi · 2025-10-15T23:46:19Z

nexus/catalog/migrations/V49__table_mappings_table.sql

@@ -0,0 +1,6 @@
+CREATE TABLE IF NOT EXISTS table_mappings (
+    flow_name varchar(255) not null,Expand commentComment on line R2ResolvedCode has comments. Press enter to view.


copypaste quirk?

ilidemi · 2025-10-16T03:47:50Z

flow/workflows/cdc_flow.go

+			// TODO: thought - maybe we need to pass additionalTables and then in the
+			// CDCWorkflow we persist them to the DB and send an incremented `tableMappingVersion`
+			// to the new workflow?


Allowing up to ~10K additional tables at a time seems like a generous restriction. Although now it does make sense to have API explicitly turn down config update requests that Temporal won't take

In #3589 we want to stop using a field in a way that's not error-prone in the rest of the code (so, removing) but also doesn't break API backcompat (so, not removing). Trying to split FlowConnectionConfigs into two contracts that are made sure to be in sync with a little codegen. Generated code has small TODOs that would be removed by #3589 if we go this route. Validated that renaming the type of a workflow arg works well with history replay, only the payload is important.

alexstoick temporarily deployed to external-contributor October 10, 2025 16:56 — with GitHub Actions Inactive

alexstoick had a problem deploying to external-contributor October 10, 2025 16:56 — with GitHub Actions Failure

alexstoick mentioned this pull request Oct 10, 2025

Activity to store DB in local file alexstoick/peerdb#3

Closed

serprex requested review from heavycrystal, ilidemi and serprex October 10, 2025 16:57

Tenanative first go at removing tableMappings & storing in DB

069d870

alexstoick requested a deployment to external-contributor October 13, 2025 13:49 — with GitHub Actions Waiting

alexstoick added 2 commits October 13, 2025 16:53

Resolve issues from deprecating TableMappings pt1

e6add9b

Dealing with the removed tableMappings - pt2; build is passing

6d045b1

alexstoick requested a deployment to external-contributor October 13, 2025 16:27 — with GitHub Actions Waiting

alexstoick temporarily deployed to external-contributor October 13, 2025 16:27 — with GitHub Actions Inactive

ilidemi mentioned this pull request Oct 15, 2025

Decouple FlowConnectionConfigs from API #3605

Merged

ilidemi reviewed Oct 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Proposal: Activity to fetch config from the DB #3589

Proposal: Activity to fetch config from the DB #3589

alexstoick commented Oct 10, 2025

Uh oh!

ilidemi commented Oct 11, 2025 •

edited

Loading

Uh oh!

alexstoick commented Oct 13, 2025

Uh oh!

ilidemi left a comment

Uh oh!

ilidemi Oct 15, 2025

Uh oh!

ilidemi Oct 15, 2025

Uh oh!

ilidemi Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		@@ -0,0 +1,6 @@
		CREATE TABLE IF NOT EXISTS table_mappings (
		flow_name varchar(255) not null,Expand commentComment on line R2ResolvedCode has comments. Press enter to view.

Proposal: Activity to fetch config from the DB #3589

Are you sure you want to change the base?

Proposal: Activity to fetch config from the DB #3589

Conversation

alexstoick commented Oct 10, 2025

Uh oh!

ilidemi commented Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexstoick commented Oct 13, 2025

Uh oh!

ilidemi left a comment

Choose a reason for hiding this comment

Uh oh!

ilidemi Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

ilidemi Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

ilidemi Oct 16, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ilidemi commented Oct 11, 2025 •

edited

Loading