Skip to content
Snippets Groups Projects

Add new jsonb column to store settings for default branch protection

Merged Michael Becker requested to merge feat/408150 into master

What does this MR do and why?

Currently, the options for default branch protection at the instance and group level lag behind, and are not as fine-grained, as the options available from the Protected Branches feature

The branch defaults lag behind because the current implementation uses an integer column to store the default settings, which becomes difficult to expand as new options and finer detail controls are added to the protected branches feature

at a high-ish level the existing feature is implemented as:

  1. there are integer columns on the database tables (application level and group level)
  2. these integers are mapped to a subset of protected branch settings
  3. when creating a default branch, those settings are passed into the protected branch service

To offer better support of protected branches features on the default branch, we can:

  1. use a jsonb column rather than an integer
  2. update the settings API to accept a payload the matches the protected branches API
  3. Pass those settings into the ProtecteBrancheService

This commit performs step 1, by adding a new jsonb column

resolves: #408150 (closed) Changelog: added

Screenshots or screen recordings

Screenshots are required for UI changes, and strongly recommended for all other merge requests.

How to set up and validate locally

Numbered steps to set up and validate the change are strongly suggested.

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 Michael Becker

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Kamil Trzciński removed review request for @ayufan

    removed review request for @ayufan

  • Jon Jenkins removed review request for @jon_jenkins

    removed review request for @jon_jenkins

  • Michael Becker added 2018 commits

    added 2018 commits

    • 582f5b52...1c610394 - 2016 commits from branch master
    • ee2f1727 - Add new jsonb column to store settings for default branch protection
    • 62251a8a - Move namespace level column to the `namespace_settings` table

    Compare with previous version

  • Michael Becker added 151 commits

    added 151 commits

    • 62251a8a...9ebaaeea - 149 commits from branch master
    • f3815cee - Add new jsonb column to store settings for default branch protection
    • 04d47744 - Move namespace level column to the `namespace_settings` table

    Compare with previous version

  • Michael Becker added 1 commit

    added 1 commit

    • 39d015c5 - Move namespace level column to the `namespace_settings` table

    Compare with previous version

  • Michael Becker added 1 commit

    added 1 commit

    • e3067815 - Move namespace level column to the `namespace_settings` table

    Compare with previous version

  • Michael Becker added 3 commits

    added 3 commits

    • 399f9df4 - Add new jsonb column to store settings for default branch protection
    • 870beaea - Move namespace level column to the `namespace_settings` table
    • ed2e610b - Make json schema stricter

    Compare with previous version

  • Michael Becker requested review from @ayufan and @jon_jenkins

    requested review from @ayufan and @jon_jenkins

  • LGTM! Passing on to @DylanGriffith for database maintainer review.

  • added databasereviewed label and removed databasereview pending label

  • Jon Jenkins requested review from @DylanGriffith and removed review request for @jon_jenkins

    requested review from @DylanGriffith and removed review request for @jon_jenkins

  • additive schema changes; no impact to data warehouse

    • Resolved by Michael Becker

      @wandering_person I feel like I need some more context on why this needs to be a JSON column rather than relying on modelling the features directly in normal Postgres types/tables/columns . I realise the JSONB can seem like it is much more efficient but excessive use of JSON can become more difficult maintenance over time. This forces us to additionally define schemas in the JSON schema validation but that doesn't give the same guarantees as Postgres constraints. Migrations can become more difficult with JSON columns because you can't guarantee the current schema unless you had Postgres constraints. Also there are additional performance considerations with storing JSONB columns because they can end up much larger than integers, end up in a TOAST column which then makes reading the rows from tables like namespaces much less performant as you have to lookup another TOAST table.

      I don't think we have a hard and fast rule about when to use JSONB but my instinct is to use it sparingly and only for cases where modelling the data statically is impossible or when the backend doesn't really care about the contents.

      To help me understand this a little better perhaps you can expand a little on:

      1. What is the next feature(s) you want to build on top of this
      2. What would the modelling look like without JSONB
      3. What would the modelling look like with JSONB
  • Contributor

    Started database testing pipeline (limited access). This comment will be updated once the pipeline has finished running.

  • Contributor

    Database migrations (on the main database)

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings Regular 1.5 s :white_check_mark: +0.00 B
    20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings Regular 1.5 s :white_check_mark: +0.00 B
    20230524124855 - AddSizeConstraintToNamespaceSettingsJson Regular 1.7 s :white_check_mark: +0.00 B
    20230524124856 - AddSizeConstraintToApplicationSettingsJson Regular 1.7 s :white_check_mark: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 14
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings

    • Type: Regular
    • Duration: 1.5 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 7.8 ms 7.8 ms 7.8 ms 0
    ALTER TABLE "application_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddDefaultBranchProtectionsJsonToApplicationSettings
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings

    • Type: Regular
    • Duration: 1.5 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 3.1 ms 3.1 ms 3.1 ms 0
    ALTER TABLE "namespace_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for AddDefaultBranchProtectionsJsonToNamespaceSettings
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124855 - AddSizeConstraintToNamespaceSettingsJson

    • Type: Regular
    • Duration: 1.7 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 0.7 ms 0.7 ms 0.7 ms 0
    ALTER TABLE namespace_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddSizeConstraintToNamespaceSettingsJson
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124856 - AddSizeConstraintToApplicationSettingsJson

    • Type: Regular
    • Duration: 1.7 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 1.9 ms 1.9 ms 1.9 ms 0
    ALTER TABLE application_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddSizeConstraintToApplicationSettingsJson
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    Other migrations pending on GitLab.com
    Migration Type Total runtime Result DB size change
    20230522103433 - RemoveGitHubImportDeprecatedWorkers Regular 1.7 s :white_check_mark: +0.00 B
    20230524095108 - RemoveIndexOnNameOnOrganization Regular 2.6 s :white_check_mark: -16.00 KiB
    20230522220709 - EnsureIncidentWorkItemTypeBackfillIsFinished Post deploy 1.7 s :white_check_mark: +0.00 B
    20230522225610 - RemoveTmpIndexIssuesOnIssueTypeAndIdOnlyIncidents Post deploy 3.3 s :white_check_mark: -8.86 MiB
    20230523131914 - RecreateIndexOnVulnerabilityReads Post deploy 179.9 s :white_check_mark: -118.62 MiB
    20230523132647 - RecreateIndexOnVulnerabilityReads2 Post deploy 179.1 s :white_check_mark: -112.62 MiB
    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-1977426-10184547-main 2023-05-30T04:47:33Z 2023-05-27T02:59:07Z 2023-05-30 16:58:16 +0000
    database-testing-1977426-10184547-ci 2023-05-30T04:47:33Z 2023-05-26T15:37:27Z 2023-05-30 16:58:16 +0000

    Job artifacts

    Database migrations (on the ci database)

    Migrations included in this change have been executed on gitlab.com data for testing purposes. For details, please see the migration testing pipeline (limited access).

    Migration Type Total runtime Result DB size change
    20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings Regular 2.1 s :white_check_mark: +0.00 B
    20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings Regular 2.1 s :white_check_mark: +0.00 B
    20230524124855 - AddSizeConstraintToNamespaceSettingsJson Regular 2.3 s :white_check_mark: +0.00 B
    20230524124856 - AddSizeConstraintToApplicationSettingsJson Regular 2.3 s :white_check_mark: +0.00 B
    Runtime Histogram for all migrations
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 14
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124754 - AddDefaultBranchProtectionsJsonToApplicationSettings

    • Type: Regular
    • Duration: 2.1 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 7.3 ms 7.3 ms 7.3 ms 0
    ALTER TABLE "application_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for AddDefaultBranchProtectionsJsonToApplicationSettings
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124854 - AddDefaultBranchProtectionsJsonToNamespaceSettings

    • Type: Regular
    • Duration: 2.1 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 4.0 ms 4.0 ms 4.0 ms 0
    ALTER TABLE "namespace_settings" ADD "default_branch_protection_defaults" jsonb DEFAULT '{}' NOT NULL
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    1 0.0 ms 0.0 ms 0.0 ms 1
    SELECT $1::regtype::oid
    Histogram for AddDefaultBranchProtectionsJsonToNamespaceSettings
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 4
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124855 - AddSizeConstraintToNamespaceSettingsJson

    • Type: Regular
    • Duration: 2.3 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 3.0 ms 3.0 ms 3.0 ms 0
    ALTER TABLE namespace_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddSizeConstraintToNamespaceSettingsJson
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Migration: 20230524124856 - AddSizeConstraintToApplicationSettingsJson

    • Type: Regular
    • Duration: 2.3 s
    • Database size change: +0.00 B
    Calls Total Time Max Time Mean Time Rows Query
    1 5.0 ms 5.0 ms 5.0 ms 0
    ALTER TABLE application_settings ADD CONSTRAINT default_branch_protection_defaults_size_constraint CHECK ( octet_length(default_branch_protection_defaults::text) <= 1024 ) NOT VALID
    2 0.0 ms 0.0 ms 0.0 ms 2
    SELECT pg_backend_pid()
    Histogram for AddSizeConstraintToApplicationSettingsJson
    Query Runtime Count
    0 seconds - 0.01 seconds 0
    0.01 seconds - 0.1 seconds 3
    0.1 seconds - 1 second 0
    1 second - 5 seconds 0
    5 seconds - 15 seconds 0
    15 seconds - 5 minutes 0
    5 minutes + 0

    Other information

    Other migrations pending on GitLab.com
    Migration Type Total runtime Result DB size change
    20230522103433 - RemoveGitHubImportDeprecatedWorkers Regular 2.3 s :white_check_mark: +0.00 B
    20230524095108 - RemoveIndexOnNameOnOrganization Regular 2.5 s :white_check_mark: -8.00 KiB
    20230522220709 - EnsureIncidentWorkItemTypeBackfillIsFinished Post deploy 2.0 s :white_check_mark: +0.00 B
    20230522225610 - RemoveTmpIndexIssuesOnIssueTypeAndIdOnlyIncidents Post deploy 3.9 s :white_check_mark: -8.00 KiB
    20230523131914 - RecreateIndexOnVulnerabilityReads Post deploy 4.2 s :white_check_mark: +0.00 B
    20230523132647 - RecreateIndexOnVulnerabilityReads2 Post deploy 4.2 s :white_check_mark: +0.00 B
    Clone details
    Clone ID Clone Created At Clone Data Timestamp Expected Removal Time
    database-testing-1977426-10184547-main 2023-05-30T04:47:33Z 2023-05-27T02:59:07Z 2023-05-30 16:58:16 +0000
    database-testing-1977426-10184547-ci 2023-05-30T04:47:33Z 2023-05-26T15:37:27Z 2023-05-30 16:58:16 +0000

    Job artifacts


    Brought to you by gitlab-org/database-team/gitlab-com-database-testing. Epic

  • @wandering_person I left some thoughts. The migration looks fine to me but my main comment is just around understanding why JSONB is important for this use case. In my experience the maintenance of schemaless columns like this tends to end up being more effort than just getting all the tables in place from the beginning.

  • Dylan Griffith removed review request for @DylanGriffith

    removed review request for @DylanGriffith

  • requested review from @DylanGriffith

  • Michael Becker removed review request for @DylanGriffith

    removed review request for @DylanGriffith

  • Kamil Trzciński removed review request for @ayufan

    removed review request for @ayufan

  • Michael Becker added 664 commits

    added 664 commits

    • ed2e610b...f6872ca1 - 660 commits from branch master
    • 520e1aac - Add new jsonb column to store settings for default branch protection
    • 3b9a7760 - Move namespace level column to the `namespace_settings` table
    • 2ef3644f - Make json schema stricter
    • b0be7526 - Add db-level size constraint to jsonb columns

    Compare with previous version

  • A deleted user added documentation label

    added documentation label

  • Michael Becker added 1 commit

    added 1 commit

    Compare with previous version

  • Michael Becker resolved all threads

    resolved all threads

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading