Skip to content

Commit 8a28958

Browse files
authored
[DBI-798] MySQL Connector: Allow passing server_id as a peer configuration parameter (#4525)
This PR allows setting `server_id` on MySQL/MariaDB peers. When it is present, CDC flows will use this value as the [replication `server_id`](https://dev.mysql.com/doc/mysql-replication-excerpt/5.7/en/replication-options.html#:~:text=server_id%20is%20set%20to%200,replica%20in%20the%20replication%20topology.) in the declared MySQL replica. This is an optional field in the configuration and offers a deterministic alternative to the default "random id" select upon CDC flow initialization in the current implementation and in the new implementation when the value is missing. Note that if a peer has this option, it can not be re-used in different mirrors. This PR also includes changes to prevent the creation of such mirrors through validation. <img width="1222" height="866" alt="image" src="https://github.com/user-attachments/assets/ecdec95f-5285-46fd-8e1d-430dcecb5c7e" /> Commits are organized so they can be reviewed independently: - bb949c0 Core functionality, adding optional parameter to the gRPC specification of PeerDB API and propagating it down to CDC initialization. - 729225c e2e tests for this functionality. - 132c87e Add a validation step in the create/validate API endpoints that prevents re-using the same MySQL peer in two mirrors if it has `server_id` set. - 35f565f API e2e tests for this validation. - 0950728 PeerDB UI changes to include this field in MySQL peer configuration Part of: https://linear.app/clickhouse/issue/DBI-798
1 parent 646075a commit 8a28958

8 files changed

Lines changed: 255 additions & 2 deletions

File tree

flow/cmd/validate_mirror.go

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88
"regexp"
99
"slices"
1010

11+
"google.golang.org/protobuf/proto"
12+
1113
"github.com/PeerDB-io/peerdb/flow/connectors"
1214
"github.com/PeerDB-io/peerdb/flow/generated/proto_conversions"
1315
"github.com/PeerDB-io/peerdb/flow/generated/protos"
@@ -102,6 +104,10 @@ func (h *FlowRequestHandler) validateCDCMirrorImpl(
102104
}
103105
}
104106

107+
if apiErr := h.checkSourcePeerReuse(ctx, connectionConfigs); apiErr != nil {
108+
return nil, apiErr
109+
}
110+
105111
srcConn, srcClose, err := connectors.GetByNameAs[connectors.MirrorSourceValidationConnector](
106112
ctx, connectionConfigs.Env, h.pool, connectionConfigs.SourceName,
107113
)
@@ -158,6 +164,73 @@ func (h *FlowRequestHandler) validateCDCMirrorImpl(
158164
return &protos.ValidateCDCMirrorResponse{}, nil
159165
}
160166

167+
// checkSourcePeerReuse rejects a CDC mirror whose MySQL source peer pins a fixed server_id while
168+
// that peer already backs another streaming CDC mirror. A fixed server_id can only be used by one
169+
// concurrent binlog connection, so sharing such a peer across mirrors makes their replicas collide
170+
// on the source. Peers without a fixed server_id fall back to a random per-connection id and are
171+
// safe to reuse.
172+
func (h *FlowRequestHandler) checkSourcePeerReuse(
173+
ctx context.Context, cfg *protos.FlowConnectionConfigsCore,
174+
) APIError {
175+
// A pinned server_id only matters for CDC; a snapshot-only mirror never opens a binlog connection.
176+
if cfg.DoInitialSnapshot && cfg.InitialSnapshotOnly {
177+
return nil
178+
}
179+
180+
peer, err := connectors.LoadPeer(ctx, h.pool, cfg.SourceName)
181+
if err != nil {
182+
return NewInternalApiError(fmt.Errorf("failed to load source peer %s: %w", cfg.SourceName, err))
183+
}
184+
mysqlCfg := peer.GetMysqlConfig()
185+
if mysqlCfg == nil || mysqlCfg.ServerId == nil {
186+
return nil
187+
}
188+
189+
// CDC flows for this source peer other than the mirror being validated.
190+
// query_string IS NULL filters out QRep flows, which do
191+
// not stream the binlog.
192+
query := `
193+
SELECT f.name name, f.config_proto config_proto
194+
FROM flows f
195+
JOIN peers p ON f.source_peer = p.id AND p.name = $1
196+
WHERE f.name != $2 AND f.config_proto IS NOT NULL AND f.query_string IS NULL
197+
`
198+
199+
rows, err := h.pool.Query(ctx, query, cfg.SourceName, cfg.FlowJobName)
200+
if err != nil {
201+
return NewInternalApiError(fmt.Errorf("failed to check source peer reuse for %s: %w", cfg.SourceName, err))
202+
}
203+
defer rows.Close()
204+
205+
for rows.Next() {
206+
var name string
207+
var configBytes []byte
208+
if err := rows.Scan(&name, &configBytes); err != nil {
209+
return NewInternalApiError(fmt.Errorf("failed to read flow row while checking peer reuse: %w", err))
210+
}
211+
var existing protos.FlowConnectionConfigsCore
212+
if err := proto.Unmarshal(configBytes, &existing); err != nil {
213+
return NewInternalApiError(fmt.Errorf("failed to unmarshal config for flow %s: %w", name, err))
214+
}
215+
// A snapshot-only mirror never streams the binlog, so it cannot collide on server_id.
216+
if existing.DoInitialSnapshot && existing.InitialSnapshotOnly {
217+
continue
218+
}
219+
return NewFailedPreconditionApiError(
220+
fmt.Errorf("source peer %q pins server_id=%d, which cannot be shared across mirrors; "+
221+
"it is already used by CDC mirror %q. Use a distinct source peer (or one without a fixed server_id) for this mirror",
222+
cfg.SourceName, mysqlCfg.GetServerId(), name),
223+
NewMirrorErrorInfo(map[string]string{
224+
common.ErrorMetadataOffendingField: "source_name",
225+
}))
226+
}
227+
if err := rows.Err(); err != nil {
228+
return NewInternalApiError(fmt.Errorf("failed to iterate flows while checking peer reuse for %s: %w", cfg.SourceName, err))
229+
}
230+
231+
return nil
232+
}
233+
161234
func (h *FlowRequestHandler) checkIfMirrorNameExists(ctx context.Context, mirrorName string) (bool, error) {
162235
var nameExists bool
163236
if err := h.pool.QueryRow(ctx, "SELECT EXISTS(SELECT * FROM flows WHERE name = $1)", mirrorName).Scan(&nameExists); err != nil {

flow/connectors/mysql/cdc.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"errors"
1111
"fmt"
1212
"log/slog"
13+
"math"
1314
"math/rand/v2"
1415
"slices"
1516
"sync/atomic"
@@ -330,9 +331,19 @@ func (c *MySqlConnector) startSyncer(ctx context.Context, env map[string]string)
330331
if err != nil {
331332
return nil, fmt.Errorf("failed to get event cache count: %w", err)
332333
}
333-
//nolint:gosec
334+
335+
var serverId uint32
336+
if c.config.ServerId != nil {
337+
serverId = *c.config.ServerId
338+
} else {
339+
// If the configuration doesn't specify a server_id value, fallback to
340+
// default behavior of generating a random server_id in the range [1000, MaxUint32).
341+
// Range reference: https://dev.mysql.com/doc/refman/9.7/en/replication-options.html#sysvar_server_id
342+
serverId = 1000 + rand.Uint32()%(math.MaxUint32-1000) //nolint:gosec // G404: server_id does not require cryptographic randomness
343+
}
344+
334345
return replication.NewBinlogSyncer(replication.BinlogSyncerConfig{
335-
ServerID: rand.Uint32(),
346+
ServerID: serverId,
336347
Flavor: c.Flavor(),
337348
Host: config.Host,
338349
Port: uint16(config.Port),

flow/connectors/mysql/cdc_test.go

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/pingcap/tidb/pkg/parser"
1515
"github.com/pingcap/tidb/pkg/parser/ast"
1616
tidbmysql "github.com/pingcap/tidb/pkg/parser/mysql"
17+
"github.com/stretchr/testify/assert"
1718
"github.com/stretchr/testify/require"
1819
"go.opentelemetry.io/otel/metric"
1920
sdkmetric "go.opentelemetry.io/otel/sdk/metric"
@@ -23,6 +24,7 @@ import (
2324
"github.com/PeerDB-io/peerdb/flow/generated/protos"
2425
"github.com/PeerDB-io/peerdb/flow/model"
2526
"github.com/PeerDB-io/peerdb/flow/otel_metrics"
27+
mysql_validation "github.com/PeerDB-io/peerdb/flow/pkg/mysql"
2628
"github.com/PeerDB-io/peerdb/flow/shared"
2729
"github.com/PeerDB-io/peerdb/flow/shared/datatypes"
2830
"github.com/PeerDB-io/peerdb/flow/shared/types"
@@ -138,6 +140,38 @@ func TestIntegrationANSIQuotesDDLParsedFromBinlog(t *testing.T) {
138140
}
139141
}
140142

143+
// TestIntegrationConfiguredServerID verifies that an explicitly configured server_id is used for
144+
// the binlog connection instead of the legacy random fallback: the source must list a replica
145+
// registered with exactly that id.
146+
func TestIntegrationConfiguredServerID(t *testing.T) {
147+
for _, tc := range []struct {
148+
name string
149+
}{
150+
{name: "mysql"},
151+
{name: "mariadb"},
152+
} {
153+
t.Run(tc.name, func(t *testing.T) {
154+
t.Parallel()
155+
ctx := t.Context()
156+
connector := newTestConnector(t, ctx, tc.name)
157+
158+
wantServerID := uint32(1987)
159+
connector.config.ServerId = &wantServerID
160+
161+
// Establishing the stream sends COM_REGISTER_SLAVE with the configured server_id.
162+
// Ref: https://github.com/mysql/mysql-server/blob/6b6d3ed3d5c6591b446276184642d7d0504ecc86/include/my_command.h#L74
163+
startBinlogStream(t, ctx, connector)
164+
165+
// The source's replica list can lag briefly after registration, so retry until it shows up.
166+
require.EventuallyWithT(t, func(c *assert.CollectT) {
167+
found, err := mysql_validation.HasReplicaWithServerId(connector.conn.Load(), wantServerID)
168+
assert.NoError(c, err)
169+
assert.True(c, found, "source does not list a replica registered with server_id %d", wantServerID)
170+
}, 15*time.Second, time.Second)
171+
})
172+
}
173+
}
174+
141175
func TestParseSQLParsesTrailingNull(t *testing.T) {
142176
for _, tc := range []struct {
143177
name string

flow/e2e/api_test.go

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -325,6 +325,81 @@ func (s APITestSuite) TestClickHouseMirrorValidation_Pass() {
325325
require.NotNil(s.t, response)
326326
}
327327

328+
// TestValidateCDCMirror_ServerIDPeerReuse verifies that validating a CDC mirror whose MySQL source
329+
// peer pins a fixed server_id is rejected when that peer already backs another streaming CDC mirror.
330+
func (s APITestSuite) TestValidateCDCMirror_ServerIDPeerReuse() {
331+
mySource, ok := s.source.(*MySqlSource)
332+
if !ok {
333+
s.t.Skip("server_id peer-reuse validation is MySQL-specific")
334+
}
335+
ctx := s.t.Context()
336+
337+
// Source table backing the existing mirror.
338+
require.NoError(s.t, s.source.Exec(ctx,
339+
fmt.Sprintf("CREATE TABLE %s(id int primary key, val text)", AttachSchema(s, "serverid_reuse"))))
340+
341+
// A MySQL source peer that pins a fixed server_id. It clones the suite's MySQL config, so it
342+
// points at the same (reachable) instance.
343+
serverID := uint32(4224)
344+
pinnedConfig := proto.CloneOf(mySource.Config)
345+
pinnedConfig.ServerId = &serverID
346+
peerName := "serverid_reuse_" + s.suffix
347+
_, err := s.CreatePeer(ctx, &protos.CreatePeerRequest{
348+
Peer: &protos.Peer{
349+
Name: peerName,
350+
Type: protos.DBType_MYSQL,
351+
Config: &protos.Peer_MysqlConfig{MysqlConfig: pinnedConfig},
352+
},
353+
})
354+
require.NoError(s.t, err)
355+
356+
// An existing streaming CDC mirror that already uses the pinned-server_id peer as its source.
357+
existingGen := FlowConnectionGenerationConfig{
358+
FlowJobName: "serverid_reuse_existing_" + s.suffix,
359+
TableNameMapping: map[string]string{AttachSchema(s, "serverid_reuse"): "serverid_reuse"},
360+
Destination: s.ch.Peer().Name,
361+
}
362+
existingCfg := existingGen.GenerateFlowConnectionConfigs(s)
363+
existingCfg.SourceName = peerName
364+
existingCfg.DoInitialSnapshot = true
365+
createResp, err := s.CreateCDCFlow(ctx, &protos.CreateCDCFlowRequest{ConnectionConfigs: existingCfg})
366+
require.NoError(s.t, err)
367+
require.NotNil(s.t, createResp)
368+
369+
tc := NewTemporalClient(s.t)
370+
env, err := GetPeerflow(ctx, s.catalog, tc, existingCfg.FlowJobName)
371+
require.NoError(s.t, err)
372+
373+
// Validating a second CDC mirror on the same pinned-server_id peer must be rejected.
374+
newGen := FlowConnectionGenerationConfig{
375+
FlowJobName: "serverid_reuse_new_" + s.suffix,
376+
TableNameMapping: map[string]string{AttachSchema(s, "serverid_reuse"): "serverid_reuse"},
377+
Destination: s.ch.Peer().Name,
378+
}
379+
newCfg := newGen.GenerateFlowConnectionConfigs(s)
380+
newCfg.SourceName = peerName
381+
newCfg.DoInitialSnapshot = true
382+
383+
res, err := s.ValidateCDCMirror(ctx, &protos.CreateCDCFlowRequest{ConnectionConfigs: newCfg})
384+
require.Nil(s.t, res)
385+
require.Error(s.t, err)
386+
st, ok := status.FromError(err)
387+
require.True(s.t, ok)
388+
require.Equal(s.t, codes.FailedPrecondition, st.Code())
389+
require.Contains(s.t, st.Message(), "cannot be shared across mirrors")
390+
391+
// Tear down via gRPC: drop the mirror, then the now-unreferenced peer.
392+
_, err = s.FlowStateChange(ctx, &protos.FlowStateChangeRequest{
393+
FlowJobName: existingCfg.FlowJobName,
394+
RequestedFlowState: protos.FlowStatus_STATUS_TERMINATING,
395+
})
396+
require.NoError(s.t, err)
397+
s.waitForFlowDropped(env, existingCfg.FlowJobName)
398+
399+
_, err = s.DropPeer(ctx, &protos.DropPeerRequest{PeerName: peerName})
400+
require.NoError(s.t, err)
401+
}
402+
328403
func (s APITestSuite) TestClickHouseMirrorValidation_NoPrimaryKey() {
329404
switch s.source.(type) {
330405
case *PostgresSource, *MySqlSource:

flow/pkg/mysql/validation.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,3 +295,38 @@ func IsVitess(conn *client.Conn) (bool, error) {
295295
}
296296
return true, nil // is a Vitess server
297297
}
298+
299+
func HasReplicaWithServerId(conn *client.Conn, serverID uint32) (bool, error) {
300+
// This check assumes that the number of replicas is not beyond the order of magnitude of thousands.
301+
// This is a reasonable assumption given that services like RDS limit them to 15 and that
302+
// this number is constrained by concurrent connections and main server resources.
303+
rs, err := conn.Execute("SHOW REPLICAS")
304+
if err != nil {
305+
if rs, err = conn.Execute("SHOW SLAVE HOSTS"); err != nil {
306+
return false, fmt.Errorf("failed to query source for registered replicas")
307+
}
308+
}
309+
310+
serverIDCol := -1
311+
for i, field := range rs.Fields {
312+
// Case insensitive match because the column name differs between MySQL and MariaDB.
313+
if strings.EqualFold(string(field.Name), "Server_id") {
314+
serverIDCol = i
315+
break
316+
}
317+
}
318+
if serverIDCol < 0 {
319+
return false, fmt.Errorf("no Server_id column in replica list, got fields %v", rs.Fields)
320+
}
321+
322+
for row := range rs.RowNumber() {
323+
got, err := rs.GetInt(row, serverIDCol)
324+
if err != nil {
325+
return false, err
326+
}
327+
if uint32(got) == serverID {
328+
return true, nil
329+
}
330+
}
331+
return false, nil
332+
}

nexus/analyzer/src/lib.rs

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1134,6 +1134,7 @@ fn parse_db_options(db_type: DbType, with_options: &[SqlOption]) -> anyhow::Resu
11341134
}
11351135
.into(),
11361136
aws_auth: None,
1137+
server_id: opts.get("server_id").and_then(|s| s.parse::<u32>().ok()),
11371138
}),
11381139
DbType::DbtypeUnknown => return Ok(None),
11391140
}))

protos/peers.proto

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -234,6 +234,7 @@ message MySqlConfig {
234234
MySqlAuthType auth_type = 15;
235235
optional AwsAuthenticationConfig aws_auth = 16;
236236
bool skip_cert_verification = 17;
237+
optional uint32 server_id = 18;
237238
}
238239

239240
message KafkaConfig {

ui/app/peers/create/[peerType]/helpers/my.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,29 @@ export const mysqlSetting: PeerSetting[] = [
9898
{ value: 'MYSQL_FILEPOS', label: 'FilePos' },
9999
],
100100
},
101+
{
102+
label: 'Server ID',
103+
field: 'serverId',
104+
type: 'number',
105+
optional: true,
106+
stateHandler: (value, setter) => {
107+
const strValue = value as string;
108+
if (!strValue) {
109+
// Leave unset so a random server_id is generated on each CDC run (legacy behavior).
110+
setter((curr) => {
111+
const newCurr = { ...curr } as MySqlConfig;
112+
delete newCurr.serverId;
113+
return newCurr;
114+
});
115+
} else {
116+
setter((curr) => ({ ...curr, serverId: parseInt(strValue, 10) }));
117+
}
118+
},
119+
tips:
120+
'Optional. Fixed server_id used for the MySQL replication (binlog) connection. ' +
121+
'Leave blank to auto-generate a random id on each CDC run. A fixed server_id must be ' +
122+
'unique among replicas, so a peer that sets it cannot be reused across multiple mirrors.',
123+
},
101124
{
102125
label: 'Root Certificate',
103126
stateHandler: (value, setter) => {

0 commit comments

Comments
 (0)