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: restore to neondb (not postgres) database #10251

Merged
merged 7 commits into from
Jan 15, 2025

Conversation

NanoBjorn
Copy link
Contributor

@NanoBjorn NanoBjorn commented Dec 31, 2024

Problem

postgres is system database at neon, so we need to do pg_restore into neondb instead

https://github.com/neondatabase/cloud/issues/22100

Summary of changes

Changed fast_import a little bit:

  1. After succesfull connection creating neondb in postgres instance
  2. Changed restore connstring to use new db
  3. Added optional source_connection_string, which allows to skip s3_prefix and just connect directly.
  4. Added -i that stops process until sigterm

TODO

@NanoBjorn NanoBjorn requested a review from a team as a code owner December 31, 2024 16:43
@NanoBjorn NanoBjorn requested review from MMeent and problame and removed request for MMeent December 31, 2024 16:43
Copy link

github-actions bot commented Dec 31, 2024

7315 tests run: 6939 passed, 0 failed, 376 skipped (full report)


Flaky tests (1)

Postgres 17

  • test_physical_replication_config_mismatch_max_locks_per_transaction: release-arm64

Code coverage* (full report)

  • functions: 33.7% (8420 of 24980 functions)
  • lines: 49.2% (70397 of 143103 lines)

* collected from Rust tests only


The comment gets automatically updated with the latest test results
ebc4735 at 2025-01-15T20:28:25.536Z :recycle:

Copy link
Contributor

@problame problame left a comment

Choose a reason for hiding this comment

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

In addition to the feedback I gave on Slack, I think instead of building a complicated CLI, let's allow specifying the spec as a filesystem path in addition to an S3 location.

It'd be a good Rust learning exercise to do that refactoring, but, probably better to wait for someone from Storage to pick this up / pair with you.

The fast_import.rs code isn't up to our usual standards, remember, it's PoC code.

@NanoBjorn NanoBjorn changed the title Made fast_import more testable and changed dbname to neondb fast import: restore to neondb (not postgres) database Jan 7, 2025
@NanoBjorn
Copy link
Contributor Author

I think instead of building a complicated CLI, let's allow specifying the spec as a filesystem path in addition to an S3 location.

Though about it as well, but not sure I like the idea of putting some testing args into the spec. Optional args have the same semantics as top-level json spec values, so I would leave them as is for now (until I really need any of those in the spec)

@NanoBjorn NanoBjorn force-pushed the 22100-change-fastimport-db-name branch from 73c14a7 to ebe26e2 Compare January 14, 2025 11:45
@VladLazar VladLazar self-requested a review January 14, 2025 12:45
@NanoBjorn NanoBjorn removed request for jcsp and arpad-m January 14, 2025 13:09
compute_tools/src/bin/fast_import.rs Show resolved Hide resolved
compute_tools/src/bin/fast_import.rs Outdated Show resolved Hide resolved
compute_tools/src/bin/fast_import.rs Show resolved Hide resolved
compute_tools/src/bin/fast_import.rs Outdated Show resolved Hide resolved
compute_tools/src/bin/fast_import.rs Outdated Show resolved Hide resolved
compute_tools/src/bin/fast_import.rs Show resolved Hide resolved
Copy link
Contributor

@hlinnaka hlinnaka left a comment

Choose a reason for hiding this comment

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

Hmm, do we want to always restore to 'neondb'? I'd expect the restore to preserve the original database name, i.e. if the source database is called "myapp_staging", I'd expect it to be imported as "myapp_staging" too.

compute_tools/src/bin/fast_import.rs Show resolved Hide resolved
@NanoBjorn NanoBjorn requested a review from VladLazar January 15, 2025 17:28
compute_tools/src/bin/fast_import.rs Outdated Show resolved Hide resolved
@NanoBjorn NanoBjorn force-pushed the 22100-change-fastimport-db-name branch from 137c377 to ebc4735 Compare January 15, 2025 18:56
@NanoBjorn NanoBjorn enabled auto-merge January 15, 2025 20:09
@NanoBjorn NanoBjorn added this pull request to the merge queue Jan 15, 2025
Merged via the queue into main with commit 55a68b2 Jan 15, 2025
88 of 89 checks passed
@NanoBjorn NanoBjorn deleted the 22100-change-fastimport-db-name branch January 15, 2025 20:52
github-merge-queue bot pushed a commit that referenced this pull request Jan 21, 2025
We did not have any tests on fast_import binary yet.

In this PR I have introduced:
- `FastImport` class and tools for testing in python
- basic test that runs fast import against vanilla postgres and checks
that data is there

Should be merged after #10251
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.

4 participants