Extract ctid strategy - #979
Open
kvch wants to merge 5 commits into
Open
Conversation
kvch
force-pushed
the
pk-strategy-table-reader-refactoring
branch
from
July 20, 2026 13:53
eb1ddb3 to
4f453d8
Compare
- Collapse the progressTracking bool + progressBars map clump, which was duplicated across the generator and the reader, into a single progressTracker value with set/advance/complete methods. A zero-value tracker is disabled and no-ops, so callers no longer guard each call.
kvch
force-pushed
the
pk-strategy-table-reader-refactoring
branch
from
July 20, 2026 13:54
4f453d8 to
0cb8d34
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
CoverageTotal: 59.8% (±0.0% vs Coverage in packages changed by this PR:
|
readSession carried a snapshotID, which is a transaction snapshot detail of the ctid strategy, and the generator reached into it: for its log fields and to run its own getSnapshotSchemaTotalBytes. So the generator still knew how the ctid reader works, and a strategy that doesn't export a transaction snapshot would have had nothing to put in that field. A tableReader now only opens a session per schema, and the session does the reading, already bound to its schema and to the state its strategy needs. ctidReader produces a ctidSession that owns the snapshotID, which also drops the id from the signatures of readTable, getTableInfo and snapshotTableRange, and makes it impossible to pair a session with the wrong reader - the previous contract could only document that. Alongside: - Move getSnapshotSchemaTotalBytes onto ctidSession.totalBytes. The generator owns the progress bar, the strategy owns how the bytes behind it are measured. - Wrap the session as well as the reader in instrumentedTableReader, since the reader hands out the sessions that do the work. beginSchema now opens a span too, so table spans have a schema parent. - Extract rowSink: the row to wal event to processor to progress path is the same whichever rows a strategy decides to read, so it no longer sits inlined in snapshotTableRange next to the query and tx handling. - Build the reader in newTableReader, so choosing a strategy and applying the decorators happens in one place instead of in NewSnapshotGenerator.
kvch
marked this pull request as ready for review
August 12, 2026 15:10
tier |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactors
pkg/snapshot/generator/postgres/dataso thetableReaderstrategy is a factory of read sessions instead of a single object the generator reaches into.Now a
tableReaderonly opens a session per schema, and the session (already bound to its schema and strategy-specific state) does the reading.ctidReaderproduces actidSessionthat owns thesnapshotID, removing it from the signatures ofreadTable,getTableInfo, andsnapshotTableRange, making it impossible to pair a session with the wrong reader, instead of relying on documentation.Related Issue(s)
Type of Change
Changes Made
tableReader/readSessioninterfaces (table_reader.go) and extract the ctid strategy intoctidSession/ctidReader(ctid_table_reader.go), which owns thesnapshotIDandtotalBytesmeasurementgetSnapshotSchemaTotalBytesontoctidSession.totalBytes— the generator owns the progress bar, the strategy owns how the bytes behind it are measuredinstrumentedTableReader;beginSchemanow opens a span too, so table spans get a schema parentrowSink(row_sink.go): the row → WAL event → processor → progress path is shared regardless of which rows a strategy reads, so it no longer sits inlined insnapshotTableRangesnapshotTx(snapshot_tx.go) andprogressTracker(progress_tracker.go) to collapse the duplicatedprogressTrackingbool +progressBarsmap into a single value withset/advance/completemethods; a zero-valuetracker no-ops, removing per-call guards
newTableReader, so choosing a strategy and applying decorators happens in one place instead of inNewSnapshotGeneratorTesting
Checklist