chore(datastore): count every table that references an attachment

Hayley Swimelarrequested to merge
313/step-10-attachment-refcheck-widening into main

Why

BlobStorageAttachmentStore.DeleteIfUnreferenced counted container_manifests alone, the only table that referenced blob_storage_attachments when it was written. Ten tables reference it now, and none of those foreign keys carries an ON DELETE action, so calling the check on an attachment a Maven or npm row still holds does not free it: the DELETE reaches the database, violates that table's foreign key, and aborts the caller's transaction. Counting every referencing table makes the call a no-op reporting 0 rows instead, which is what the delete composers in later steps of this plan need.

Step 10 of the S17 Phase 4 plan. The branch also absorbed two composer steps that consume the widening, so the diff spans Steps 10, 11, and 15: chore(datastore): compose the id-keyed npm pack... (!1459 - merged) • Hayley Swimelar • 19.3 and chore(datastore): remove a Maven file and its a... (!1463 - merged) • Hayley Swimelar • 19.3 merged into it.

What

  • Ten tables, where the plan's Files entry and the spec's Blob attachments section both name eight. Two arrived after the plan's research, the way maven_remote_files landed after the spec's original seven-table inventory that this plan's own spec patch corrected to eight. container_remote_manifests was already on main when this branch opened (753dadd9). container_remote_blobs landed while this MR sat in review, and this MR's own pin is what caught it: the rebase turned it into a failing test naming the missing table, rather than a clause nobody noticed was absent. Both hold live remote-cache references, so the spec's reason for counting the other remote-cache tables covers them. The pin reads the table list out of internal/datastore/migrations/structure.sql instead of restating it, which is why an eleventh table will fail a test too.
  • No soft_deleted_at filter, and a test pins its absence. A marked, unreaped row still counts as a reference, which the spec calls the one place that predicate must not go.
  • The cost is planning, not execution. Warm planning moves from 0.14 ms at one clause to 1.20 ms at ten, the delete node's own execution stays near 0.12 ms, and every clause prunes to a single partition through the (namespace_id, blob_storage_attachment_id) index. Nothing amortizes the planning, because the pool runs the simple protocol and docs/dev/database.md rules out server-side prepared statements, so a per-row caller pays it per row. The bulk-delete steps later in this plan should batch rather than call this once per row.
  • One follow-up, deliberately not fixed here. The widening turns a visible 23503 abort into a 0 that every current caller discards, so a future writer sharing an attachment id across tables would strand a row with no signal. removed == 1 && n == 0 disambiguates that for free at the caller, and a bare zero-count alarm would misfire on ordinary lost races. That caller change is tracked, with the outcome shape review proposed for it, in Attachment reference-check follow-ups: outcome ... (#607) • Unassigned. The guard comments this widening falsified are corrected here, in the cascade and in this store's own suite, and the one remaining stale test premise sits on that issue.

Of Step 10's own diff, 1345 of the 1524 changed lines are tests. The ten-table fixture matrix is one test idea, and splitting it by format would leave the inventory test asserting against a deliberately partial guard.

Context for LLM agents

Rationale

  • One NOT EXISTS over a UNION ALL of the ten tables. Rejected on measurement: the correlated subquery blocks plan-time partition pruning, and the variant planned orders of magnitude slower than the flat clauses.
  • Building the SQL from a []string of table names. Rejected because the statement is read far more often than it is edited, and a generated string moves the reviewable shape out of the file. The ten clauses stay explicit, and a test parses them.
  • structure.sql as the inventory the test pins against, rather than the migration files. It is the resolved schema, so a dropped foreign key is reflected, and CI's db:structure-check re-dumps and diffs it, so it cannot drift from the migrations.

Non-goals

  • Updating the composers beyond the two absorbed steps. Each remaining composer is its own step of the same plan.
  • An EXPLAIN plan-shape pin. The plan asks for one where an index changed, none did, and pruning was verified empirically. Plan-shape pins are a known flake class in this package.
  • Emitting a metric or log when the check declines to delete. Needs a caller change, and #54's planned query counter carries no outcome label. See the follow-up in Attachment reference-check follow-ups: outcome ... (#607) • Unassigned.

Spec coverage

Spec: docs/specs/S17-rest-management-api.md. Plan: Step 10. Rows owned by the handler and bulk steps (authentication, authorization, and the 409, 400, and 404 error cases) are omitted.

# Criterion Tests
AC-38 Whatever removes an artifact row removes its attachment row, so a blob whose last artifact row is gone has zero attachments ..._DeleteIfUnreferenced_EveryReferencingTable (prune half, ten tables). The same-transaction composition belongs to the composer steps that call this
AC-38 A marked but unreaped artifact still holds its attachment ..._MarkedRowStillCounts, TestDeleteUnreferencedAttachmentStmt_CarriesNoDeletionMarkerFilter
B-1 Every referencing table is counted, the remote-cache tables included, or the check frees an attachment a cached row is using TestDeleteUnreferencedAttachmentStmt_GuardsEveryReferencingTable (set equality against the schema dump), ..._EveryReferencingTable (ten survivor subtests), TestAttachmentReferenceCases_CoverEveryGuardedTable
B-2 ADR-010 collects a blob once it has zero attachments, so a stranded attachment makes its blob permanently uncollectable Motivates B-1 and AC-38. Not separately assertable while blob collection is unimplemented (B-5)
B-3 A marking write removes nothing ..._MarkedRowStillCounts
B-4 The reverse-reference check is the one place the deletion-marker predicate must not go TestDeleteUnreferencedAttachmentStmt_CarriesNoDeletionMarkerFilter, ..._MarkedRowStillCounts
B-5 Reclaiming the blob itself is not implemented, so a delete frees rows and no storage Pre-existing ..._DeleteIfUnreferenced/prunes_an_orphaned_attachment_link_and_leaves_the_CAS_blob
P-1 An attachment referenced only by a marked, unreaped row survives ..._MarkedRowStillCounts
P-2 An attachment referenced by any referencing table survives ..._EveryReferencingTable (survivor half, ten subtests)
P-3 An attachment referenced by none deletes ..._EveryReferencingTable (prune half)
P-4 The existing container manifest-delete behavior is unchanged Pre-existing ..._DeleteIfUnreferenced (three subtests, unchanged and passing)
P-5 A unit test pins the table list against the FK inventory, so a further referencing table fails a test rather than leaking TestDeleteUnreferencedAttachmentStmt_GuardsEveryReferencingTable (missing and extra both fail), with a seven-layout fixture matrix over the dump parser
E-1 A referencing row in an uncounted table: the foreign key is NO ACTION, so the DELETE raises 23503 and aborts the caller's transaction instead of reporting a no-op ..._EveryReferencingTable requires no error and 0 rows on every table, which is what failed before the widening
E-2 A duplicate clause reads as coverage while counting nothing new TestDeleteUnreferencedAttachmentStmt_EnumeratesEachTableOnce
S-1 Tenant isolation: every clause is scoped by namespace_id, the partition key TestDeleteUnreferencedAttachmentStmt_CorrelatesOnNamespaceAndAttachmentID (per clause, so one uncorrelated clause among ten reddens), ..._AnotherAttachmentDoesNotHold
S-2 Injection: the statement is a compile-time constant and its three inputs are bound TestDeleteUnreferencedAttachmentStmt_OuterDeleteCarriesThePartitionKey pins the shape

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against an ephemeral PostgreSQL 17 container (matching GL_PG_CURR_VERSION from .gitlab-ci-other-versions.yml), with synthesized seed data rolled back per query and the container torn down at the end of the run. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing, methodology, and the anomalies the skill flags. Expand each row's details for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
BlobStorageAttachmentStore.DeleteIfUnreferenced Delete index_blob_storage_attachments_on_namespace_id_and_sha256 driving, plus each referencing table's (namespace_id, blob_storage_attachment_id) index 0 / 0 92.89 0.967ms 36 / 0 1/64 on each of 11 tables

Query notes:

  • The ten foreign-key re-check triggers that follow the delete are schema-driven rather than statement-driven. The single-arm statement at the merge base fires the same ten in the same seeded schema (0.564 ms), so trigger time tracks how many tables carry a foreign key into blob_storage_attachments rather than how many guard clauses the statement carries. The widening did not create that cost.
  • The 1.204 ms planning and 0.967 ms execution figures are medians of nine warm runs, each of which re-prepares. Holding one prepared statement instead makes the sixth execution build a generic plan across all 64 partitions of all 11 tables, costing 145 ms of planning once. Production cannot reach that path, because the service pins QueryExecModeSimpleProtocol and replans per execution, so the medians above are the shape a caller sees.
BlobStorageAttachmentStore.DeleteIfUnreferenced

Summary: Plan matches the method's intent. Eleven Index Scans stack into ten Nested Loop Anti Joins, one per referencing table, with every table pruned to a single partition of 64 and no node reading from disk. The ten foreign-key re-check triggers that follow the delete are schema-driven rather than statement-driven: the same ten fire on the single-arm statement at the merge base, so they are not a cost of the guard clauses. No anomalies.

Seed shape: namespaces=1, repositories=6, container_repositories=1, container_images=1, container_remote_repositories=1, container_remote_images=1, maven_repositories=1, maven_packages=1, maven_remote_repositories=1, maven_remote_packages=1, npm_repositories=1, npm_packages=16668, npm_versions=1, npm_remote_repositories=1, npm_remote_packages=16668, npm_remote_versions=1, blob_storage_blobs=50001, blob_storage_attachments=50001, container_blobs=50000, container_manifests=50000, container_remote_blobs=50000, container_remote_manifests=50000, maven_files=50000, maven_remote_files=50000, npm_files=50000, npm_metadata_files=50000, npm_remote_files=50000, npm_remote_metadata_files=50000

Rendered SQL:

DELETE FROM blob_storage_attachments bsa
	WHERE bsa.namespace_id = $1 AND bsa.id = $2 AND bsa.sha256 = $3
	AND NOT EXISTS (
		SELECT 1 FROM container_blobs ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM container_manifests ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM container_remote_blobs ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM container_remote_manifests ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM maven_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM maven_remote_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM npm_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM npm_metadata_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM npm_remote_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)
	AND NOT EXISTS (
		SELECT 1 FROM npm_remote_metadata_files ref
		WHERE ref.namespace_id = bsa.namespace_id AND ref.blob_storage_attachment_id = bsa.id
	)

Bound args: [52f3a893-b524-47f0-b761-5000b744defd, 150003, \x000000000000000000000000000000000000000000000000000000000030e22c]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Delete on blob_storage_attachments bsa  (cost=4.56..92.89 rows=0 width=0) (actual time=0.115..0.116 rows=0 loops=1)
  Delete on blob_storage_attachments_p00 bsa_1
  Buffers: shared hit=36
  ->  Nested Loop Anti Join  (cost=4.56..92.89 rows=1 width=110) (actual time=0.103..0.105 rows=1 loops=1)
        Buffers: shared hit=34
        ->  Nested Loop Anti Join  (cost=4.15..84.44 rows=1 width=124) (actual time=0.092..0.094 rows=1 loops=1)
              Buffers: shared hit=31
              ->  Nested Loop Anti Join  (cost=3.73..76.00 rows=1 width=114) (actual time=0.084..0.086 rows=1 loops=1)
                    Buffers: shared hit=28
                    ->  Nested Loop Anti Join  (cost=3.32..67.55 rows=1 width=104) (actual time=0.076..0.078 rows=1 loops=1)
                          Buffers: shared hit=25
                          ->  Nested Loop Anti Join  (cost=2.90..59.11 rows=1 width=94) (actual time=0.066..0.068 rows=1 loops=1)
                                Buffers: shared hit=22
                                ->  Nested Loop Anti Join  (cost=2.49..50.66 rows=1 width=84) (actual time=0.057..0.059 rows=1 loops=1)
                                      Buffers: shared hit=19
                                      ->  Nested Loop Anti Join  (cost=2.07..42.22 rows=1 width=74) (actual time=0.050..0.051 rows=1 loops=1)
                                            Buffers: shared hit=16
                                            ->  Nested Loop Anti Join  (cost=1.66..33.77 rows=1 width=64) (actual time=0.042..0.044 rows=1 loops=1)
                                                  Buffers: shared hit=13
                                                  ->  Nested Loop Anti Join  (cost=1.24..25.33 rows=1 width=54) (actual time=0.034..0.035 rows=1 loops=1)
                                                        Buffers: shared hit=10
                                                        ->  Nested Loop Anti Join  (cost=0.83..16.88 rows=1 width=44) (actual time=0.027..0.028 rows=1 loops=1)
                                                              Buffers: shared hit=7
                                                              ->  Index Scan using blob_storage_attachments_p00_namespace_id_sha256_idx on blob_storage_attachments_p00 bsa_1  (cost=0.41..8.44 rows=1 width=34) (actual time=0.016..0.017 rows=1 loops=1)
                                                                    Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (sha256 = '\x000000000000000000000000000000000000000000000000000000000030e22c'::bytea))
                                                                    Filter: (id = '150003'::bigint)
                                                                    Buffers: shared hit=4
                                                              ->  Index Scan using container_blobs_p07_namespace_id_blob_storage_attachment_id_idx on container_blobs_p07 ref  (cost=0.41..8.43 rows=1 width=34) (actual time=0.009..0.009 rows=0 loops=1)
                                                                    Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                                                    Buffers: shared hit=3
                                                        ->  Index Scan using container_manifests_p07_namespace_id_blob_storage_attachmen_idx on container_manifests_p07 ref_1  (cost=0.41..8.43 rows=1 width=34) (actual time=0.007..0.007 rows=0 loops=1)
                                                              Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                                              Buffers: shared hit=3
                                                  ->  Index Scan using container_remote_blobs_p07_namespace_id_blob_storage_attach_idx on container_remote_blobs_p07 ref_2  (cost=0.41..8.43 rows=1 width=34) (actual time=0.008..0.008 rows=0 loops=1)
                                                        Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                                        Buffers: shared hit=3
                                            ->  Index Scan using container_remote_manifests_p0_namespace_id_blob_storage_at_idx7 on container_remote_manifests_p07 ref_3  (cost=0.41..8.43 rows=1 width=34) (actual time=0.007..0.007 rows=0 loops=1)
                                                  Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                                  Buffers: shared hit=3
                                      ->  Index Scan using maven_files_p07_namespace_id_blob_storage_attachment_id_idx on maven_files_p07 ref_4  (cost=0.41..8.43 rows=1 width=34) (actual time=0.007..0.007 rows=0 loops=1)
                                            Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                            Buffers: shared hit=3
                                ->  Index Scan using maven_remote_files_p07_namespace_id_blob_storage_attachment_idx on maven_remote_files_p07 ref_5  (cost=0.41..8.43 rows=1 width=34) (actual time=0.008..0.008 rows=0 loops=1)
                                      Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                      Buffers: shared hit=3
                          ->  Index Scan using npm_files_p07_namespace_id_blob_storage_attachment_id_idx on npm_files_p07 ref_6  (cost=0.41..8.43 rows=1 width=34) (actual time=0.010..0.010 rows=0 loops=1)
                                Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                                Buffers: shared hit=3
                    ->  Index Scan using npm_metadata_files_p07_namespace_id_blob_storage_attachment_idx on npm_metadata_files_p07 ref_7  (cost=0.41..8.43 rows=1 width=34) (actual time=0.007..0.007 rows=0 loops=1)
                          Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                          Buffers: shared hit=3
              ->  Index Scan using npm_remote_files_p07_namespace_id_blob_storage_attachment_i_idx on npm_remote_files_p07 ref_8  (cost=0.41..8.43 rows=1 width=34) (actual time=0.008..0.008 rows=0 loops=1)
                    Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
                    Buffers: shared hit=3
        ->  Index Scan using npm_remote_metadata_files_p07_namespace_id_blob_storage_att_idx on npm_remote_metadata_files_p07 ref_9  (cost=0.41..8.43 rows=1 width=34) (actual time=0.010..0.010 rows=0 loops=1)
              Index Cond: ((namespace_id = '52f3a893-b524-47f0-b761-5000b744defd'::uuid) AND (blob_storage_attachment_id = '150003'::bigint))
              Buffers: shared hit=3
