Merge move-tables PRs onto feature branch: applier logic#1703
Conversation
… for move-tables feature
There was a problem hiding this comment.
Pull request overview
This PR advances the move-tables feature by introducing separate SELECT/INSERT query builders for cross-cluster row copy, adding an applier method to execute those queries per chunk, and adapting DML application to write into the move-tables target database.
Changes:
- Added
MoveTableCopySelectQueryBuilderandMoveTableCopyInsertQueryBuilderfor move-tables row copy. - Added
Applier.ApplyIterationMoveTableCopyQueries()and switched chunk iteration to use it in move-tables mode. - Updated the applier to apply DML events to the target DB in move-tables mode, and added move-tables-focused tests/utilities.
Show a summary per file
| File | Description |
|---|---|
| go/sql/builder.go | Adds move-tables SELECT/INSERT query builders for chunk copy. |
| go/sql/builder_test.go | Adds unit tests and benchmarks for the new move-tables query builders. |
| go/logic/test_utils.go | Adds helper constants/functions for a secondary test database. |
| go/logic/migrator.go | Switches chunk-copy to use move-tables copy logic when enabled. |
| go/logic/applier.go | Adds target DB wiring and implements move-tables chunk copy + DML routing. |
| go/logic/applier_test.go | Adds integration tests for move-tables DML routing and chunk copy behavior. |
| go/base/context.go | Extends move-tables context with a connection config and propagates connection settings. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 10
| return nil | ||
| } | ||
| _, rowsAffected, _, err := mgtr.applier.ApplyIterationInsertQuery() | ||
| var rowsAffected int64 |
There was a problem hiding this comment.
@danieljoos quick boundary question based on the Copilot comment on these same lines . #8207 (target table CRUD), is under my purview from (#1700 and #1704), and also touches MoveTables() initialization, .
I was wondering if you were planning to wire prepareQueries() into MoveTables() here as a follow-up commit, or does that stay open for whoever picks up the next sub-issue that touches initialization (would be #8207 on my side)? Either is fine, just want to know whether the scope expands on my end!
A Pull Request should be associated with an Issue.
Related issue (public): #1681
Related issue (internal): https://github.com/github/database-infrastructure/issues/8175
Related issue (internal): https://github.com/github/database-infrastructure/issues/8163
Description
This PR is part of the move-tables feature development.
It combines 3 PRs from the previous stacked PRs.
It introduces query builders for separate
SELECT+INSERTqueries that are used for the move-tables mode instead of the range-INSERTquery that usually runs on the primary host.For move-tables the copy step needs to first
SELECTon the source, thenINSERTon the target. Of course this comes with much higher cost: copying the data to and from the host that runsgh-ost.It introduces a new function
ApplyIterationMoveTableCopyQueriesto theApplierthat will be used for copying rows in the move-tables mode.It adapts the
Applierto write DML events to the target database when in move-tables mode.script/cibuildreturns with no formatting errors, build errors or unit test errors.