Skip to content

Use direct SQL to generate internal ids

Patrick Bair requested to merge pb-generate-iid-in-db-behind-ff into master

What does this MR do?

Related issue: #325861 (closed)

Reimplement changes from !64240 (closed) that removes database round-trips and explicit locking when updating internal_ids. We make the assumption on generating a new value that most operations will modify existing records, so we prefer to attempt to update the row first. After some discussion, we decided to put consider putting the changes behind a feature flag, so this MR was created to do that (rather than the direct cutover in the previous MR).

Example of creation of object with internal_id, where no existing record exists (create_record!):

  Update InternalId (0.7ms)  UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + 1) WHERE "internal_ids"."project_id" = 7 AND "internal_ids"."usage" = 0 RETURNING "last_value" /*application:console,line:/app/models/internal_id.rb:91:in `update_record!'*/
  TRANSACTION (0.1ms)  SAVEPOINT active_record_1 /*application:console,line:/app/models/concerns/atomic_internal_id.rb:211:in `block in project_init'*/
   (2.1ms)  SELECT MAX("issues"."iid") FROM "issues" WHERE "issues"."project_id" = 7 /*application:console,line:/app/models/concerns/atomic_internal_id.rb:211:in `block in project_init'*/
  InternalId Create (0.7ms)  INSERT INTO "internal_ids" ("project_id", "usage", "last_value") VALUES (7, 0, 1) RETURNING "id" /*application:console,line:/app/models/internal_id.rb:102:in `block in create_record!'*/

First attempts to update the existing record, and because none is found, it creates a new record. If the new record were to conflict with a concurrent insert, it retries the update.

Example of update of object with internal_id, where a record exists (update_record!):

 Update InternalId (0.3ms)  UPDATE "internal_ids" SET "last_value" = ("internal_ids"."last_value" + 1) WHERE "internal_ids"."project_id" = 7 AND "internal_ids"."usage" = 0 RETURNING "last_value" /*application:console,line:/app/models/internal_id.rb:91:in `update_record!'*/

It updates the record only.

Screenshots or Screencasts (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

Does this MR contain changes to processing or storing of credentials or tokens, authorization and authentication methods or other items described in the security review guidelines? If not, then delete this Security section.

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Patrick Bair

Merge request reports

Loading