backupccl: Support RESTORE SYSTEM USERS from a backup #71542

gh-casper · 2021-10-13T20:11:47Z

Support a new variant of RESTORE that recreates system users that don't exist in current cluster from a backup that contains system.users and also grant roles for these users. Example invocation: RESTORE SYSTEM USERS FROM 'nodelocal://foo/1';

Similar with full cluster restore, we firstly restore a temp system database which contains system.users and system.role_members into the restoring cluster and insert users and roles into the current system table from the temp system table.

Fixes: #45358

Release note (sql change): A special flavor of RESTORE, RESTORE SYSTEM USERS FROM ..., is added to support restoring system users from a backup. When executed, the statement recreates those users which are in a backup of system.users but do not currently exist (ignoring those who do) and re-grant roles for users if the backup contains system.role_members.

cockroach-teamcity · 2021-10-13T20:11:54Z

This change is

shermanCRL · 2021-10-13T21:11:41Z

cc @kathancox there is new syntax here. Do we need to open a Docs issue (this will be for v22.1).

kathancox · 2021-10-15T12:55:30Z

cc @kathancox there is new syntax here. Do we need to open a Docs issue (this will be for v22.1).

This will automatically come through as a release notes docs issue for 22.1, so I don't think it is necessary to open one manually

adityamaru · 2021-10-26T09:07:41Z

Hey @gh-casper apologies for the delay on this review, I will take a look at this today.

adityamaru · 2021-10-28T09:53:21Z

This is looking great, I haven't looked at the tests yet but wanted to get a first round of comments out. I like that we're able to use the tempSystemDB. I'm adding another reviewer as well since this is a biggish change and i wouldnt mind another set of eyes.

pkg/sql/sem/tree/backup.go

pkg/sql/sem/tree/grant.go

pkg/jobs/jobspb/jobs.proto

pkg/ccl/backupccl/targets.go

adityamaru · 2021-10-28T09:32:58Z

pkg/ccl/backupccl/restore_planning.go

-		tempSysDBID, err = catalogkv.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec)
-		if err != nil {
-			return nil, err
+		if nextID > descpb.ID(maxDescIDInBackup) {


While I think this is correct, this change makes me a little nervous. Thinking through the cases:

In a full cluster restore we expect the restoring cluster to have no user defined objects prior to the restore. So nextID should be <= maxDescIDInBackup and all subsequent descriptors will be written at maxDescIDInBackup+1.

In a system users restore the restoring cluster could have any number of user defined objects. Further, in a future where system tables do not have static IDs, maxDescIDInBackup could also be any ID. Is it possible for us to get into a situation where nextID returns 55, maxDescIDInBackup is set to 100 because that was the dynamic system table ID in the backing up cluster. In that case we would leave IDs 56-100 unused since we would see the generator with the ID 101? In todays world of static system table IDs I think this works fine but I'm wondering if there is some way we can future proof this.

What if we let cluster restores remain the way they were. For system user restore we just use whatever the next valid ID in the restoring cluster is for the tempSysDBID, and don't get into the business of reseeding the generator?

Makes sense.

pkg/ccl/backupccl/restore_job.go

adityamaru

Sorry for the delay! LGTM mod confirmation that role options don't have to be restored.

adityamaru · 2021-11-15T13:48:33Z

pkg/ccl/backupccl/restore_planning.go

+			// Restore the key which generates descriptor IDs.
+			if err = p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+				b := txn.NewBatch()
+					// write int64 values.


nit: any reason to replace the N.B.? It provides a little more context for future readers.

adityamaru · 2021-11-15T14:01:11Z

pkg/ccl/backupccl/restore_planning.go

+				return nil, err
+			}
+		} else if restoreSystemUsers {
+			tempSysDBID, err = catalogkv.GenerateUniqueDescID(ctx, p.ExecCfg().DB, p.ExecCfg().Codec)


nit: can we add a small comment that we're just using the next available ID for the temp system ID in the case of system users restore.

adityamaru · 2021-11-15T14:15:37Z

pkg/ccl/backupccl/targets.go

+					systemTables = append(systemTables, desc)
+				case systemschema.RoleMembersTable.GetName():
+					systemTables = append(systemTables, desc)
+					// TODO(casper): should we handle role_options table?


@RichardJCai can we get your take on this. If we only restore users, and role members without role options is that okay? My understanding of what information the role options table contains is very shallow.

I think ideally we should restore entries for the users we're restoring in the system.role_options table. Role options are scoped per user and in a sense does contain information about a user.

I would add it to the restore and it seems like it wouldn't really be difficult to do so right?

Support a new variant of RESTORE that recreates system users that don't exist in current cluster from a backup that contains system.users and also grant roles for these users. Example invocation: RESTORE SYSTEM USERS FROM 'nodelocal://foo/1'; Similar with full cluster restore, we firstly restore a temp system database which contains system.users and system.role_members into the restoring cluster and insert users and roles into the current system table from the temp system table. Fixes: cockroachdb#45358 Release note (sql change): A special flavor of RESTORE, RESTORE SYSTEM USERS FROM ..., is added to support restoring system users from a backup. When executed, the statement recreates those users which are in a backup of system.users but do not currently exist (ignoring those who do) and re-grant roles for users if the backup contains system.role_members.

gh-casper · 2022-02-18T16:53:27Z

bors r+

craig · 2022-02-18T22:36:35Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-02-19T00:32:44Z

Build succeeded:

GitHub CI (Cockroach)

gh-casper requested review from adityamaru, RichardJCai and a team October 13, 2021 20:11

gh-casper force-pushed the fr-system-users branch from f6988bf to 761c18c Compare October 14, 2021 19:09

adityamaru suggested changes Oct 28, 2021

View reviewed changes

adityamaru requested review from adityamaru and dt and removed request for a team October 28, 2021 09:54

gh-casper force-pushed the fr-system-users branch from 761c18c to 4536697 Compare November 6, 2021 22:05

adityamaru approved these changes Nov 15, 2021

View reviewed changes

gh-casper force-pushed the fr-system-users branch from 4536697 to 1b730e7 Compare February 17, 2022 23:16

gh-casper requested a review from a team as a code owner February 17, 2022 23:16

gh-casper requested review from HonoreDB and removed request for a team February 17, 2022 23:16

gh-casper force-pushed the fr-system-users branch from 1b730e7 to 6f695f3 Compare February 18, 2022 08:11

craig bot merged commit 255c1fb into cockroachdb:master Feb 19, 2022

cockroach-teamcity mentioned this pull request Feb 19, 2022

backupccl: Support RESTORE SYSTEM USERS from a backup cockroachdb/docs#13073

Open

rafiss mentioned this pull request Dec 21, 2022

restore: RESTORE SYSTEM USERS does not restore role_options #94107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

backupccl: Support RESTORE SYSTEM USERS from a backup #71542

backupccl: Support RESTORE SYSTEM USERS from a backup #71542

gh-casper commented Oct 13, 2021

cockroach-teamcity commented Oct 13, 2021

shermanCRL commented Oct 13, 2021

kathancox commented Oct 15, 2021

adityamaru commented Oct 26, 2021

adityamaru commented Oct 28, 2021 •

edited

Loading

adityamaru Oct 28, 2021

gh-casper Nov 6, 2021

adityamaru left a comment

adityamaru Nov 15, 2021

adityamaru Nov 15, 2021

adityamaru Nov 15, 2021

RichardJCai Nov 15, 2021

gh-casper commented Feb 18, 2022

craig bot commented Feb 18, 2022

craig bot commented Feb 19, 2022

backupccl: Support RESTORE SYSTEM USERS from a backup #71542

backupccl: Support RESTORE SYSTEM USERS from a backup #71542

Conversation

gh-casper commented Oct 13, 2021

cockroach-teamcity commented Oct 13, 2021

shermanCRL commented Oct 13, 2021

kathancox commented Oct 15, 2021

adityamaru commented Oct 26, 2021

adityamaru commented Oct 28, 2021 • edited Loading

adityamaru Oct 28, 2021

Choose a reason for hiding this comment

gh-casper Nov 6, 2021

Choose a reason for hiding this comment

adityamaru left a comment

Choose a reason for hiding this comment

adityamaru Nov 15, 2021

Choose a reason for hiding this comment

adityamaru Nov 15, 2021

Choose a reason for hiding this comment

adityamaru Nov 15, 2021

Choose a reason for hiding this comment

RichardJCai Nov 15, 2021

Choose a reason for hiding this comment

gh-casper commented Feb 18, 2022

craig bot commented Feb 18, 2022

craig bot commented Feb 19, 2022

adityamaru commented Oct 28, 2021 •

edited

Loading