Skip to content

Commit 534b2e0

Browse files
committed
Fix adversarial review findings
- Table-only destinations (ClickHouse, Elasticsearch) derive physical names via LegacyDotted so legacy dotted destinations keep targeting the same table/index - Add-tables signal path validates additions against the flow's existing tables; structural identifier validation runs even with skip_validation; source dotted collisions rejected (duplicate clone workflow IDs) - Missing entry normalization added: RemoveTablesFromRawTable, QRepHasNewRows, SetupQRepMetadataTables activities and QRepWaitForNewRowsWorkflow - Legacy fallbacks when consuming activity outputs recorded by old releases - Continue-As-New payloads denormalized for rollback safety (continue_as_new.go) - InitialLoadSummary recomposes LegacyDotted names from qrep_runs split columns - CancelTableAddition response denormalized; telemetry reads structs; reset_sequences handles empty-namespace destinations; ScopedEventhub rejects extra dots; QRep watermark validation; UI QRep destination re-parsed at submit; UI removals reuse canonical mappings; delete ParseTableIdentifier (plan 1.3)
1 parent 83e08a1 commit 534b2e0

27 files changed

Lines changed: 268 additions & 92 deletions

flow/activities/flowable.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -546,6 +546,7 @@ func (a *FlowableActivity) pullAndSyncPg(
546546

547547
// SetupQRepMetadataTables sets up the metadata tables for QReplication.
548548
func (a *FlowableActivity) SetupQRepMetadataTables(ctx context.Context, config *protos.QRepConfig) error {
549+
internal.NormalizeQRepConfig(config)
549550
conn, connClose, err := connectors.GetByNameAs[connectors.QRepSyncConnector](ctx, config.Env, a.CatalogPool, config.DestinationName)
550551
if err != nil {
551552
return a.Alerter.LogFlowError(ctx, config.FlowJobName, fmt.Errorf("failed to get connector: %w", err))
@@ -1597,6 +1598,8 @@ var activeFlowStatuses = map[protos.FlowStatus]struct{}{
15971598
func (a *FlowableActivity) QRepHasNewRows(ctx context.Context,
15981599
config *protos.QRepConfig, last *protos.QRepPartition,
15991600
) (bool, error) {
1601+
internal.NormalizeQRepConfig(config)
1602+
internal.NormalizeQRepPartition(last)
16001603
shutdown := common.HeartbeatRoutine(ctx, func() string {
16011604
return "scanning for new rows"
16021605
})
@@ -1893,6 +1896,7 @@ func (a *FlowableActivity) RemoveTablesFromRawTable(
18931896
cfg *protos.FlowConnectionConfigsCore,
18941897
tablesToRemove []*protos.TableMapping,
18951898
) error {
1899+
internal.NormalizeTableMappings(tablesToRemove)
18961900
shutdown := common.HeartbeatRoutine(ctx, func() string {
18971901
return "removing tables from raw table"
18981902
})

flow/cmd/cancel_table_addition.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,5 +60,8 @@ func (h *FlowRequestHandler) CancelTableAddition(
6060
slog.String("flowJobName", req.FlowJobName),
6161
slog.String("workflowID", workflowID))
6262

63+
// populate legacy string identifiers for clients that predate QualifiedTable
64+
internal.DenormalizeTableMappings(output.GetTablesAfterCancellation())
65+
6366
return output, nil
6467
}

flow/cmd/handler.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,6 +218,11 @@ func (h *FlowRequestHandler) CreateCDCFlow(
218218
slog.ErrorContext(ctx, "validate mirror error", slog.Any("error", err))
219219
return nil, NewInternalApiError(fmt.Errorf("invalid mirror: %w", err))
220220
}
221+
} else if err := validateTableMappingIdentifiers(connectionConfigsCore.TableMappings); err != nil {
222+
// SkipValidation skips environmental checks only; structural identifier
223+
// invariants (duplicates, dotted-name collisions, empty components) are
224+
// relied on by the raw-table round-trip and must always hold
225+
return nil, NewInvalidArgumentApiError(err)
221226
}
222227

223228
if resp, err := h.createCDCFlow(ctx, connectionConfigsCore, workflowID); err != nil {
@@ -467,6 +472,20 @@ func (h *FlowRequestHandler) FlowStateChange(
467472
if err := validateTableMappingIdentifiers(cdcUpdate.AdditionalTables); err != nil {
468473
return nil, NewInvalidArgumentApiError(err)
469474
}
475+
if len(cdcUpdate.AdditionalTables) > 0 {
476+
// added tables must not duplicate or dotted-collide with the flow's
477+
// EXISTING tables either, not just within the additions batch
478+
config, err := h.getFlowConfigFromCatalog(ctx, req.FlowJobName)
479+
if err != nil {
480+
return nil, NewInternalApiError(fmt.Errorf("unable to load flow config to validate additional tables: %w", err))
481+
}
482+
combined := make([]*protos.TableMapping, 0, len(config.TableMappings)+len(cdcUpdate.AdditionalTables))
483+
combined = append(combined, config.TableMappings...)
484+
combined = append(combined, cdcUpdate.AdditionalTables...)
485+
if err := validateTableMappingIdentifiers(combined); err != nil {
486+
return nil, NewInvalidArgumentApiError(fmt.Errorf("additional tables conflict with existing tables: %w", err))
487+
}
488+
}
470489
}
471490

472491
if req.FlowConfigUpdate != nil && req.FlowConfigUpdate.GetCdcFlowConfigUpdate() != nil &&

flow/cmd/mirror_status.go

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/PeerDB-io/peerdb/flow/connectors"
1919
"github.com/PeerDB-io/peerdb/flow/generated/protos"
2020
"github.com/PeerDB-io/peerdb/flow/internal"
21+
"github.com/PeerDB-io/peerdb/flow/pkg/common"
2122
"github.com/PeerDB-io/peerdb/flow/shared"
2223
"github.com/PeerDB-io/peerdb/flow/shared/exceptions"
2324
"github.com/PeerDB-io/peerdb/flow/workflows/cdc_state"
@@ -300,7 +301,9 @@ func (h *FlowRequestHandler) InitialLoadSummary(
300301
q := `
301302
SELECT
302303
distinct qr.flow_name,
304+
qr.destination_table_namespace,
303305
qr.destination_table,
306+
qr.source_table_namespace,
304307
qr.source_table,
305308
qr.start_time AS StartTime,
306309
qr.fetch_complete as FetchCompleted,
@@ -312,10 +315,13 @@ func (h *FlowRequestHandler) InitialLoadSummary(
312315
FROM peerdb_stats.qrep_partitions qp
313316
RIGHT JOIN peerdb_stats.qrep_runs qr ON qp.flow_name = qr.flow_name
314317
WHERE qr.parent_mirror_name = $1
315-
GROUP BY qr.flow_name, qr.destination_table, qr.source_table, qr.start_time, qr.fetch_complete, qr.consolidate_complete;
318+
GROUP BY qr.flow_name, qr.destination_table_namespace, qr.destination_table,
319+
qr.source_table_namespace, qr.source_table, qr.start_time, qr.fetch_complete, qr.consolidate_complete;
316320
`
317321
var flowName pgtype.Text
322+
var destinationTableNamespace pgtype.Text
318323
var destinationTable pgtype.Text
324+
var sourceTableNamespace pgtype.Text
319325
var sourceTable pgtype.Text
320326
var fetchCompleted pgtype.Bool
321327
var consolidateCompleted pgtype.Bool
@@ -338,7 +344,9 @@ func (h *FlowRequestHandler) InitialLoadSummary(
338344
for rows.Next() {
339345
if err := rows.Scan(
340346
&flowName,
347+
&destinationTableNamespace,
341348
&destinationTable,
349+
&sourceTableNamespace,
342350
&sourceTable,
343351
&startTime,
344352
&fetchCompleted,
@@ -360,11 +368,11 @@ func (h *FlowRequestHandler) InitialLoadSummary(
360368
}
361369

362370
if destinationTable.Valid {
363-
res.TableName = destinationTable.String
371+
res.TableName = common.QualifiedTable{Namespace: destinationTableNamespace.String, Table: destinationTable.String}.LegacyDotted()
364372
}
365373

366374
if sourceTable.Valid {
367-
res.SourceTable = sourceTable.String
375+
res.SourceTable = common.QualifiedTable{Namespace: sourceTableNamespace.String, Table: sourceTable.String}.LegacyDotted()
368376
}
369377

370378
if startTime.Valid {

flow/cmd/reset_sequences.go

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
connpostgres "github.com/PeerDB-io/peerdb/flow/connectors/postgres"
1111
"github.com/PeerDB-io/peerdb/flow/generated/protos"
1212
"github.com/PeerDB-io/peerdb/flow/internal"
13+
"github.com/PeerDB-io/peerdb/flow/pkg/common"
1314
)
1415

1516
func (h *FlowRequestHandler) ResetMirrorSequences(
@@ -48,7 +49,13 @@ func (h *FlowRequestHandler) ResetMirrorSequences(
4849
for _, tm := range config.TableMappings {
4950
// quoted form: pg_get_serial_sequence parses its argument as a possibly-quoted
5051
// qualified name, so this stays correct for names containing dots
51-
destTables = append(destTables, internal.QualifiedTableFromProto(tm.DestinationTable).String())
52+
destTable := internal.QualifiedTableFromProto(tm.DestinationTable)
53+
if destTable.Namespace == "" {
54+
// unqualified names resolve via search_path; `""."t"` would be invalid
55+
destTables = append(destTables, common.QuoteIdentifier(destTable.Table))
56+
} else {
57+
destTables = append(destTables, destTable.String())
58+
}
5259
}
5360

5461
quotedTables := make([]string, 0, len(destTables))

flow/cmd/validate_mirror.go

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,9 @@ func validateQRepIdentifiers(cfg *protos.QRepConfig) error {
3838
if cfg.DestinationTable.GetTable() == "" {
3939
return errors.New("destination table name is empty")
4040
}
41+
if cfg.WatermarkColumn != "" && cfg.QualifiedWatermarkTable.GetTable() == "" {
42+
return errors.New("watermark table is required when a watermark column is configured")
43+
}
4144
return nil
4245
}
4346

@@ -174,11 +177,13 @@ func (h *FlowRequestHandler) validateCDCMirrorImpl(
174177
}
175178

176179
// validateTableMappingIdentifiers rejects duplicate sources/destinations, empty table
177-
// components, and destination pairs whose LegacyDotted renderings collide (e.g.
178-
// {"a","b.c"} vs {"a.b","c"}) — those would merge in the raw table, which stores the
179-
// dotted format.
180+
// components, and source/destination pairs whose LegacyDotted renderings collide (e.g.
181+
// {"a","b.c"} vs {"a.b","c"}) — colliding destinations would merge in the raw table,
182+
// which stores the dotted format, and colliding sources would produce duplicate
183+
// snapshot clone child-workflow IDs.
180184
func validateTableMappingIdentifiers(tableMappings []*protos.TableMapping) error {
181185
sources := make(map[common.QualifiedTable]struct{}, len(tableMappings))
186+
sourcesDotted := make(map[string]common.QualifiedTable, len(tableMappings))
182187
destinations := make(map[common.QualifiedTable]struct{}, len(tableMappings))
183188
destinationsDotted := make(map[string]common.QualifiedTable, len(tableMappings))
184189
for _, tm := range tableMappings {
@@ -197,6 +202,11 @@ func validateTableMappingIdentifiers(tableMappings []*protos.TableMapping) error
197202
return fmt.Errorf("duplicate source table %s", source)
198203
}
199204
sources[source] = struct{}{}
205+
if other, ok := sourcesDotted[source.LegacyDotted()]; ok {
206+
return fmt.Errorf("source tables %s and %s are ambiguous with each other due to dots in names",
207+
source, other)
208+
}
209+
sourcesDotted[source.LegacyDotted()] = source
200210
if _, ok := destinations[destination]; ok {
201211
return fmt.Errorf("duplicate destination table %s", destination)
202212
}

flow/connectors/clickhouse/avro_sync.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ func (s *ClickHouseAvroSyncMethod) CopyStageToDestination(ctx context.Context, a
5050
}
5151

5252
query := fmt.Sprintf("INSERT INTO %s SELECT * FROM %s",
53-
peerdb_clickhouse.QuoteIdentifier(s.config.DestinationTable.GetTable()), stagingTableFunction)
53+
peerdb_clickhouse.QuoteIdentifier(internal.QualifiedTableFromProto(s.config.DestinationTable).LegacyDotted()), stagingTableFunction)
5454
return s.exec(ctx, query)
5555
}
5656

@@ -61,7 +61,7 @@ func (s *ClickHouseAvroSyncMethod) SyncRecords(
6161
flowJobName string,
6262
syncBatchID int64,
6363
) (int64, error) {
64-
dstTableName := internal.QualifiedTableFromProto(s.config.DestinationTable).Table
64+
dstTableName := internal.QualifiedTableFromProto(s.config.DestinationTable).LegacyDotted()
6565

6666
schema, err := stream.Schema()
6767
if err != nil {
@@ -101,7 +101,7 @@ func (s *ClickHouseAvroSyncMethod) SyncQRepRecords(
101101
stream *model.QRecordStream,
102102
) (int64, shared.QRepWarnings, error) {
103103
destinationTable := internal.QualifiedTableFromProto(config.DestinationTable)
104-
dstTableName := destinationTable.Table
104+
dstTableName := destinationTable.LegacyDotted()
105105
startTime := time.Now()
106106
schema, err := stream.Schema()
107107
if err != nil {
@@ -244,7 +244,7 @@ func (s *ClickHouseAvroSyncMethod) pushStagingDataToClickHouseForSnapshot(
244244
config *protos.QRepConfig,
245245
) error {
246246
insertConfig := &insertFromTableFunctionConfig{
247-
destinationTable: config.DestinationTable.GetTable(),
247+
destinationTable: internal.QualifiedTableFromProto(config.DestinationTable).LegacyDotted(),
248248
schema: schema,
249249
columnNameMap: columnNameAvroFieldMap,
250250
excludedColumns: config.Exclude,
@@ -318,7 +318,8 @@ func (s *ClickHouseAvroSyncMethod) pushStagingDataToClickHouseForSnapshot(
318318
slog.Uint64("numParts", numParts),
319319
slog.Int("chunkIdx", chunkIdx),
320320
slog.Any("error", err))
321-
return exceptions.NewClickHouseQRepSyncError(err, config.DestinationTable.GetTable(), s.ClickHouseConnector.Config.Database)
321+
return exceptions.NewClickHouseQRepSyncError(
322+
err, internal.QualifiedTableFromProto(config.DestinationTable).LegacyDotted(), s.ClickHouseConnector.Config.Database)
322323
}
323324

324325
s.logger.Info("inserted part",

flow/connectors/clickhouse/cdc.go

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ func (c *ClickHouseConnector) ReplayTableSchemaDeltas(
261261
var shardTableName string
262262
if c.Config.Cluster != "" && (tm == nil || tm.Engine != protos.TableEngine_CH_ENGINE_NULL) {
263263
var err error
264-
shardTableName, err = c.getDistributedShardTable(ctx, dstTable.Table)
264+
shardTableName, err = c.getDistributedShardTable(ctx, dstTable.LegacyDotted())
265265
if err != nil {
266266
return fmt.Errorf("failed to resolve shard table for %s: %w", dstTable, err)
267267
}
@@ -288,7 +288,7 @@ func (c *ClickHouseConnector) ReplayTableSchemaDeltas(
288288

289289
if err := c.execWithLogging(ctx,
290290
fmt.Sprintf("ALTER TABLE %s%s ADD COLUMN IF NOT EXISTS %s %s",
291-
peerdb_clickhouse.QuoteIdentifier(dstTable.Table), onCluster,
291+
peerdb_clickhouse.QuoteIdentifier(dstTable.LegacyDotted()), onCluster,
292292
peerdb_clickhouse.QuoteIdentifier(addedColumn.Name), clickHouseColType),
293293
); err != nil {
294294
return fmt.Errorf("failed to add column %s for table %s: %w", addedColumn.Name, dstTable, err)
@@ -321,7 +321,7 @@ func (c *ClickHouseConnector) RenameTables(
321321
continue
322322
}
323323

324-
resyncTableExists, err := c.checkIfTableExists(ctx, c.Config.Database, currentTable.Table)
324+
resyncTableExists, err := c.checkIfTableExists(ctx, c.Config.Database, currentTable.LegacyDotted())
325325
if err != nil {
326326
return nil, fmt.Errorf("unable to check if resync table %s exists: %w", currentTable, err)
327327
}
@@ -331,7 +331,7 @@ func (c *ClickHouseConnector) RenameTables(
331331
continue
332332
}
333333

334-
originalTableExists, err := c.checkIfTableExists(ctx, c.Config.Database, newTable.Table)
334+
originalTableExists, err := c.checkIfTableExists(ctx, c.Config.Database, newTable.LegacyDotted())
335335
if err != nil {
336336
return nil, fmt.Errorf("unable to check if table %s exists: %w", newTable, err)
337337
}
@@ -342,11 +342,11 @@ func (c *ClickHouseConnector) RenameTables(
342342
c.logger.Info("attempting atomic exchange",
343343
slog.String("OldName", currentTable.String()), slog.String("NewName", newTable.String()))
344344
if err = c.execWithLogging(ctx,
345-
fmt.Sprintf("EXCHANGE TABLES %s and %s%s", peerdb_clickhouse.QuoteIdentifier(newTable.Table),
346-
peerdb_clickhouse.QuoteIdentifier(currentTable.Table), onCluster),
345+
fmt.Sprintf("EXCHANGE TABLES %s and %s%s", peerdb_clickhouse.QuoteIdentifier(newTable.LegacyDotted()),
346+
peerdb_clickhouse.QuoteIdentifier(currentTable.LegacyDotted()), onCluster),
347347
); err == nil {
348348
if err := c.execWithLogging(ctx,
349-
fmt.Sprintf(dropTableSQLWithCHSetting, peerdb_clickhouse.QuoteIdentifier(currentTable.Table), onCluster),
349+
fmt.Sprintf(dropTableSQLWithCHSetting, peerdb_clickhouse.QuoteIdentifier(currentTable.LegacyDotted()), onCluster),
350350
); err != nil {
351351
return nil, fmt.Errorf("unable to drop exchanged table %s: %w", currentTable, err)
352352
}
@@ -361,14 +361,14 @@ func (c *ClickHouseConnector) RenameTables(
361361
// or err is set (in which case err comes from EXCHANGE TABLES)
362362
if !originalTableExists || err != nil {
363363
if err := c.execWithLogging(ctx,
364-
fmt.Sprintf(dropTableSQLWithCHSetting, peerdb_clickhouse.QuoteIdentifier(newTable.Table), onCluster),
364+
fmt.Sprintf(dropTableSQLWithCHSetting, peerdb_clickhouse.QuoteIdentifier(newTable.LegacyDotted()), onCluster),
365365
); err != nil {
366366
return nil, fmt.Errorf("unable to drop table %s: %w", newTable, err)
367367
}
368368

369369
if err := c.execWithLogging(ctx, fmt.Sprintf("RENAME TABLE %s TO %s%s",
370-
peerdb_clickhouse.QuoteIdentifier(currentTable.Table),
371-
peerdb_clickhouse.QuoteIdentifier(newTable.Table), onCluster,
370+
peerdb_clickhouse.QuoteIdentifier(currentTable.LegacyDotted()),
371+
peerdb_clickhouse.QuoteIdentifier(newTable.LegacyDotted()), onCluster,
372372
)); err != nil {
373373
return nil, fmt.Errorf("unable to rename table %s to %s: %w", currentTable, newTable, err)
374374
}

flow/connectors/clickhouse/normalize.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,10 @@ func (c *ClickHouseConnector) SetupNormalizedTable(
5252
destinationTable common.QualifiedTable,
5353
sourceTableSchema *protos.TableSchema,
5454
) (bool, error) {
55-
tableAlreadyExists, err := c.checkIfTableExists(ctx, c.Config.Database, destinationTable.Table)
55+
// LegacyDotted: ClickHouse table names may contain dots; configs persisted before
56+
// the QualifiedTable refactor arrive first-dot-split, LegacyDotted reconstructs the
57+
// original single-part name (equal to .Table for new configs, namespace == "")
58+
tableAlreadyExists, err := c.checkIfTableExists(ctx, c.Config.Database, destinationTable.LegacyDotted())
5659
if err != nil {
5760
return false, fmt.Errorf("error occurred while checking if destination ClickHouse table exists: %w", err)
5861
}
@@ -89,7 +92,7 @@ func (c *ClickHouseConnector) generateCreateTableSQLForNormalizedTable(
8992
chVersion *chproto.Version,
9093
flags []string,
9194
) ([]string, error) {
92-
tableIdentifier := destinationTable.Table
95+
tableIdentifier := destinationTable.LegacyDotted()
9396
var engine string
9497
tmEngine := protos.TableEngine_CH_ENGINE_REPLACING_MERGE_TREE
9598

flow/connectors/clickhouse/normalize_query.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ func (t *NormalizeQueryGenerator) BuildQuery(ctx context.Context) (string, error
330330
}
331331

332332
insertIntoSelectQuery := fmt.Sprintf("INSERT INTO %s %s %s%s",
333-
peerdb_clickhouse.QuoteIdentifier(t.TableName.Table), colSelector.String(), selectQuery.String(), chSettings.String())
333+
peerdb_clickhouse.QuoteIdentifier(t.TableName.LegacyDotted()), colSelector.String(), selectQuery.String(), chSettings.String())
334334

335335
t.Query = insertIntoSelectQuery
336336

0 commit comments

Comments
 (0)