sql,backupccl: set system.role_members ID columns to NOT NULL

[andyyang890]

This patch sets the newly added role_id and member_id columns in
system.role_members to be NOT NULL. It also changes the RESTORE
logic to be able to handle the case when restoring from a backup where
the system.role_members table did not have the columns.

Part of #87079

Release note: None

[cockroach-teamcity]

This change is 

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890 and @rhu713)

---

pkg/ccl/backupccl/restore_job.go line 2858 at r1 (raw file):

		}

		userIDMap := make(map[tree.DString]oid.Oid)

nit: can you make this use tree.DOid instead of oid.Oid? i think it's better to treat these all as datums

---

pkg/ccl/backupccl/system_schema.go line 252 at r1 (raw file):

	txn *kv.Txn,
	systemTableName, tempTableName string,
) error {

can you make this a named return parameter, retErr? (see below comment)

---

pkg/ccl/backupccl/system_schema.go line 283 at r1 (raw file):

	if err != nil {
		return err
	}

there should be a call like this here:

	defer func() { retErr = errors.CombineErrors(retErr, it.Close()) }()

actually, we'd want the same fix to be in roleOptionsRestoreFunc. the reason is that some errors are only returned when the iterator is closed, and this is important for correct error handling.

---

pkg/ccl/backupccl/system_schema.go line 318 at r1 (raw file):

		}

		restoreQuery := fmt.Sprintf(`INSERT INTO system.%s ("role", "member", "isAdmin", role_id, member_id) VALUES ($1, $2, $3, $4, $5)`, systemTableName)

i think you can avoid the need for the ids map, if you use an update ... from statement: https://www.cockroachlabs.com/docs/stable/update.html#update-using-values-from-a-different-table

also, it feels like it would be cleaner if the INSERT was done in a different loop after the one that scans through it. i don't think we are allowed to execute other statements while the internal executor still has an iterator open.

[andyyang890]

Added another test for restoring from a backup that did not have the ID columns!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss and @rhu713)

---

pkg/ccl/backupccl/restore_job.go line 2858 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: can you make this use tree.DOid instead of oid.Oid? i think it's better to treat these all as datums

Done.

---

pkg/ccl/backupccl/system_schema.go line 252 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you make this a named return parameter, retErr? (see below comment)

Done.

---

pkg/ccl/backupccl/system_schema.go line 283 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

there should be a call like this here:

	defer func() { retErr = errors.CombineErrors(retErr, it.Close()) }()

actually, we'd want the same fix to be in roleOptionsRestoreFunc. the reason is that some errors are only returned when the iterator is closed, and this is important for correct error handling.

Done.

---

pkg/ccl/backupccl/system_schema.go line 318 at r1 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i think you can avoid the need for the ids map, if you use an update ... from statement: https://www.cockroachlabs.com/docs/stable/update.html#update-using-values-from-a-different-table

also, it feels like it would be cleaner if the INSERT was done in a different loop after the one that scans through it. i don't think we are allowed to execute other statements while the internal executor still has an iterator open.

I've moved it into a separate loop after the iterator loop. I think you can't actually use UPDATE ... FROM here because the new table starts empty and we are inserting rows into it.

[stevendanna]

Thanks for fixing this up and adding the cross-version tests. I left a couple of questions.

[andyyang890]

Addressed comments and ready for another review!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rafiss, @rhu713, and @stevendanna)

[stevendanna]

Thanks for adding the backup/restore tests. Overall the backup/restore parts look reasonable to me.

[rafiss]

lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @rhu713 and @stevendanna)

[andyyang890]

TFTR!

bors r=stevendanna,rafiss

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)