Skip to content

Restore indexes in parallel during pg2pg schema snapshot - #1144

Open
kvch wants to merge 2 commits into
mainfrom
parallel-index-creation
Open

Restore indexes in parallel during pg2pg schema snapshot#1144
kvch wants to merge 2 commits into
mainfrom
parallel-index-creation

Conversation

@kvch

@kvch kvch commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Description

Index restore during a pg-to-pg schema snapshot runs as a single sequential psql session, so a schema with many indexes builds them one at a time even though CREATE INDEX statements have no dependencies on each other.

This adds an opt-in index_restore_workers setting that restores the standalone CREATE INDEX/CREATE UNIQUE INDEX statements concurrently. Everything that can depend on an index existing (constraints added USING INDEX, comments, partition attachments, REPLICA IDENTITY, CLUSTER) is still restored afterwards, once every index has been created, preserving the original relative order.

It defaults to 1, which is byte-for-byte the previous sequential behaviour, so nothing changes for existing users until they opt in.

Improvement:

Measured locally.

Scenario Sequential 4 workers 8 workers
300 indexes on a small table 4.8s 3.8s 1.7s
8 indexes on a 3M-row table 34.4s 11.0s

Type of Change

Please select the relevant option(s):

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • 📚 Documentation update
  • 🔧 Refactoring (no functional changes)
  • ⚡ Performance improvement
  • 🧪 Test coverage improvement
  • 🔨 Build/CI changes
  • 🧹 Code cleanup

Changes Made

  • Add IndexRestoreWorkers to the pgdump/pgrestore snapshot config, wired through YAML (index_restore_workers) and env (PGSTREAM_POSTGRES_SNAPSHOT_INDEX_RESTORE_WORKERS)
  • Split the indices-and-constraints dump into its statement blocks and restore the CREATE INDEX ones through a bounded errgroup, then restore the rest sequentially.
  • A failing index deliberately does not cancel the restores still in flight
  • Merge the per-index failures into a single PGRestoreErrors, so the retry decision is taken over the whole wave rather than over whichever error arrived first
  • Key the index progress bars by backend pid instead of table name.

Testing

  • Unit tests added/updated
  • Integration tests added/updated
  • Manual testing performed
  • All existing tests pass

Checklist

  • Code follows project style guidelines
  • Self-review completed
  • Code is well-commented
  • Documentation updated where necessary

Additional Notes

@kvch kvch added the ai-review Set for simple PRs that does not require human approval label Sep 3, 2026
@xata-bot

xata-bot commented Sep 3, 2026

Copy link
Copy Markdown

⚠️ The pgstream review agent is deferring to a human reviewer.

The change adds non-trivial concurrency to the pg2pg schema-snapshot index restore path (flagged snapshot-core), including new dump-block splitting/reassembly heuristics, merged error classification for concurrent restores, and a progress-tracker keying change; the code is well-reasoned, extensively tested (unit + integration), defaults to the exact prior sequential behaviour, and I found no concrete bug, but the correctness of the string-based \n\n-block splitting against the full space of real pg_dump output shapes (exclusion constraints, WHERE-clause partial indexes, concurrent index builds on partitioned children, etc.) is not something I can fully verify from the code and tests alone, so a human familiar with pg_dump/pg_restore output quirks should confirm before merging.

  • The dependency-block partitioning (partitionDumpBlocks) relies on pg_dump always separating statements with a blank line and on isIndexStatement/isAttachPartitionIndexStatement correctly classifying every index-dependent DDL shape (constraints, comments, CLUSTER, REPLICA IDENTITY, partition attach); this is exercised by unit and integration tests but the coverage of exotic dump shapes (e.g. exclusion constraints, concurrently-built partitioned indexes, indexes with WHERE clauses spanning multiple lines) is limited, and a misclassification would silently reorder or split restore statements incorrectly.
  • The pre-existing single-line 'ALTER TABLE ... ADD CONSTRAINT' branch in parseDump writes no trailing newline, so consecutive such statements can merge into one '\n\n'-delimited block; this predates the PR but is now relied upon more heavily by the new parallel-restore block splitting, and its interaction was not explicitly tested.
  • scrutiny areas touched: snapshot-core

tier T1-agent. Address the above (or get a human review) and re-add the ai-review label to re-run.

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown

Coverage

Total: 61.3% (+0.1% vs main)

Coverage in packages changed by this PR:

Package Coverage Δ
cmd/config 85.1% ±0.0%
internal/postgres 49.3% -0.8%
pkg/snapshot/generator/postgres/schema/pgdumprestore 90.2% +0.6%

@kvch
kvch force-pushed the parallel-index-creation branch from 8d3ac48 to 53d5822 Compare September 3, 2026 18:04
@kvch kvch added ai-review Set for simple PRs that does not require human approval and removed ai-review Set for simple PRs that does not require human approval labels Sep 3, 2026
@kvch
kvch force-pushed the parallel-index-creation branch from c64763b to 8354fcb Compare September 3, 2026 18:12
@kvch
kvch force-pushed the parallel-index-creation branch from 8354fcb to 4657a70 Compare September 3, 2026 18:19
@kvch kvch added the ai-review Set for simple PRs that does not require human approval label Sep 3, 2026

@xata-bot xata-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved by the pgstream review agent.

The change is well-scoped, defaults preserve the exact prior sequential behaviour (workers<=1 short-circuits to the original code path, and WithRestoreToWAL forces workers back to 1), the worker cap is validated at startup before any connections are opened, error classification across concurrent index restores is merged correctly via MergePGRestoreErrors (verified against addError/IsRetryable/HasCriticalErrors semantics), and the block-splitting/ordering logic (partitionDumpBlocks, joinDumpBlocks) mirrors the pre-existing splitConflictTargetConstraints pattern with added defensive statement separators, backed by extensive unit/integration tests including a real pg_dump fixture covering exotic index shapes.

tier T1-agent · 8 files / 280 substantive lines · risk medium. This is an automated review and does not replace human judgement on anything it escalates.

@kvch
kvch requested a review from tsg September 3, 2026 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Set for simple PRs that does not require human approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants