Skip to content

db: drop duplicate snapshots sandbox_id index - #3405

Merged
ValentaTomas merged 1 commit into
mainfrom
drop-dup-snapshots-sandbox-id-index
Jul 27, 2026
Merged

ValentaTomas merged 1 commit into
mainfrom
drop-dup-snapshots-sandbox-id-index

Conversation

@ValentaTomas

@ValentaTomas ValentaTomas commented Jul 27, 2026

Copy link
Copy Markdown
Member

idx_snapshots_sandbox_id is an exact duplicate of the constraint-backed snapshots_sandbox_id_unique (same key column). The unique twin serves every lookup; the plain copy shows no scans over a multi-month usage window while every snapshot write maintains both. Dropped CONCURRENTLY; uniqueness enforcement is untouched. Fresh, isolated re-roll of the earlier closed draft (#3390).

Schema proof, both definitions in this repo: the dropped index (20250708135400 L3) and the surviving unique constraint (20251009170758 L4-L5) — identical key (sandbox_id). Background on why identical indexes are redundant: PostgreSQL wiki — duplicate indexes.

@cla-bot cla-bot Bot added the cla-signed label Jul 27, 2026
@cursor

cursor Bot commented Jul 27, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Schema cleanup only; uniqueness and lookups stay on the constraint-backed index, with standard concurrent-drop operational caveats.

Overview
Removes the redundant non-unique index on snapshots.sandbox_id because snapshots_sandbox_id_unique already indexes that column for lookups and uniqueness. The drop runs CONCURRENTLY with bounded statement_timeout on up and a careful down path that clears invalid indexes before rebuilding.

Reviewed by Cursor Bugbot for commit 55899f2. Bugbot is set up for automated code reviews on this repo. Configure here.

@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

❌ 5 Tests Failed:

Tests completed Failed Passed Skipped
3512 5 3507 9
View the top 3 failed test(s) by shortest run time
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_0_lists_only_root_directory
Stack Traces | 0.01s run time
=== RUN   TestListDir/depth_0_lists_only_root_directory
=== PAUSE TestListDir/depth_0_lists_only_root_directory
=== CONT  TestListDir/depth_0_lists_only_root_directory
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_0_lists_only_root_directory
--- FAIL: TestListDir/depth_0_lists_only_root_directory (0.01s)
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
Stack Traces | 0.01s run time
=== RUN   TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
=== PAUSE TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
=== CONT  TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory)
--- FAIL: TestListDir/depth_2_lists_first_level_of_subdirectories_(in_this_case_the_root_directory) (0.01s)
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_1_lists_root_directory
Stack Traces | 0.02s run time
=== RUN   TestListDir/depth_1_lists_root_directory
=== PAUSE TestListDir/depth_1_lists_root_directory
=== CONT  TestListDir/depth_1_lists_root_directory
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_1_lists_root_directory
--- FAIL: TestListDir/depth_1_lists_root_directory (0.02s)
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir
Stack Traces | 0.48s run time
=== RUN   TestListDir
=== PAUSE TestListDir
=== CONT  TestListDir
--- FAIL: TestListDir (0.48s)
View the full list of 1 ❄️ flaky test(s)
github.com/e2b-dev/infra/tests/integration/internal/tests/envd::TestListDir/depth_3_lists_all_directories_and_files

Flake rate in main: 30.77% (Passed 9 times, Failed 4 times)

Stack Traces | 0.01s run time
=== RUN   TestListDir/depth_3_lists_all_directories_and_files
=== PAUSE TestListDir/depth_3_lists_all_directories_and_files
=== CONT  TestListDir/depth_3_lists_all_directories_and_files
    filesystem_test.go:96: 
        	Error Trace:	.../tests/envd/filesystem_test.go:96
        	Error:      	Received unexpected error:
        	            	unavailable: 502 Bad Gateway
        	Test:       	TestListDir/depth_3_lists_all_directories_and_files
--- FAIL: TestListDir/depth_3_lists_all_directories_and_files (0.01s)

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

@ValentaTomas
ValentaTomas marked this pull request as ready for review July 27, 2026 04:12

@claude claude 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.

LGTM — straightforward, low-risk migration.

Verified the redundant-index premise directly: idx_snapshots_sandbox_id was created in 20250708135400_snapshots_migrations.sql, and the unique constraint snapshots_sandbox_id_unique (added later in 20251009170758_unique_snapshots.sql) on the same column already provides Postgres's implicit backing index, making the plain index redundant. Checked the migration mechanics — correct timestamp, NO TRANSACTION for the CONCURRENTLY drop/create, statement_timeout bounds and the migrator's 3h baseline restore, and a down path that clears a stranded INVALID index before rebuilding — all consistent with the pattern used in the recent 20260725100500_env_builds_status_group_statistics.sql migration.

Extended reasoning...

Overview

Single new migration file dropping a redundant non-unique index on snapshots.sandbox_id, run CONCURRENTLY with a bounded statement_timeout. Down migration clears any leftover INVALID index and rebuilds it, also CONCURRENTLY.

Security risks

None. This is schema-only cleanup; no auth, crypto, or data-exposure surface is touched. Uniqueness enforcement remains on the constraint-backed index (snapshots_sandbox_id_unique), so there is no correctness regression for duplicate detection.

Level of scrutiny

Low. This is a mechanical DB migration following an established pattern in this repo (see 20260725100500_env_builds_status_group_statistics.sql for the same statement_timeout/NO TRANSACTION structure). I independently verified the redundancy claim against the migration history rather than taking the PR description at face value, and confirmed the down-migration correctly guards against the classic "stranded INVALID index" pitfall of interrupted CONCURRENTLY builds.

Other factors

No CODEOWNERS-sensitive paths, no outstanding reviewer comments to address, and Codecov reports full coverage on the change (trivially, since it's a migration file). This is a fresh re-roll of a previously closed draft PR (#3390) per the description, with no changes needed based on the current diff.

@ValentaTomas
ValentaTomas merged commit 85a0199 into main Jul 27, 2026
43 checks passed
@ValentaTomas
ValentaTomas deleted the drop-dup-snapshots-sandbox-id-index branch July 27, 2026 07:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants