comparing RelationMessage against TableSchema, stop caching RelationMessages #1438
Conversation
Co-authored-by: Philip Dubé <[email protected]>
|
Since we now maintain the replication connection across syncs, we don't get a second Also added defensive check to only update |
| @@ -99,7 +87,6 @@ message CreateTablesFromExistingOutput { | |||
|
|
|||
| message SyncFlowOptions { | |||
| uint32 batch_size = 1; | |||
| map<uint32, RelationMessage> relation_message_mapping = 2; | |||
There was a problem hiding this comment.
if we only deprecate this field then we won't break pause/unpause upgrade
There was a problem hiding this comment.
will need to retain the other Message types for this, also not sure if it's enough since CDC state also loses fields
There was a problem hiding this comment.
That's fine, mark it all deprecated & leave it unused, then we make an explicit breaking change PR afterwards
There was a problem hiding this comment.
please deprecate and not delete
|
lgtm, good to see a red PR, would want @iskakaushik to take a look over it too |
Previously we diffed the previous and current
RelationMessagesreceived from Postgres during logical replication to ascertain if the schema changed or not. This fails in cases where the schema changed after mirror setup but before we get the firstRelationMessagewhich can happen a) during initial load, before CDC and b) during CDC, if no rows are synced before schema changes are made.This causes a desync between PeerDB's view of the table and the actual status of the table, causing issues. Changed to diff from the current
TableSchemainstead, which should always be present. Eliminates having to store and passRelationMessageMappingaround, it is scoped to reading from the replication connection.Also fixes bugs with schema changes being handled incorrectly for PG and BQ, and schema audits being logged without schema deltas actually being present.