Skip to content

Fix cleanup_package_file_worker specs on PG11

Peter Leitzen requested to merge pl-fix-cleanup-package-file-worker-pg11 into master

What does this MR do and why?

This MR fixes a failureflaky-test (only on PG11 - introduced in !84073 (merged)) which relied on implicit ID ordering when cleaning up package files.

This MR also adds a composite index for status=1 and id to packages_package_files to make the 🆕 resulting SQL query performant.

Closes #359097 (closed).

Note, with pipeline:run-all-rspec we are also running pg11 specs to verify this fix.

database migration

CREATE INDEX index_packages_package_files_on_id_for_cleanup ON packages_package_files USING btree (id) WHERE (status = 1);

Migration took ~4 minutes on #database-lab (internal):

The query has been executed. Duration: 4.176 min

rake db:migrate 

== 20220420192542 AddIdForCleanupIndexPackagesPackageFiles: migrating =========
-- transaction_open?()
   -> 0.0000s
-- index_exists?(:packages_package_files, :id, {:name=>"index_packages_package_files_on_id_for_cleanup", :where=>"status = 1", :algorithm=>:concurrently})
   -> 0.0062s
-- execute("SET statement_timeout TO 0")
   -> 0.0005s
-- add_index(:packages_package_files, :id, {:name=>"index_packages_package_files_on_id_for_cleanup", :where=>"status = 1", :algorithm=>:concurrently})
   -> 0.0036s
-- execute("RESET statement_timeout")
   -> 0.0534s
== 20220420192542 AddIdForCleanupIndexPackagesPackageFiles: migrated (0.0703s)

rake db:rollback 

== 20220420192542 AddIdForCleanupIndexPackagesPackageFiles: reverting =========
-- transaction_open?()
   -> 0.0000s
-- indexes(:packages_package_files)
   -> 0.0065s
-- execute("SET statement_timeout TO 0")
   -> 0.0004s
-- remove_index(:packages_package_files, {:algorithm=>:concurrently, :name=>"index_packages_package_files_on_id_for_cleanup"})
   -> 0.0034s
-- execute("RESET statement_timeout")
   -> 0.0005s
== 20220420192542 AddIdForCleanupIndexPackagesPackageFiles: reverted (0.0160s)

database Query plan

Packages::PackageFile.next_pending_destruction(order_by: :id) 

SELECT "packages_package_files".* FROM "packages_package_files" WHERE "packages_package_files"."status" = 1 ORDER BY "packages_package_files"."id" ASC LIMIT 1 FOR UPDATE SKIP LOCKED

I've tested on #database-lab (internal):

 Limit  (cost=0.28..0.51 rows=1 width=841) (actual time=0.264..0.265 rows=1 loops=1)
   Buffers: shared hit=2 read=2
   I/O Timings: read=0.092 write=0.000
   ->  LockRows  (cost=0.28..788.22 rows=3412 width=841) (actual time=0.261..0.262 rows=1 loops=1)
         Buffers: shared hit=2 read=2
         I/O Timings: read=0.092 write=0.000
         ->  Index Scan using index_packages_package_files_on_id_for_cleanup on public.packages_package_files  (cost=0.28..754.10 rows=3412 width=841) (actual time=0.174..0.175 rows=1 loops=1)
               Filter: (packages_package_files.status = 1)
               Rows Removed by Filter: 0
               Buffers: shared hit=1 read=2
               I/O Timings: read=0.092 write=0.000

Packages::PackageFile.pending_destruction.exists? 

SELECT 1 AS one FROM "packages_package_files" WHERE "packages_package_files"."status" = 1 LIMIT 1;
 Limit  (cost=0.28..0.30 rows=1 width=4) (actual time=0.088..0.088 rows=1 loops=1)
   Buffers: shared hit=4
   I/O Timings: read=0.000 write=0.000
   ->  Index Only Scan using index_packages_package_files_on_id_for_cleanup on public.packages_package_files  (cost=0.28..84.08 rows=3412 width=4) (actual time=0.086..0.086 rows=1 loops=1)
         Heap Fetches: 1
         Buffers: shared hit=4
         I/O Timings: read=0.000 write=0.000

Screenshots or screen recordings

n/a

How to set up and validate locally

  1. Run PG 11.6 on GDK
  2. Run bin/rspec ./spec/workers/packages/cleanup_package_file_worker_spec.rb -e 'with work to do'

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Peter Leitzen

Merge request reports