oracledb_cdc: remove primary key ordering from unfiltered snapshot - #4696
Conversation
3abe4c3 to
30c28b0
Compare
30c28b0 to
72a3870
Compare
72a3870 to
36aee7d
Compare
| Default(1)). | ||
| Field(service.NewIntField(ociFieldSnapshotMaxBatchSize). | ||
| Description("The maximum number of rows to be streamed in a single batch when taking a snapshot."). | ||
| Description("The maximum number of rows fetched per query when taking a snapshot of a table with a `" + ociFieldSnapshotFilters + "` entry configured. Tables without one are streamed through a single unordered cursor, where this value only paces how often a cancellation is checked."). |
There was a problem hiding this comment.
snapshot_max_batch_size now carries two unrelated meanings depending on whether the table happens to have a snapshot_filters entry: for filtered tables it is the page size of the keyset-pagination query, and for every other table (the default, since snapshot_filters is optional) it degrades to "how often ctx.Err() is polled" — see snapshot.go#L362-L381.
This is a material UX regression under CONTRIBUTING §1.1.3 ("UX should be intuitive and require minimal explanation") and §3.1.5 (consistency with the rest of the fleet, where snapshot_max_batch_size uniformly means rows-per-fetch). The description text itself is the evidence — it has to explain two divergent behaviours for one knob.
Concretely: a user who raised snapshot_max_batch_size to tune snapshot throughput — exactly what docs/benchmark-results/oracledb-cdc.md documents with snapshot_max_batch_size: 160000 — now gets no throughput effect at all on the default path, silently.
Suggested fix: keep the knob meaning "rows fetched per round trip" on both paths by wiring it into the driver's prefetch/array-fetch size for the full-scan cursor, and use a separate fixed internal constant for the cancellation-poll interval. If the two really must differ, a dedicated field for the scan path would be clearer than overloading this one.
There was a problem hiding this comment.
I'm not sure we want snapshot_max_batch_size to be the prefetch_rows value, it would be better to have that as a dedicated config and deprecate snapshot_max_batch_size if needed.
| // snapshotTableFullScan performs a single, full unordered scan so Oracle's optimizer | ||
| // picks a full table scan (sequential multiblock reads) over random disk I/O when ordered. | ||
| func (s *Snapshot) snapshotTableFullScan(ctx context.Context, tx *sql.Tx, table UserTable, maxBatchSize int, tableName string) (numRowsProcessed int, err error) { | ||
| q := fmt.Sprintf(`SELECT * FROM "%s"."%s"`, table.Schema, table.Name) |
There was a problem hiding this comment.
This change is justified purely on throughput grounds — the doc comment here and the inline comment at snapshot.go#L232-L234 both claim the unordered scan is faster than the previous ORDER BY keyset pagination — but no benchmark evidence ships with it.
CONTRIBUTING §1.3.4/§1.3.5 requires both local and real-endpoint benchmarking at various throughput levels, with results recorded under docs/benchmark-results/. docs/benchmark-results/oracledb-cdc.md already has a "Snapshot Results" section, and it is left untouched by this PR — so the recorded numbers now describe a code path that no longer runs for unfiltered tables, and were captured under a snapshot_max_batch_size: 160000 setting whose meaning this PR changes.
Suggested fix: re-run the snapshot benchmarks against the new scan path and update docs/benchmark-results/oracledb-cdc.md, so the performance claim in this comment is backed by recorded numbers rather than an assertion about Oracle's optimizer.
There was a problem hiding this comment.
Benchmarks included in proof of work. Will look to update these as a separate PR.
Problem
By default, OracleDB uses heaped tables which aren't sorted by primary key, meaning snapshotting ordered by primary key can incur high disk I/O as every single row lookup via ROWID forces Oracle to jump to a different physical data block on disk.
Solution
As this is a full table scan, and not batched/chunked into ranges we can remove ordering which improves overall read performance.
Proof of Work
AWS Benchmark
Benchmarks went from ~9.8MB/s (post cache-warm) to a consistent 98MB/s:
Local Benchmark:
Before: Avg. ~40MB/s
After: Avg. ~51MB/s