Planning:
  Buffers: shared hit=4
Planning Time: 1.159 ms
Trigger for constraint container_blobs_blob_storage_attachment_id_namespace_id_bl_fkey on blob_storage_attachments_p00: time=0.094 calls=1
Trigger for constraint container_manifests_blob_storage_attachment_id_namespace_i_fkey on blob_storage_attachments_p00: time=0.073 calls=1
Trigger for constraint npm_files_blob_storage_attachment_id_namespace_id_blob_sha_fkey on blob_storage_attachments_p00: time=0.066 calls=1
Trigger for constraint npm_metadata_files_blob_storage_attachment_id_namespace_id_fkey on blob_storage_attachments_p00: time=0.067 calls=1
Trigger for constraint maven_files_blob_storage_attachment_id_namespace_id_blob_s_fkey on blob_storage_attachments_p00: time=0.066 calls=1
Trigger for constraint npm_remote_metadata_files_blob_storage_attachment_id_names_fkey on blob_storage_attachments_p00: time=0.068 calls=1
Trigger for constraint npm_remote_files_blob_storage_attachment_id_namespace_id_b_fkey on blob_storage_attachments_p00: time=0.068 calls=1
Trigger for constraint maven_remote_files_blob_storage_attachment_id_namespace_id_fkey on blob_storage_attachments_p00: time=0.069 calls=1
Trigger for constraint container_remote_manifests_blob_storage_attachment_id_name_fkey on blob_storage_attachments_p00: time=0.064 calls=1
Trigger for constraint container_remote_blobs_blob_storage_attachment_id_namespac_fkey on blob_storage_attachments_p00: time=0.062 calls=1
Execution Time: 0.967 ms

Timings: planning 1.204ms, execution 0.967ms, total 2.171ms (medians of nine warm runs; the plan above is the run whose execution time equals that median, and its own planning footer reads 1.159ms).

Related to #313 (closed)

Edited by Hayley Swimelar

Merge request reports