Skip to content
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

fast import: allow restore to provided connection string #10407

Open
wants to merge 31 commits into
base: main
Choose a base branch
from

Conversation

NanoBjorn
Copy link
Contributor

@NanoBjorn NanoBjorn commented Jan 15, 2025

Within https://github.com/neondatabase/cloud/issues/22089 we decided that would be nice to start with import that runs dump-restore into a running compute (more on this here) We could do it by writing another tool or by extending existing fast_import.rs, we chose the latter.

In this PR, I have added optional restore_connection_string as a cli arg and as a part of the json spec. If specified, the script will not run postgres and will just perform restore into provided connection string.

TODO:

  • fast_import.rs:
  • tests:
    • restore with cli arg in the fast_import.rs
    • restore with encoded connstring in json spec in s3
    • test with custom dbname will do in a separate PR
    • test with s3 + pageserver + fast import binary will do in a separate PR
    • fast import: basic python test #10271 (comment) will do in a separate PR

neondatabase/cloud#22775

@NanoBjorn NanoBjorn force-pushed the cloud-22775-restore-to-connstring branch from 70d43af to 4a8ff6d Compare January 15, 2025 14:08
Copy link

github-actions bot commented Jan 15, 2025

7359 tests run: 6981 passed, 1 failed, 377 skipped (full report)


Failures on Postgres 17

# Run all failed tests locally:
scripts/pytest -vv -n $(nproc) -k "test_scrubber_tenant_snapshot[debug-pg17-4]"

Test coverage report is not available

The comment gets automatically updated with the latest test results
b79e5f5 at 2025-01-21T17:33:24.690Z :recycle:

@NanoBjorn NanoBjorn force-pushed the cloud-22775-restore-to-connstring branch from 4a8ff6d to 0e40544 Compare January 16, 2025 19:08
@NanoBjorn NanoBjorn force-pushed the cloud-22775-restore-to-connstring branch 2 times, most recently from 32ed28a to 01604b3 Compare January 20, 2025 19:21
@duskpoet
Copy link
Member

why not put the restore connection string into the spec? Shouldn't it be encoded the same way as the source string?

@NanoBjorn NanoBjorn force-pushed the cloud-22775-restore-to-connstring branch from 01604b3 to 6cd4b9c Compare January 21, 2025 12:53
@VladLazar VladLazar self-requested a review January 21, 2025 13:25
@NanoBjorn NanoBjorn marked this pull request as ready for review January 21, 2025 13:25
@NanoBjorn NanoBjorn requested a review from a team as a code owner January 21, 2025 13:26
@NanoBjorn NanoBjorn requested review from knizhnik and lubennikovaav and removed request for a team January 21, 2025 13:26
@NanoBjorn
Copy link
Contributor Author

NanoBjorn commented Jan 21, 2025

why not put the restore connection string into the spec? Shouldn't it be encoded the same way as the source string?

I have done it both ways as with source connection strings, there are tests on both paths as well

Base automatically changed from 22037-basic-fast-import-e2e to main January 21, 2025 16:57
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.

2 participants