Skip to content

Introduce a SourcePrimaryConnectionConfig for primary-required move-table ops#1718

Open
zacharysierakowski wants to merge 7 commits into
feature-move-tablesfrom
move-tables-connection-audit
Open

Introduce a SourcePrimaryConnectionConfig for primary-required move-table ops#1718
zacharysierakowski wants to merge 7 commits into
feature-move-tablesfrom
move-tables-connection-audit

Conversation

@zacharysierakowski

@zacharysierakowski zacharysierakowski commented Jun 19, 2026

Copy link
Copy Markdown

re: https://github.com/github/database-infrastructure/issues/8275

Description

This PR does a few things that were lingering on our backlog.

  1. Primarily, it adds a separate connection specifically for the source primary. We need this for things like the write cutover RENAME, as well as the post-move-tables cleanup for DROP the source renamed table (if specified by the cli arg)
  2. It also condenses the write cutover T1 and T2 ops so that it's safer by running the RENAME and GTID gather in a single DB query using a multi-statement. This is safer because it lessens the chance of crash between the RENAME and the GTID query. And still ensures we're running the two on the same connection.
  3. Adds a guard to ensure the operator is providing a pointer to the primary for the TARGET. All target operations require to run against the primary (unlike the source) so this is just a way to provide an early exit
  4. Adds another guard to ensure the operator is providing a pointer to a replica (if possible) for the SOURCE. If there are no replicas, or the operator is absolutely sure they want to use the primary, they can pass --allow-on-source-primary to bypass this guard

Demo(s)

Auto resolve source primary from source replica

image

[Guard] Early exit if target is a replica

image

[Guard] Early exit if source is a primary and have not specified the new override arg (--allow-on-source-primary)

image

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.

@zacharysierakowski zacharysierakowski changed the base branch from move-tables-1.9-cleanup to feature-move-tables June 19, 2026 19:02
Comment thread script/move-tables/reset

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

random script i've had stashed for a while that's been handy between local testing so i don't have to do a full teardown/setup.

happy to remove from this PR if we don't want it merged.

Comment thread go/logic/streamer.go
// handle uses InspectorConnectionConfig (the source) and stays open in both the
// normal and the cutover-resume paths (Close() only closes the binlog reader,
// not `db`).
func (es *EventsStreamer) DropSourceOldTable() error {

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was only added recently as part of #1717. I added it knowing that it probably wasn't the right spot for it, but without having the distinction between the primary vs. replica connections (this PR) it was hard to figure out the best spot for this drop operation to dangle from

@zacharysierakowski zacharysierakowski self-assigned this Jun 19, 2026
@zacharysierakowski zacharysierakowski marked this pull request as ready for review June 19, 2026 20:02
Copilot AI review requested due to automatic review settings June 19, 2026 20:02

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 improves --move-tables correctness and safety by introducing a dedicated source-primary connection for primary-required operations (cutover RENAME, drain-GTID capture, and source __del cleanup), while also adding early guards to prevent accidentally running move-tables against non-writable targets or against the source primary for read-heavy phases.

Changes:

  • Add a dedicated sourcePrimaryDB/SourcePrimaryConnectionConfig for move-tables primary-required operations, including consolidated RENAME; SELECT @@global.gtid_executed in a single multi-statement round trip.
  • Add startup guards to fail fast when the move-tables target is read_only, and to block running the source read path on the source primary unless --allow-on-source-primary is set.
  • Update local move-tables scripts/docs to better support primary auto-resolution from a source replica, including /etc/hosts alias assistance and a new reset helper.
Show a summary per file
File Description
script/move-tables/setup Adds best-effort /etc/hosts aliasing so host-side gh-ost can resolve docker-compose service names used by replication metadata.
script/move-tables/reset New helper script to reset source/target tables for local move-tables testing.
script/move-tables/README.md Updates the example invocation to read from the source replica port and includes --initially-drop-socket-file.
go/logic/streamer.go Removes source-side cleanup responsibility from the streamer.
go/logic/migrator.go Introduces source-primary connection/config, consolidated cutover T1/T2 multi-statement, read-path guard, and source __del drop via primary.
go/logic/migrator_move_tables_cutover_test.go Adds/updates unit+integration coverage for source-primary resolution, writability guard, and source-side drop.
go/logic/migrator_move_tables_cleanup_test.go Adds coverage for nil source-primary connection error on cleanup.
go/logic/applier.go Adds a writability guard for move-tables target connections.
go/cmd/gh-ost/main.go Adds --allow-on-source-primary flag wiring.
go/base/context.go Extends move-tables context with AllowOnSourcePrimary and SourcePrimaryConnectionConfig.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 10/10 changed files
  • Comments generated: 1

Comment thread script/move-tables/reset
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.

2 participants