Skip to content

Refactor SQL quoting and replication stream handling - #114

Merged
isdaniel merged 1 commit into
mainfrom
chore/review
Sep 13, 2026
Merged

isdaniel merged 1 commit into
mainfrom
chore/review

Conversation

@isdaniel

Copy link
Copy Markdown
Owner
  • Introduced QuoteStyle::ReplicationLiteral to handle quoting for replication commands, ensuring that backslashes are treated as ordinary characters and single quotes are doubled.
  • Updated quote_replication_literal function to correctly quote string literals for replication commands, avoiding the escape-string form and ensuring safety against SQL injection.
  • Modified build_start_replication_sql and build_base_backup_sql to use quote_replication_literal for quoting options, ensuring correct SQL syntax for replication commands.
  • Enhanced documentation for quoting functions to clarify behavior differences between SQL and replication contexts.
  • Improved handling of LSN (Log Sequence Number) in LogicalReplicationStream, ensuring that reconnections use the correct applied position and that feedback is sent appropriately.
  • Updated BaseBackupOptions struct to ensure default values match server expectations, preventing silent failures in backup options.
  • Added tests to verify correct behavior of LSN parsing and timestamp conversion, ensuring robustness against edge cases.

@codspeed

codspeed Bot commented Sep 13, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 50 untouched benchmarks


Comparing chore/review (69282d2) with main (69d3ee6)

Open in CodSpeed

@codecov

codecov Bot commented Sep 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 95.62130% with 37 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.98%. Comparing base (69d3ee6) to head (69282d2).

Files with missing lines Patch % Lines
src/stream.rs 15.15% 28 Missing ⚠️
src/connection/native/startup.rs 0.00% 4 Missing ⚠️
src/connection/native/copy.rs 92.30% 2 Missing ⚠️
src/retry.rs 96.07% 2 Missing ⚠️
src/connection/native/conninfo.rs 99.76% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #114      +/-   ##
==========================================
+ Coverage   94.80%   94.98%   +0.18%     
==========================================
  Files          27       27              
  Lines       21285    21891     +606     
==========================================
+ Hits        20179    20794     +615     
+ Misses       1106     1097       -9     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Introduced `QuoteStyle::ReplicationLiteral` to handle quoting for replication commands, ensuring that backslashes are treated as ordinary characters and single quotes are doubled.
- Updated `quote_replication_literal` function to correctly quote string literals for replication commands, avoiding the escape-string form and ensuring safety against SQL injection.
- Modified `build_start_replication_sql` and `build_base_backup_sql` to use `quote_replication_literal` for quoting options, ensuring correct SQL syntax for replication commands.
- Enhanced documentation for quoting functions to clarify behavior differences between SQL and replication contexts.
- Improved handling of LSN (Log Sequence Number) in `LogicalReplicationStream`, ensuring that reconnections use the correct applied position and that feedback is sent appropriately.
- Updated `BaseBackupOptions` struct to ensure default values match server expectations, preventing silent failures in backup options.
- Added tests to verify correct behavior of LSN parsing and timestamp conversion, ensuring robustness against edge cases.

refactor(lsn): accept `impl Into<Lsn>` in the feedback setters

Every example ended its handler with the same wart:

    event_stream.update_applied_lsn(event.lsn.value());

`ChangeEvent.lsn` is an `Lsn`, but the setters took a raw `XLogRecPtr`,
so acking the position you were just handed required unwrapping it. All
six examples carried that `.value()`.

Widen `update_applied_lsn` / `update_flushed_lsn` (on both
`SharedLsnFeedback` and the `EventStream` wrappers) to `impl Into<Lsn>`
and normalise once inside. `From<u64> for Lsn` already exists, so every
existing `u64` call site still compiles — this is additive, not a break.
Monomorphised, so the hot path is unchanged.
@isdaniel
isdaniel merged commit 1929117 into main Sep 13, 2026
25 checks passed
@isdaniel
isdaniel deleted the chore/review branch September 14, 2026 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant