-
Notifications
You must be signed in to change notification settings - Fork 249
fix(postgres): prevent first-sync lsn mismatch under concurrent write load #1084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: staging
Are you sure you want to change the base?
Changes from all commits
08e917b
1005110
a831b09
2e70692
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,14 +130,35 @@ func NewReplicator(ctx context.Context, config *Config, slot ReplicationSlot, re | |
| } | ||
| } | ||
|
|
||
| // advanceLSN advances the logical replication position to the current WAL position. | ||
| // AdvanceLSN moves the slot to currentWalPos and retries until postgres confirms it there: | ||
| // pg_replication_slot_advance silently clamps its target to the flushed WAL position. | ||
| func AdvanceLSN(ctx context.Context, db *sqlx.DB, slot, currentWalPos string) error { | ||
| // Get replication slot position | ||
| if _, err := db.ExecContext(ctx, fmt.Sprintf(AdvanceLSNTemplate, slot, currentWalPos)); err != nil { | ||
| return fmt.Errorf("failed to advance replication slot: %w", err) | ||
| target, err := pglogrepl.ParseLSN(currentWalPos) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to parse advance target lsn[%s]: %s", currentWalPos, err) | ||
| } | ||
|
|
||
| timeoutCtx, cancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer cancel() | ||
| for { | ||
| var slotName string | ||
| var endLSN pglogrepl.LSN | ||
| if err := db.QueryRowContext(timeoutCtx, fmt.Sprintf(AdvanceLSNTemplate, slot, currentWalPos)).Scan(&slotName, &endLSN); err != nil { | ||
| return fmt.Errorf("failed to advance replication slot: %w", err) | ||
| } | ||
|
Comment on lines
+146
to
+148
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand why we're using timeoutCtx here, but I noticed that elsewhere in the codebase, QueryRowContext is used with the regular ctx. Do we need the timeout specifically here, or should we keep it consistent with the rest of the codebase? Is there any specific reason for using timeoutCtx here? |
||
| if endLSN >= target { | ||
| logger.Debugf("advanced LSN to %s", currentWalPos) | ||
|
Comment on lines
+149
to
+150
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we have >= check but when > check will be possible over here ? |
||
| return nil | ||
| } | ||
| // end_lsn short of the request means the advance was clamped to the flushed position; | ||
| // wait for the walwriter to catch up (bounded by ~3x wal_writer_delay) and re-advance | ||
| logger.Debugf("slot advance clamped at %s (target %s), retrying", endLSN, target) | ||
| select { | ||
| case <-timeoutCtx.Done(): | ||
| return fmt.Errorf("slot advance stopped at %s and could not reach target %s: %w", endLSN, target, timeoutCtx.Err()) | ||
| case <-time.After(100 * time.Millisecond): | ||
|
Comment on lines
+158
to
+159
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not use %w in our codebase for normal error,we use it for non-retryable error?If changing also look for other error messages as well. |
||
| } | ||
| } | ||
| logger.Debugf("advanced LSN to %s", currentWalPos) | ||
| return nil | ||
| } | ||
|
|
||
| // Confirm that Logs has been recorded | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove this slotName variable from here if it is redundant?