Skip to content

Moving nullify from before_destroy to destroy

Max Fan requested to merge 328596-move-nullify-to-destroy into master

What does this MR do and why?

From the previous MR. Update All on all pipelines was being called before batching.

Unfortunately, it doesn't look like the order of executions was correct, as the error is still happening: https://sentry.gitlab.net/gitlab/gitlabcom/issues/4103394/events/latest/

This is to hopefully change that, so the query does not timeout anymore

Screenshots or screen recordings

New Database Queries

[1] pry(main)> Ci::PipelineSchedule.last.destroy
  Ci::PipelineSchedule Load (2.1ms)  SELECT "ci_pipeline_schedules".* FROM "ci_pipeline_schedules" ORDER BY "ci_pipeline_schedules"."id" DESC LIMIT 1 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:(pry):1:in `__pry__'*/
  Ci::Pipeline Update All (2.2ms)  UPDATE "ci_pipelines" SET "pipeline_schedule_id" = NULL, "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "ci_pipelines"."id" IN (SELECT "ci_pipelines"."id" FROM "ci_pipelines" WHERE "ci_pipelines"."pipeline_schedule_id" = 29 LIMIT 100) /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/app/models/concerns/batch_nullify_dependent_associations.rb:21:in `block (2 levels) in nullify_dependent_associations_in_batches'*/
  TRANSACTION (0.1ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/app/models/ci/pipeline_schedule.rb:89:in `destroy'*/
  Ci::Pipeline Update All (0.2ms)  UPDATE "ci_pipelines" SET "pipeline_schedule_id" = NULL, "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "ci_pipelines"."pipeline_schedule_id" = 29 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/app/models/ci/pipeline_schedule.rb:89:in `destroy'*/
  Ci::PipelineSchedule Destroy (0.7ms)  DELETE FROM "ci_pipeline_schedules" WHERE "ci_pipeline_schedules"."id" = 29 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/app/models/ci/pipeline_schedule.rb:89:in `destroy'*/
  TRANSACTION (0.1ms)  COMMIT /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/lib/gitlab/database.rb:380:in `commit'*/

Previous Database Queries

[28] pry(main)> Ci::PipelineSchedule.last.destroy
  Ci::PipelineSchedule Load (1.7ms)  SELECT "ci_pipeline_schedules".* FROM "ci_pipeline_schedules" ORDER BY "ci_pipeline_schedules"."id" DESC LIMIT 1 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:(pry):28:in `__pry__'*/
  TRANSACTION (0.2ms)  BEGIN /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/lib/gitlab/database/schema_cache_with_renamed_table.rb:25:in `columns'*/
  Ci::Pipeline Update All (1.5ms)  UPDATE "ci_pipelines" SET "pipeline_schedule_id" = NULL, "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "ci_pipelines"."pipeline_schedule_id" = 23 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:(pry):28:in `__pry__'*/
  Ci::Pipeline Update All (0.7ms)  UPDATE "ci_pipelines" SET "pipeline_schedule_id" = NULL, "lock_version" = COALESCE("lock_version", 0) + 1 WHERE "ci_pipelines"."id" IN (SELECT "ci_pipelines"."id" FROM "ci_pipelines" WHERE "ci_pipelines"."pipeline_schedule_id" = 23 LIMIT 100) /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/app/models/concerns/batch_nullify_dependent_associations.rb:21:in `block (2 levels) in nullify_dependent_associations_in_batches'*/
  Ci::PipelineSchedule Destroy (0.5ms)  DELETE FROM "ci_pipeline_schedules" WHERE "ci_pipeline_schedules"."id" = 23 /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:(pry):28:in `__pry__'*/
  TRANSACTION (0.2ms)  COMMIT /*application:console,db_config_name:ci,console_hostname:MaxPro.local,console_username:maxfan,line:/lib/gitlab/database.rb:380:in `commit'*/

The Key Differences are

  1. Ci::Pipeline Update All with batching is called before Ci::Pipeline Update All total
  2. Update All call is no longer in the same transaction block

How to set up and validate locally

  1. Create a pipelineSchedule http://172.16.123.1:3000/root/maxci/-/pipeline_schedules
  2. In the console destroy the schedule and see the queries

MR acceptance checklist

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

Related to #328596 (closed)

Edited by Max Fan

Merge request reports