Fix Postgres CDC issues (table mappings, error handling, state, CDC scaling, etc) #415
Conversation
This PR fixes multiple issues with the Postgres CDC provider including * Allow editable table mappings in a single API call * Convert table mappings to a set to avoid order issues in the plan * Add better error handling for table mappings * Fix source validation to disallow source type changes * Add warning for deleting pipes to warn that the user needs to manually delete their destination tables * Allow snapshot state as an acceptable state so both snapshot and CDC replication modes will provision faster and successfully on large loads
tpanetti
changed the title
ilidemi
left a comment
ilidemi
left a comment
There was a problem hiding this comment.
State looks good
Scaling looks good, with minor comments (feel free to check in after applying)
Deletion - @whites11 would defer to you on what is the best experience for "this plan assumes you're ok with deleting your data, you shouldn't really do this". Regular deletion doesn't need a warning IMO, if that is at all separate from replace
Table mappings - this will need careful iteration, please split out in a different PR which we'll take after I'm back
| plan.ReplicaCpuMillicores = types.Int64Value(scaling.ReplicaCpuMillicores) | ||
| plan.ReplicaMemoryGb = types.Float64Value(scaling.ReplicaMemoryGb) |
There was a problem hiding this comment.
After UpdateCDCScaling returns, the values are "accepted", not "applied" - to properly update the plan we should wait till GetCDCScaling starts returning them (average 2-5 min)
| plan.ReplicaCpuMillicores = types.Int64Value(scaling.ReplicaCpuMillicores) | ||
| plan.ReplicaMemoryGb = types.Float64Value(scaling.ReplicaMemoryGb) |
There was a problem hiding this comment.
Same here, need to wait till they're applied
| var stateTableMappings []models.ClickPipePostgresTableMappingModel | ||
| // Table mappings - convert API response to Set (order doesn't matter) | ||
| // Get state mappings for preserving null values on optional fields | ||
| var stateTableMappingsMap map[string]models.ClickPipePostgresTableMappingModel |
There was a problem hiding this comment.
The key here should be struct{sourceSchemaName, sourceTable string} (simple structs can be keys in Go) - each source table can only be present once, and I'm not sure if it's completely impossible to have dots in names
| if diags := state.Source.As(ctx, &sourceModel, basetypes.ObjectAsOptions{}); !diags.HasError() { | ||
| if !sourceModel.Postgres.IsNull() { | ||
| response.Diagnostics.AddWarning( | ||
| "Manual table cleanup required", |
There was a problem hiding this comment.
Curious, is this the only way to issue a warning during recreation? If yes all good, if no, during normal pipe deletion the users probably don't expect to delete data on the destination
| } | ||
|
|
||
| // Validation 1: Cannot delete the last table mapping | ||
| if len(stateMappings) == 1 && len(planMappings) == 0 { |
There was a problem hiding this comment.
Are stateMappings guaranteed to go through 1 before attempting 0? Going from 2 to 0 would also be a mistake
| if len(stateMappings) > 0 && len(planMappings) > 0 { | ||
| // Build sets of source tables (schema.table as key) | ||
| stateTableKeys := make(map[string]bool) | ||
| planTableKeys := make(map[string]bool) |
There was a problem hiding this comment.
Struct keys here too please (having just source is already great), just for the sake of possible dots
| if addedTables[removed] { | ||
| response.Diagnostics.AddError( | ||
| "Invalid table_mappings configuration", | ||
| fmt.Sprintf("Cannot delete and add a table mapping for the same source table '%s'. To modify a table mapping, update it in place rather than deleting and recreating it.", removed), |
There was a problem hiding this comment.
This one is important - updating a mapping is not allowed at all. So either need to check other fields on the non-added non-removed manually or find a way to make all fields the keys of the map, then they'd show up as added+removed. Although not fully clear how to do this with sortingKeys that are a slice
This PR fixes multiple issues with the Postgres CDC provider including