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

feat(postgres): use faster sql.DB instead of docker exec psql for snapshot/restore #2599

Closed
wants to merge 6 commits into from

Conversation

cfstras
Copy link
Contributor

@cfstras cfstras commented Jun 20, 2024

What does this PR do?

The postgres module has functions for saving snapshots and restoring them later.
This feature is helpful for integration testing without having to manually clear the database.

This PR speeds up the snapshot/restore functionality by executing the necessary SQL commands via a regular sql.DB connection.

If the driver is not available in the test environment, or the commands fail for some other reason (SSL config, etc), it falls back to the old variant of calling docker exec.

Why is it important?

docker exec can be unnecessarily slow, especially when using docker in a VM with Docker Desktop or colima.

On my laptop (MacBook Air M1), each call to Container.Restore() takes about 150~200ms.
In my suite of 20 tests, where each test restores the database before starting, with creating the initial container, this brings the total time up to 12 seconds, which is quite a lot for a few small database tests.

Related issues

How to test this PR

cd modules/postgres
go test ./...

Follow-ups

I've also considered caching the DB pool that is created to manage the databases.
After running some benchmark with this code (from branch X), I noticed it does not have a big impact on the whole test suite - my internal test suite does not change in execution time (since the time is dominated by re-connecting the actual test client, which will be terminated when the database gets deleted during the restore).
The module/postgres test suite even gets slower:

cfstras@9ad456d:

# At 9ad456dc22ff6f2c8980f61887fb603e82f9f5bd (caching *sql.DB)
➜ cd module/postgres
➜ hyperfine --warmup 1 "go test ./... -count 1"
Benchmark 1: go test ./... -count 1
  Time (mean ± σ):     40.816 s ±  1.697 s    [User: 1.231 s, System: 0.974 s]
  Range (min … max):   38.398 s … 42.658 s    10 runs
# At e6babe45d6b91c433ce88ae57c6b62bfe46be794 (no caching of *sql.DB)
➜ cd module/postgres
➜ hyperfine --warmup 1 "go test ./... -count 1"
Benchmark 1: go test ./... -count 1
  Time (mean ± σ):     36.602 s ±  1.241 s    [User: 1.166 s, System: 0.953 s]
  Range (min … max):   35.877 s … 39.071 s    10 runs

@cfstras cfstras requested a review from a team as a code owner June 20, 2024 16:20
@cfstras
Copy link
Contributor Author

cfstras commented Jun 20, 2024

@mdelapenya
Note that I initially wrote this for main, but saw you're working on v1, so I rebased. I cannot help but notice that container setup seems a lot slower on this branch - my benchmark wins are almost completely eaten up by this slowdown... Haven't looked further into it yet.

@mdelapenya
Copy link
Member

Thanks for submitting this PR, I'll take a look asap.

I'd prefer targeting everything to main, as I'm doing the merges/cherries/rebases to avoid the push the burden on the community. What I'm really interested is in how you run those benchmarks. Could you share a bit about it?

@cfstras
Copy link
Contributor Author

cfstras commented Jun 21, 2024

Hi @mdelapenya, thanks for your response :)

I've re-based onto main, and ran some of my benchmarks again.
#2600
Results are posted on the PR.

The benchmarks are run with hyperfine, which is a great tool indeed :)

@mdelapenya
Copy link
Member

If you mind, let's close this one in favor of #2600, thanks!

@cfstras cfstras closed this Jun 21, 2024
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