Skip to content

Merge move-tables PRs onto feature branch: applier logic#1703

Merged
zacharysierakowski merged 8 commits into
feature-move-tablesfrom
move-tables/applier-dml-events
Jun 11, 2026
Merged

Merge move-tables PRs onto feature branch: applier logic#1703
zacharysierakowski merged 8 commits into
feature-move-tablesfrom
move-tables/applier-dml-events

Conversation

@danieljoos

Copy link
Copy Markdown
Contributor

A Pull Request should be associated with an Issue.

We wish to have discussions in Issues. A single issue may be targeted by multiple PRs.
If you're offering a new feature or fixing anything, we'd like to know beforehand in Issues,
and potentially we'll be able to point development in a particular direction.

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

Further notes in https://github.com/github/gh-ost/blob/master/.github/CONTRIBUTING.md
Thank you! We are open to PRs, but please understand if for technical reasons we are unable to accept each and any PR

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 + INSERT queries that are used for the move-tables mode instead of the range-INSERT query that usually runs on the primary host.

For move-tables the copy step needs to first SELECT on the source, then INSERT on the target. Of course this comes with much higher cost: copying the data to and from the host that runs gh-ost.

It introduces a new function ApplyIterationMoveTableCopyQueries to the Applier that will be used for copying rows in the move-tables mode.

It adapts the Applier to write DML events to the target database when in move-tables mode.

In case this PR introduced Go code changes:

  • contributed code is using same conventions as original code
  • script/cibuild returns with no formatting errors, build errors or unit test errors.

Copilot AI review requested due to automatic review settings June 10, 2026 14:07
@danieljoos danieljoos added the feature-move-tables PRs that are associated with the new move-tables feature label Jun 10, 2026
@danieljoos danieljoos changed the title Move tables/applier dml events Merge move-tables PRs onto feature branch: applier logic Jun 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 MoveTableCopySelectQueryBuilder and MoveTableCopyInsertQueryBuilder for 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

Comment thread go/sql/builder.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/logic/migrator.go
Comment thread go/logic/applier_test.go
@danieljoos danieljoos changed the base branch from move-tables/cli to feature-move-tables June 10, 2026 14:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot's findings

  • Files reviewed: 7/7 changed files
  • Comments generated: 3

Comment thread go/logic/applier.go
Comment thread go/logic/applier.go
Comment thread go/sql/builder.go
@zacharysierakowski zacharysierakowski merged commit 7a9dca7 into feature-move-tables Jun 11, 2026
7 checks passed
Comment thread go/logic/migrator.go
return nil
}
_, rowsAffected, _, err := mgtr.applier.ApplyIterationInsertQuery()
var rowsAffected int64

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-move-tables PRs that are associated with the new move-tables feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants