Validate uniqnuess of member role name

changed milestone to %16.10

added Category:Permissions backend devopsgovern featureenhancement groupauthorization missed:16.9 priority3 quick win sectionsec typefeature workflowready for development labels

assigned to @jarka

added database databasereview pending labels

2 Warnings

This MR changes code in ee/, but its Changelog commit is missing the EE: true trailer. Consider adding it to your Changelog commits.

featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.

For more information, see:

The Handbook page on merge request types.
The definition of done documentation.

Reviewer roulette

Category	Reviewer	Maintainer
backend	`@mc_rocha` (UTC-5, 6 hours behind author)	`@alejandro` (UTC-5, 6 hours behind author)
database	`@huzaifaiftikhar1` (UTC+5.5, 4.5 hours ahead of author)	`@dfrazao-gitlab` (UTC+1, same timezone as author)
~"Authorization"	Reviewer review is optional for ~"Authorization"	`@mokhax` (UTC-7, 8 hours behind author)

Please check reviewer's status!

Reviewer is available!
Reviewer is unavailable!

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

added 1618 commits

4e029849...7f11ee70 - 1617 commits from branch master
c2193e4b - Validate uniqnuess of member role name

Compare with previous version

added 1 commit

f9a4de0d - Validate uniqnuess of member role name

Compare with previous version

added 1 commit

a472e621 - Validate uniqnuess of member role name

Compare with previous version

added Data WarehouseImpact Check label

added 1 commit

0e130079 - Validate uniqnuess of member role name

Compare with previous version

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
20240210104125 - EnsureMemberRolesNamesUniq	Post deploy	2.0 s		+16.00 KiB
20240221134504 - AddNameUniqueIndexToMemberRoles	Post deploy	2.4 s		+16.00 KiB

Runtime Histogram for all migrations

Query Runtime	Count
0 seconds - 0.01 seconds	0
0.01 seconds - 0.1 seconds	6
0.1 seconds - 1 second	1
1 second - 5 seconds	0
5 seconds - 15 seconds	0
15 seconds - 5 minutes	0
5 minutes +	0

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Type: Post deploy
Duration: 2.0 s
Database size change: +16.00 KiB

Calls

Total Time

Max Time

Mean Time

Rows

Query

1

94.1 ms

58

UPDATE member_roles
  SET name = CONCAT(name, $1, id, $2)
  WHERE id IN (
    SELECT mr.id
    FROM member_roles mr
    WHERE EXISTS (
      SELECT mr_duplicates.id
      FROM member_roles mr_duplicates
      WHERE mr_duplicates.name = mr.name AND ( mr_duplicates.namespace_id = mr.namespace_id OR (mr_duplicates.namespace_id IS NULL AND mr.namespace_id IS NULL) ) AND mr_duplicates.id < mr.id
    )
  )

2

0.0 ms

2

SELECT pg_backend_pid()

Histogram for EnsureMemberRolesNamesUniq

Query Runtime	Count
0 seconds - 0.01 seconds	0
0.01 seconds - 0.1 seconds	2
0.1 seconds - 1 second	1
1 second - 5 seconds	0
5 seconds - 15 seconds	0
15 seconds - 5 minutes	0
5 minutes +	0

Migration: 20240221134504 - AddNameUniqueIndexToMemberRoles

Type: Post deploy
Duration: 2.4 s
Database size change: +16.00 KiB

Calls

Total Time

Max Time

Mean Time

Rows

Query

1

5.1 ms

0

CREATE UNIQUE INDEX CONCURRENTLY "index_member_roles_on_namespace_id_name_unique" ON "member_roles" ("namespace_id", "name")

1

2.9 ms

0

DROP INDEX CONCURRENTLY "index_member_roles_on_namespace_id"

2

0.0 ms

2

SELECT pg_backend_pid()

Histogram for AddNameUniqueIndexToMemberRoles

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

Other information

Other migrations pending on GitLab.com

Migration	Type	Total runtime	DB size change
20240201111294 - AddIndexOnIssuesTableTmpEpicIdColumn	Post deploy	2074.0 s	+1.87 GiB
20240201112204 - AddFkToTmpEpicIdColumnOnIssues	Post deploy	3.6 s	+0.00 B
20240219135417 - ReplaceOldFkCiBuildTraceMetadataToCiJobArtifacts	Post deploy	2.8 s	+0.00 B
20240219142421 - ReplaceOldFkCiJobArtifactStatesToCiJobArtifacts	Post deploy	2.2 s	+0.00 B
20240222000002 - FinalizeBackfillVsCodeSettingsUuid	Post deploy	1.9 s	+0.00 B

Clone details

Clone ID	Clone Created At	Clone Data Timestamp	Expected Removal Time
`database-testing-2898253-12938221-main`	2024-02-22T19:01:11Z	2024-02-21T19:41:26Z	2024-02-23 07:42:38 +0000
`database-testing-2898253-12938221-ci`	2024-02-22T19:01:11Z	2024-02-22T16:46:05Z	2024-02-23 07:42:38 +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
20240210104125 - EnsureMemberRolesNamesUniq	Post deploy	3.0 s		+0.00 B
20240221134504 - AddNameUniqueIndexToMemberRoles	Post deploy	3.8 s		+0.00 B

Runtime Histogram for all migrations

Query Runtime	Count
0 seconds - 0.01 seconds	0
0.01 seconds - 0.1 seconds	5
0.1 seconds - 1 second	1
1 second - 5 seconds	0
5 seconds - 15 seconds	0
15 seconds - 5 minutes	0
5 minutes +	0

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Type: Post deploy
Duration: 3.0 s
Database size change: +0.00 B

Calls	Total Time	Max Time	Mean Time	Rows	Query
2	0.0 ms	0.0 ms	0.0 ms	2	SELECT pg_backend_pid()

Histogram for EnsureMemberRolesNamesUniq

Query Runtime	Count
0 seconds - 0.01 seconds	0
0.01 seconds - 0.1 seconds	2
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: 20240221134504 - AddNameUniqueIndexToMemberRoles

Type: Post deploy
Duration: 3.8 s
Database size change: +0.00 B

Calls

Total Time

Max Time

Mean Time

Rows

Query

1

60.6 ms

0

CREATE UNIQUE INDEX CONCURRENTLY "index_member_roles_on_namespace_id_name_unique" ON "member_roles" ("namespace_id", "name")

1

41.0 ms

0

DROP INDEX CONCURRENTLY "index_member_roles_on_namespace_id"

2

0.0 ms

2

SELECT pg_backend_pid()

Histogram for AddNameUniqueIndexToMemberRoles

Query Runtime	Count
0 seconds - 0.01 seconds	0
0.01 seconds - 0.1 seconds	3
0.1 seconds - 1 second	1
1 second - 5 seconds	0
5 seconds - 15 seconds	0
15 seconds - 5 minutes	0
5 minutes +	0

Other information

No other migrations pending on GitLab.com

Clone details

Clone ID	Clone Created At	Clone Data Timestamp	Expected Removal Time
`database-testing-2898253-12938221-main`	2024-02-22T19:01:11Z	2024-02-21T19:41:26Z	2024-02-23 07:42:38 +0000
`database-testing-2898253-12938221-ci`	2024-02-22T19:01:11Z	2024-02-22T16:46:05Z	2024-02-23 07:42:38 +0000

Job artifacts

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

added database-testing-automation label

changed the description

@mc_rocha could you do the initial backend review?

And @ck3g could you do the initial database review?

requested review from @mc_rocha and @ck3g

approved this merge request

@mc_rocha, thanks for approving this merge request.

This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break master, a new pipeline will be started shortly.

Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.

added pipeline:mr-approved label

requested review from @rkadam3 and removed review request for @mc_rocha

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 48629cb6

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Govern      | 66     | 0      | 0       | 0     | 66    | ✅     |
| Monitor     | 4      | 0      | 0       | 0     | 4     | ✅     |
| Create      | 8      | 0      | 3       | 0     | 11    | ✅     |
| Plan        | 4      | 0      | 0       | 0     | 4     | ✅     |
| Data Stores | 2      | 0      | 0       | 0     | 2     | ✅     |
| Package     | 0      | 0      | 1       | 0     | 1     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 84     | 0      | 4       | 0     | 88    | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

e2e-package-and-test: test report for 48629cb6

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Govern      | 272    | 0      | 19      | 0     | 291   | ✅     |
| Create      | 153    | 0      | 19      | 0     | 172   | ✅     |
| Monitor     | 8      | 0      | 0       | 0     | 8     | ✅     |
| Package     | 0      | 0      | 2       | 0     | 2     | ➖     |
| Plan        | 8      | 0      | 0       | 0     | 8     | ✅     |
| Data Stores | 4      | 0      | 0       | 0     | 4     | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 445    | 0      | 40      | 0     | 485   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

approved this merge request

removed review request for @rkadam3

requested review from @ck3g

approved this merge request

added databasereviewed label and removed databasereview pending label

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

@jarka this approach seems sound but I left a couple of minor suggested improvements.

removed review request for @DylanGriffith

added 1258 commits

0e130079...f7cdd921 - 1256 commits from branch master
a2d97d88 - Validate uniqnuess of member role name
31da3e19 - Make sure instance level role names are uniq

Compare with previous version

reset approvals from @ck3g and @rkadam3 by pushing to the branch

added 1 commit

b191bcc4 - Make sure instance level role names are uniq

Compare with previous version

requested review from @DylanGriffith

removed review request for @DylanGriffith

added 1 commit

48629cb6 - Validate member role name uniqnuess only on create

Compare with previous version

mentioned in issue #443780 (closed)

requested review from @DylanGriffith

approved this merge request

added databaseapproved label and removed databasereviewed label

resolved all threads

I can't merge this. It requires Authorization approval. @alexbuijs could you please take that.

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

approved this merge request

removed review request for @alexbuijs

requested review from @DylanGriffith

added Data WarehouseNot Impacted label and removed Data WarehouseImpact Check label

resolved all threads

@rkadam3 as the db is approved, could you please do the final check & merge?

requested review from @rkadam3

approved this merge request

resolved all threads

enabled an automatic merge when the pipeline for 457ff24c succeeds

merged

Hello @jarka

The database team is looking for ways to improve the database review process and we would love your help!

If you'd be open to someone on the database team reaching out to you for a chat, or if you'd like to leave some feedback asynchronously, just post a reply to this comment mentioning:

@gitlab-org/database-team

And someone will be by shortly!

Thanks for your help!

This message was generated automatically. You're welcome to improve it.

mentioned in commit b0f11581

added workflowstaging-canary label and removed workflowready for development label

added workflowcanary label and removed workflowstaging-canary label

added workflowstaging label and removed workflowcanary label

added workflowproduction label and removed workflowstaging label

added workflowpost-deploy-db-staging label and removed workflowproduction label

mentioned in merge request !146293 (merged)

Even after scripts/regenerate-schema I see git-diff in db/structure.sql file:

diff --git db/structure.sql db/structure.sql
index ad951b7af4ae..4ff89e76becf 100644
--- db/structure.sql
+++ db/structure.sql
@@ -25647,12 +25647,12 @@ CREATE INDEX index_member_approval_on_requested_by_id ON member_approvals USING

 CREATE INDEX index_member_approval_on_reviewed_by_id ON member_approvals USING btree (reviewed_by_id);

-CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL);
-
 CREATE INDEX index_member_approvals_on_member_role_id ON member_approvals USING btree (member_role_id);

 CREATE INDEX index_member_approvals_on_user_id ON member_approvals USING btree (user_id);

+CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL);
+
 CREATE INDEX index_member_roles_on_namespace_id ON member_roles USING btree (namespace_id);

 CREATE UNIQUE INDEX index_member_roles_on_namespace_id_name_unique ON member_roles USING btree (namespace_id, name) WHERE (namespace_id IS NOT NULL);

I wondering whether it is something with my local environment or whether we should update that file on the master branch?

I get the same diff after gdk update. We have to update strcuture.sql, the order should alphabetical by index name.

It's strange that https://gitlab.com/gitlab-org/gitlab/-/jobs/6311410967 did not fail.

Ah, the problem is actually caused by !144882 (merged), and db:check-migrations did fail for it (though it as a job that is allowed to fail).

!146959 (merged) to fix.

mentioned in commit 2bc9dad9

mentioned in merge request !146959 (merged)

mentioned in commit 903ff37a

mentioned in commit b8a0b426

mentioned in merge request !144882 (merged)

mentioned in commit 29c9b06c

mentioned in merge request !147048 (merged)

mentioned in merge request kubitus-project/kubitus-installer!2869 (merged)

mentioned in merge request !151172 (merged)

added releasedpublished label

added pipelinetier-3 label

mentioned in merge request gitlab-com/www-gitlab-com!135684 (merged)

25517	25517
25518	25518	CREATE INDEX index_member_approval_on_reviewed_by_id ON member_approvals USING btree (reviewed_by_id);
25519	25519
	25520	CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL);
Bogdan Denkovych @bdenkovych · 11 months ago Maintainer Even after `scripts/regenerate-schema` I see git-diff in `db/structure.sql` file: diff --git db/structure.sql db/structure.sql index ad951b7af4ae..4ff89e76becf 100644 --- db/structure.sql +++ db/structure.sql @@ -25647,12 +25647,12 @@ CREATE INDEX index_member_approval_on_requested_by_id ON member_approvals USING CREATE INDEX index_member_approval_on_reviewed_by_id ON member_approvals USING btree (reviewed_by_id); -CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL); - CREATE INDEX index_member_approvals_on_member_role_id ON member_approvals USING btree (member_role_id); CREATE INDEX index_member_approvals_on_user_id ON member_approvals USING btree (user_id); +CREATE UNIQUE INDEX index_member_roles_on_name_unique ON member_roles USING btree (name) WHERE (namespace_id IS NULL); + CREATE INDEX index_member_roles_on_namespace_id ON member_roles USING btree (namespace_id); CREATE UNIQUE INDEX index_member_roles_on_namespace_id_name_unique ON member_roles USING btree (namespace_id, name) WHERE (namespace_id IS NOT NULL); I wondering whether it is something with my local environment or whether we should update that file on the `master` branch? Krasimir Angelov @krasio · 11 months ago Maintainer I get the same diff after `gdk update`. We have to update `strcuture.sql`, the order should alphabetical by index name. Krasimir Angelov @krasio · 11 months ago Maintainer It's strange that https://gitlab.com/gitlab-org/gitlab/-/jobs/6311410967 did not fail. Krasimir Angelov @krasio · 11 months ago Maintainer Ah, the problem is actually caused by !144882 (merged), and `db:check-migrations` did fail for it (though it as a job that is allowed to fail). Edited 11 months ago by Krasimir Angelov Krasimir Angelov @krasio · 11 months ago Maintainer !146959 (merged) to fix. Please register or sign in to reply

Validate uniqnuess of member role name

What does this MR do and why?

Database queries

MR acceptance checklist

Screenshots or screen recordings

How to set up and validate locally

Activity

Reviewer roulette

Database migrations (on the main database)

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Migration: 20240221134504 - AddNameUniqueIndexToMemberRoles

Other information

Database migrations (on the ci database)

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Migration: 20240221134504 - AddNameUniqueIndexToMemberRoles

Other information

E2E Test Result Summary

Validate uniqnuess of member role name

What does this MR do and why?

Database queries

MR acceptance checklist

Screenshots or screen recordings

How to set up and validate locally

Merge request reports

Activity

Reviewer roulette

Database migrations (on the main database)

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Migration: 20240221134504 - AddNameUniqueIndexToMemberRoles

Other information

Database migrations (on the ci database)

Migration: 20240210104125 - EnsureMemberRolesNamesUniq

Migration: 20240221134504 - AddNameUniqueIndexToMemberRoles

Other information

E2E Test Result Summary