Skip to content

Commit 907cbe1

Browse files
committed
Merge branch 'main' into qualified-table-identifiers
Ports #4414 (shared destinations / N:1 mappings) to QualifiedTable structs: - processTableRemovals keys remainingDestinations/exclusivelyRemovedTables by struct instead of the (cleared) legacy string identifiers - RemoveTablesFromRawTable dedupes destination structs + keeps main's early exit - ClickHouse RemoveTableEntriesFromRawTable chunks IN-lists over LegacyDotted names - validateTableMappingIdentifiers now permits exact-duplicate destinations (N:1 is supported per #4414); dotted-rendering collisions between DIFFERENT destinations are still rejected; e2e duplicate-destination rejection cases dropped accordingly - ui schema.ts resolved to main's version (duplicate-destination check removed)
2 parents fc32c4e + 3c4134b commit 907cbe1

7 files changed

Lines changed: 150 additions & 58 deletions

File tree

flow/activities/flowable.go

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1916,6 +1916,26 @@ func (a *FlowableActivity) RemoveTablesFromRawTable(
19161916
return a.Alerter.LogFlowError(ctx, cfg.FlowJobName, err)
19171917
}
19181918

1919+
// N:1 mappings can list the same destination more than once; dedupe by struct
1920+
destinationTables := make([]*protos.QualifiedTable, 0, len(tablesToRemove))
1921+
seenDestinations := make(map[common.QualifiedTable]struct{}, len(tablesToRemove))
1922+
for _, table := range tablesToRemove {
1923+
key := internal.QualifiedTableFromProto(table.DestinationTable)
1924+
if _, ok := seenDestinations[key]; !ok {
1925+
seenDestinations[key] = struct{}{}
1926+
destinationTables = append(destinationTables, table.DestinationTable)
1927+
}
1928+
}
1929+
slices.SortFunc(destinationTables, func(a, b *protos.QualifiedTable) int {
1930+
return internal.CompareQualifiedTables(internal.QualifiedTableFromProto(a), internal.QualifiedTableFromProto(b))
1931+
})
1932+
if len(destinationTables) == 0 || syncBatchID <= normBatchID {
1933+
logger.Info("[RemoveTablesFromRawTable] no pending raw rows to remove, skipping",
1934+
slog.Int64("syncBatchID", syncBatchID), slog.Int64("normalizeBatchID", normBatchID),
1935+
slog.Int("tables", len(destinationTables)))
1936+
return nil
1937+
}
1938+
19191939
dstConn, dstClose, err := connectors.GetByNameAs[connectors.RawTableConnector](ctx, cfg.Env, a.CatalogPool, cfg.DestinationName)
19201940
if err != nil {
19211941
if errors.Is(err, errors.ErrUnsupported) {
@@ -1929,10 +1949,6 @@ func (a *FlowableActivity) RemoveTablesFromRawTable(
19291949
}
19301950
defer dstClose(ctx)
19311951

1932-
destinationTables := make([]*protos.QualifiedTable, 0, len(tablesToRemove))
1933-
for _, table := range tablesToRemove {
1934-
destinationTables = append(destinationTables, table.DestinationTable)
1935-
}
19361952
if err := dstConn.RemoveTableEntriesFromRawTable(ctx, &protos.RemoveTablesFromRawTableInput{
19371953
FlowJobName: cfg.FlowJobName,
19381954
DestinationTables: destinationTables,

flow/cmd/validate_mirror.go

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -176,15 +176,15 @@ func (h *FlowRequestHandler) validateCDCMirrorImpl(
176176
return &protos.ValidateCDCMirrorResponse{}, nil
177177
}
178178

179-
// validateTableMappingIdentifiers rejects duplicate sources/destinations, empty table
180-
// components, and source/destination pairs whose LegacyDotted renderings collide (e.g.
179+
// validateTableMappingIdentifiers rejects duplicate sources, empty table components,
180+
// and source/destination pairs whose LegacyDotted renderings collide (e.g.
181181
// {"a","b.c"} vs {"a.b","c"}) — colliding destinations would merge in the raw table,
182182
// which stores the dotted format, and colliding sources would produce duplicate
183-
// snapshot clone child-workflow IDs.
183+
// snapshot clone child-workflow IDs. The exact same destination MAY repeat: N:1
184+
// mappings (multiple sources sharing one destination) are supported.
184185
func validateTableMappingIdentifiers(tableMappings []*protos.TableMapping) error {
185186
sources := make(map[common.QualifiedTable]struct{}, len(tableMappings))
186187
sourcesDotted := make(map[string]common.QualifiedTable, len(tableMappings))
187-
destinations := make(map[common.QualifiedTable]struct{}, len(tableMappings))
188188
destinationsDotted := make(map[string]common.QualifiedTable, len(tableMappings))
189189
for _, tm := range tableMappings {
190190
source := internal.QualifiedTableFromProto(tm.SourceTable)
@@ -207,11 +207,7 @@ func validateTableMappingIdentifiers(tableMappings []*protos.TableMapping) error
207207
source, other)
208208
}
209209
sourcesDotted[source.LegacyDotted()] = source
210-
if _, ok := destinations[destination]; ok {
211-
return fmt.Errorf("duplicate destination table %s", destination)
212-
}
213-
destinations[destination] = struct{}{}
214-
if other, ok := destinationsDotted[destination.LegacyDotted()]; ok {
210+
if other, ok := destinationsDotted[destination.LegacyDotted()]; ok && other != destination {
215211
return fmt.Errorf("destination tables %s and %s are ambiguous with each other due to dots in names",
216212
destination, other)
217213
}

flow/connectors/clickhouse/cdc.go

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"fmt"
66
"log/slog"
7+
"slices"
78
"strconv"
89
"strings"
910

@@ -423,20 +424,24 @@ func (c *ClickHouseConnector) RemoveTableEntriesFromRawTable(
423424
return nil
424425
}
425426

426-
for _, table := range destinationTables {
427+
// chunk to bound statement size, one mutation covers many tables instead of one mutation each
428+
for chunk := range slices.Chunk(destinationTables, 100) {
429+
quoted := make([]string, len(chunk))
430+
for i, table := range chunk {
431+
// LegacyDotted: _peerdb_destination_table_name values were written in the
432+
// dotted format, by this and older releases
433+
quoted[i] = peerdb_clickhouse.QuoteLiteral(table.LegacyDotted())
434+
}
427435
// Better to use lightweight deletes here as the main goal is to not have
428436
// rows in the table be visible by the NormalizeRecords' INSERT INTO SELECT queries
429-
// LegacyDotted: _peerdb_destination_table_name values were written in the dotted
430-
// format, by this and older releases
431-
if err := c.execWithLogging(ctx, fmt.Sprintf("DELETE FROM %s WHERE _peerdb_destination_table_name = %s"+
437+
if err := c.execWithLogging(ctx, fmt.Sprintf("DELETE FROM %s WHERE _peerdb_destination_table_name IN (%s)"+
432438
" AND _peerdb_batch_id > %d AND _peerdb_batch_id <= %d",
433-
c.GetRawTableName(req.FlowJobName), peerdb_clickhouse.QuoteLiteral(table.LegacyDotted()),
434-
req.NormalizeBatchId, req.SyncBatchId),
439+
c.GetRawTableName(req.FlowJobName), strings.Join(quoted, ","), req.NormalizeBatchId, req.SyncBatchId),
435440
); err != nil {
436-
return fmt.Errorf("unable to remove table %s from raw table: %w", table, err)
441+
return fmt.Errorf("unable to remove tables from raw table: %w", err)
437442
}
438443

439-
c.logger.Info("successfully removed entries for table from raw table", slog.String("table", table.String()))
444+
c.logger.Info("successfully removed entries for tables from raw table", slog.Any("tables", chunk))
440445
}
441446

442447
return nil

flow/e2e/clickhouse_test.go

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,98 @@ func (s ClickHouseSuite) Test_Addition_Removal() {
209209
RequireEnvCanceled(s.t, env)
210210
}
211211

212+
// Removing one of several source tables feeding a shared destination must keep
213+
// that destination's raw rows & catalog schema mapping intact, otherwise the
214+
// surviving source can no longer normalize into it. Disjoint id ranges keep the
215+
// two sources' rows distinct under the destination's ReplacingMergeTree.
216+
func (s ClickHouseSuite) Test_Removal_Shared_Destination() {
217+
tc := NewTemporalClient(s.t)
218+
219+
srcTableA := s.attachSchemaSuffix("test_shared_dst_a")
220+
srcTableB := s.attachSchemaSuffix("test_shared_dst_b")
221+
dstTableName := "test_shared_dst_target"
222+
223+
for _, srcTableName := range []string{srcTableA, srcTableB} {
224+
require.NoError(s.t, s.source.Exec(s.t.Context(), fmt.Sprintf(`
225+
CREATE TABLE IF NOT EXISTS %s (
226+
id INT PRIMARY KEY,
227+
"key" TEXT NOT NULL
228+
);
229+
`, srcTableName)))
230+
}
231+
232+
connectionGen := FlowConnectionGenerationConfig{
233+
FlowJobName: s.attachSuffix("clickhousesharedremoval"),
234+
TableNameMapping: map[string]string{srcTableA: dstTableName, srcTableB: dstTableName},
235+
Destination: s.Peer().Name,
236+
}
237+
238+
flowConnConfig := connectionGen.GenerateFlowConnectionConfigs(s)
239+
flowConnConfig.MaxBatchSize = 1
240+
241+
env := ExecutePeerflow(s.t, tc, flowConnConfig)
242+
243+
SetupCDCFlowStatusQuery(s.t, env, flowConnConfig)
244+
require.NoError(s.t, s.source.Exec(s.t.Context(),
245+
fmt.Sprintf(`INSERT INTO %s (id,"key") VALUES (1,'a1')`, srcTableA)))
246+
require.NoError(s.t, s.source.Exec(s.t.Context(),
247+
fmt.Sprintf(`INSERT INTO %s (id,"key") VALUES (101,'b1')`, srcTableB)))
248+
EnvWaitForCount(env, s, "both sources reach shared destination", dstTableName, "id,\"key\"", 2)
249+
250+
SignalWorkflow(s.t.Context(), env, model.FlowSignal, model.PauseSignal)
251+
EnvWaitFor(s.t, env, 3*time.Minute, "pausing for removing shared table", func() bool {
252+
return env.GetFlowStatus(s.t) == protos.FlowStatus_STATUS_PAUSED
253+
})
254+
255+
if pgconn, ok := s.source.Connector().(*connpostgres.PostgresConnector); ok {
256+
conn := pgconn.Conn()
257+
_, err := conn.Exec(s.t.Context(),
258+
fmt.Sprintf(`SELECT pg_terminate_backend(pid) FROM pg_stat_activity
259+
WHERE query LIKE '%%START_REPLICATION%%' AND query LIKE '%%%s%%' AND backend_type='walsender'`,
260+
s.attachSuffix("clickhousesharedremoval")))
261+
require.NoError(s.t, err)
262+
263+
EnvWaitFor(s.t, env, 3*time.Minute, "waiting for replication to stop", func() bool {
264+
rows, err := conn.Query(s.t.Context(),
265+
fmt.Sprintf(`SELECT pid FROM pg_stat_activity
266+
WHERE query LIKE '%%START_REPLICATION%%' AND query LIKE '%%%s%%' AND backend_type='walsender'`,
267+
s.attachSuffix("clickhousesharedremoval")))
268+
require.NoError(s.t, err)
269+
defer rows.Close()
270+
return !rows.Next()
271+
})
272+
}
273+
274+
runID := EnvGetRunID(s.t, env)
275+
SignalWorkflow(s.t.Context(), env, model.CDCDynamicPropertiesSignal, &protos.CDCFlowConfigUpdate{
276+
RemovedTables: []*protos.TableMapping{{
277+
SourceTableIdentifier: srcTableA,
278+
DestinationTableIdentifier: dstTableName,
279+
}},
280+
})
281+
282+
EnvWaitFor(s.t, env, 4*time.Minute, "removing shared source", func() bool {
283+
return env.GetFlowStatus(s.t) == protos.FlowStatus_STATUS_RUNNING
284+
})
285+
EnvWaitFor(s.t, env, time.Minute, "ContinueAsNew", func() bool {
286+
return runID != EnvGetRunID(s.t, env)
287+
})
288+
289+
// removed source: must not replicate; surviving source: must still reach shared destination
290+
require.NoError(s.t, s.source.Exec(s.t.Context(),
291+
fmt.Sprintf(`INSERT INTO %s (id,"key") VALUES (2,'a2')`, srcTableA)))
292+
require.NoError(s.t, s.source.Exec(s.t.Context(),
293+
fmt.Sprintf(`INSERT INTO %s (id,"key") VALUES (102,'b2')`, srcTableB)))
294+
295+
EnvWaitForCount(env, s, "surviving source still reaches shared destination", dstTableName, "id,\"key\"", 3)
296+
297+
rows, err := s.GetRows(dstTableName, "id")
298+
require.NoError(s.t, err)
299+
require.Len(s.t, rows.Records, 3, "removed source must not leak into shared destination")
300+
env.Cancel(s.t.Context())
301+
RequireEnvCanceled(s.t, env)
302+
}
303+
212304
func (s ClickHouseSuite) Test_NullableMirrorSetting() {
213305
srcTableName := "test_nullable_mirror"
214306
srcFullName := s.attachSchemaSuffix(srcTableName)

flow/e2e/dotted_names_test.go

Lines changed: 3 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -264,20 +264,14 @@ func (s APITestSuite) TestDottedTableAddition() {
264264
fmt.Sprintf("INSERT INTO %s(id, val) VALUES (2,'cdc')", dottedSrcQuoted)))
265265
EnvWaitForEqualTablesWithNames(env, s.ch, "added dotted table cdc", dottedTable, dottedDst, "id,val")
266266

267-
// additions must also be validated against the flow's EXISTING tables
267+
// additions must also be validated against the flow's EXISTING tables (exact
268+
// duplicates are allowed — N:1 mappings — but dotted collisions between
269+
// DIFFERENT destinations are not)
268270
for _, tc := range []struct {
269271
name string
270272
mapping *protos.TableMapping
271273
errContains string
272274
}{
273-
{
274-
name: "duplicate of existing destination",
275-
mapping: &protos.TableMapping{
276-
SourceTable: &protos.QualifiedTable{Namespace: Schema(s), Table: "dotadd_other"},
277-
DestinationTable: &protos.QualifiedTable{Table: baseTable},
278-
},
279-
errContains: "duplicate destination table",
280-
},
281275
{
282276
name: "dotted collision with existing destination",
283277
mapping: &protos.TableMapping{
@@ -321,20 +315,6 @@ func (s APITestSuite) TestMirrorValidation_DottedIdentifierCollisions() {
321315
mappings []*protos.TableMapping
322316
errContains string
323317
}{
324-
{
325-
name: "duplicate destination structs",
326-
mappings: []*protos.TableMapping{
327-
{
328-
SourceTable: &protos.QualifiedTable{Namespace: Schema(s), Table: "vsrc1"},
329-
DestinationTable: &protos.QualifiedTable{Table: "vdst"},
330-
},
331-
{
332-
SourceTable: &protos.QualifiedTable{Namespace: Schema(s), Table: "vsrc2"},
333-
DestinationTable: &protos.QualifiedTable{Table: "vdst"},
334-
},
335-
},
336-
errContains: "duplicate destination table",
337-
},
338318
{
339319
name: "legacy dotted collision pair",
340320
mappings: []*protos.TableMapping{

flow/workflows/cdc_flow.go

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -385,10 +385,24 @@ func processTableRemovals(
385385
}
386386
logger.Info("tables removed from publication")
387387

388+
// destinations still fed by remaining mappings keep raw rows & schema mapping,
389+
// pending rows still normalize into surviving destination table
390+
// relies on TableMappings being trimmed before selector callbacks fire
391+
remainingDestinations := make(map[common.QualifiedTable]struct{}, len(state.SyncFlowOptions.TableMappings))
392+
for _, tm := range state.SyncFlowOptions.TableMappings {
393+
remainingDestinations[internal.QualifiedTableFromProto(tm.DestinationTable)] = struct{}{}
394+
}
395+
exclusivelyRemovedTables := make([]*protos.TableMapping, 0, len(state.FlowConfigUpdate.RemovedTables))
396+
for _, tm := range state.FlowConfigUpdate.RemovedTables {
397+
if _, shared := remainingDestinations[internal.QualifiedTableFromProto(tm.DestinationTable)]; !shared {
398+
exclusivelyRemovedTables = append(exclusivelyRemovedTables, tm)
399+
}
400+
}
401+
388402
rawTableCleanupFuture := workflow.ExecuteActivity(
389403
removeTablesCtx,
390404
flowable.RemoveTablesFromRawTable,
391-
cfg, state.FlowConfigUpdate.RemovedTables)
405+
cfg, exclusivelyRemovedTables)
392406
removeTablesSelector.AddFuture(rawTableCleanupFuture, func(f workflow.Future) {
393407
if err := f.Get(ctx, nil); err != nil {
394408
logger.Error("failed to clean up raw table for removed tables", slog.Any("error", err))
@@ -400,7 +414,7 @@ func processTableRemovals(
400414
removeTablesFromCatalogFuture := workflow.ExecuteActivity(
401415
removeTablesCtx,
402416
flowable.RemoveTablesFromCatalog,
403-
cfg, state.FlowConfigUpdate.RemovedTables)
417+
cfg, exclusivelyRemovedTables)
404418
removeTablesSelector.AddFuture(removeTablesFromCatalogFuture, func(f workflow.Future) {
405419
if err := f.Get(ctx, nil); err != nil {
406420
logger.Error("failed to clean up raw table for removed tables", slog.Any("error", err))

ui/app/mirrors/create/schema.ts

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -38,18 +38,7 @@ export const tableMappingSchema = z
3838
partitionKey: z.string().optional(),
3939
})
4040
)
41-
.nonempty('At least one table mapping is required')
42-
.superRefine((mappingArray, ctx) => {
43-
const destinations = mappingArray.map(
44-
(val) => `${val.destinationTable.namespace}.${val.destinationTable.table}`
45-
);
46-
if (destinations.length !== new Set(destinations).size) {
47-
ctx.addIssue({
48-
code: 'custom',
49-
message: `Two source tables have been mapped to the same destination table`,
50-
});
51-
}
52-
});
41+
.nonempty('At least one table mapping is required');
5342

5443
export const cdcSchema = z.object({
5544
sourceName: z.string({ error: 'Source peer is required' }).min(1),

0 commit comments

Comments
 (0)