sql/catalog, backupccl: support offline descriptors, add all new restored descriptors in offline state

[thoszhang]

Closes #53375.

---

backupccl: fix bug in writing database schema map

Release justification: Bug fixes to new functionality

Release note (bug fix): Fixes a bug introduced in a 20.2 alpha release
where incorrect caching could incur extra writes to the store during
RESTORE on user-defined schemas.

---

backupccl: reset Version and ModificationTime on new descriptors

The Version and ModificationTime fields are now reset to 1 and the
empty timestamp, respectively, on new descriptors created in RESTORE, to
make them behave like other newly created descriptors.

Release justification: Low risk, high benefit changes to existing
functionality

Release note: None

---

sql/catalog: add descriptor API support for offline state

This change adds methods for the offline state to the descriptor
interfaces and makes the state-related API more uniform.

Release justification: Low risk, high benefit changes to existing
functionality

Release note: None

---

sql/catalog: handle dropped and offline descriptors in lookups

This change updates the name resolution and descriptor lookup
implementations to respect the state of dropped and offline descriptors:
All lookups now read the IncludeOffline and IncludeDropped flags in
tree.CommonLookupFlags and filter results accordingly.

Tests in the context of RESTORE are forthcoming in the following commit.

Release justification: Bug fixes and low-risk updates to new
functionality

Release note: None

---

backupccl: add all new restored descriptors in offline state

This change updates the RESTORE job to add all new descriptors prior to
restoring data in the OFFLINE state, and update their state to PUBLIC
after data has been restored. This makes all descriptors behave like
tables.

Release justification: Low risk, high benefit changes to existing
functionality (for database descriptors; other changes apply only to new
functionality)

Release note (sql change): Databases being restored will now be in the
offline state, invisible to users, until the data has been restored.
This is the same as the existing behavior for restored tables. (This
change is also applied to enums and user-defined schemas being restored,
which is a change relative to only the 20.2 alpha releases.)

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 1 of 1 files at r1, 6 of 6 files at r2, 15 of 15 files at r3, 6 of 6 files at r4, 2 of 4 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @lucy-zhang, and @pbardea)

---

pkg/ccl/backupccl/backup_test.go, line 6093 at r5 (raw file):

Quoted 7 lines of code…

	// Protects beforePublishingNotification, continueNotification.
	var mu syncutil.Mutex
	// Closed when the restore job has reached the point right before publishing
	// descriptors.
	beforePublishingNotification := make(chan struct{})
	// Closed when we're ready to resume with the restore.
	continueNotification := make(chan struct{})

total nit: I usually like to write this as an anonymous struct to make it clearer what the mutex protects

var state = struct {
    syncutil.Mutex
    beforePublishingNotification chan struct{}
    continueNotification chan struct{}   
}

---

pkg/ccl/backupccl/backup_test.go, line 6131 at r5 (raw file):

		for _, server := range tc.Servers {
			registry := server.JobRegistry().(*jobs.Registry)
			registry.TestingResumerCreationKnobs = map[jobspb.Type]func(raw jobs.Resumer) jobs.Resumer{

it feels lucky that this isn't racy.

---

pkg/ccl/backupccl/restore_job.go, line 1498 at r5 (raw file):

	// TODO (lucy): It's possible for tables and types to be added under the
	// schema after it becomes public. To account for this, we need to refrain
	// from dropping the schema if it has children, like we do for databases.

What's the scenario where we fail and want to rollback after we've made the descriptors public?

---

pkg/ccl/backupccl/restore_planning.go, line 717 at r2 (raw file):

// rewriteDatabaseDescs rewrites all ID's in the input slice of
// DatabaseDescriptors using the input ID rewrite mapping.

nit: comment about the semantics of and reason for resetVersion

---

pkg/ccl/backupccl/restore_planning.go, line 1507 at r2 (raw file):

		return err
	}
	if err := rewriteDatabaseDescs(databases, descriptorRewrites, false /* resetVersion */); err != nil {

The reason this is false isn't immediately obvious to me. Could you add some commentary?

---

pkg/ccl/storageccl/key_rewriter.go, line 70 at r2 (raw file):

Quoted 4 lines of code…

		// Use GetTable() (which does not check and set the ModificationTime on the
		// descriptor) because these table descriptors are only used for their index
		// information in the KeyRewriter.
		table := desc.GetTable()

Is this really necessary? Isn't it the case that if the version is <= 1 we don't care about the modification time being set? When is the modification time unset and the version not 0 or 1?

---

pkg/sql/catalog/catalogkv/physical_accessor.go, line 81 at r4 (raw file):

	if inUnacceptableState :=
		(db.Dropped() && !flags.IncludeDropped) ||
			(db.Offline() && !flags.IncludeOffline); inUnacceptableState {

nit: I could see pulling this into a helperfunc descriptorInAcceptableState(desc catalog.Descriptor, flags tree.CommonLookupFlags) bool so this and elsewhere could be:

if !descriptorInAcceptableState(db, flag.CommonLookupFlags) { ... }

---

pkg/sql/catalog/catalogkv/physical_accessor.go, line 320 at r4 (raw file):

		// return a descriptor during draining.
		// TODO (lucy): Is this check actually a good idea? If so, we should extend
		// it to all the other descriptors.

So yeah, what's the deal here?

---

pkg/sql/catalog/descpb/descriptor.go, line 98 at r3 (raw file):

// TODO (lucy): Does this method belong on Descriptor? This state does matter
// for descriptor leasing, but arguably we should be upwrapping the descriptor
// to get it.

Maybe nix this TODO and note when it makes sense to use this?

[pbardea]

Reviewed 1 of 1 files at r1, 6 of 6 files at r2, 15 of 15 files at r3, 6 of 6 files at r4, 4 of 4 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

pkg/ccl/backupccl/restore_job.go, line 1498 at r5 (raw file):

Previously, ajwerner wrote…

	// TODO (lucy): It's possible for tables and types to be added under the
	// schema after it becomes public. To account for this, we need to refrain
	// from dropping the schema if it has children, like we do for databases.

What's the scenario where we fail and want to rollback after we've made the descriptors public?

Cluster restore will mark all tables as public after restoring all of the user data, but before restoring the system data. This is unfortunate since if the job were to fail restoring the system data, we'd need to clean up these public tables. We should really only be marking the temporary system tables as public before the end of the job.

More generally, the Resumer could stop at any point during the job, and could enter the OnFailOrCancel hook. That being said, since the job needs to detect that it has been canceled I can only imagine this happening if:

1. the coordinator crashes after marking the tables public, but before succeeding the job
2. the job then gets canceled while it's trying to be adopted

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

---

pkg/ccl/backupccl/restore_job.go, line 1498 at r5 (raw file):

That being said, since the job needs to detect that it has been canceled I can only imagine this happening if:

This isn't how it works anymore. Since 20.1, the job's context will get canceled

[thoszhang]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @pbardea)

---

pkg/ccl/backupccl/backup_test.go, line 6093 at r5 (raw file):

Previously, ajwerner wrote…

	// Protects beforePublishingNotification, continueNotification.
	var mu syncutil.Mutex
	// Closed when the restore job has reached the point right before publishing
	// descriptors.
	beforePublishingNotification := make(chan struct{})
	// Closed when we're ready to resume with the restore.
	continueNotification := make(chan struct{})

total nit: I usually like to write this as an anonymous struct to make it clearer what the mutex protects

var state = struct {
    syncutil.Mutex
    beforePublishingNotification chan struct{}
    continueNotification chan struct{}   
}

Done.

---

pkg/ccl/backupccl/backup_test.go, line 6131 at r5 (raw file):

Previously, ajwerner wrote…

it feels lucky that this isn't racy.

Is this more racy than other testing knobs we have?

---

pkg/ccl/backupccl/restore_job.go, line 1498 at r5 (raw file):

Previously, ajwerner wrote…

That being said, since the job needs to detect that it has been canceled I can only imagine this happening if:

This isn't how it works anymore. Since 20.1, the job's context will get canceled

Yeah, what Paul said re: restoring the system data later. (With that said, why can't we mark the user tables and system tables as public at the same time? Not saying we should do that now.)

---

pkg/ccl/backupccl/restore_planning.go, line 717 at r2 (raw file):

Previously, ajwerner wrote…

nit: comment about the semantics of and reason for resetVersion

I'm removing this argument. My original reasoning was that if we were in the mixed-version state, we couldn't be restoring backups from a post-finalization state (but, actually, I'm not even sure if this is guaranteed to be true), so the version would be 0 and we'd want to keep it that way. But I think it's actually fine to unconditionally set the new version to 1 even in the mixed-version state. I can't think of any obstacles to correctly leasing the descriptor.

---

pkg/ccl/backupccl/restore_planning.go, line 1507 at r2 (raw file):

Previously, ajwerner wrote…

The reason this is false isn't immediately obvious to me. Could you add some commentary?

FWIW, this was an arbitrary choice since the only reason to call rewrite...() while planning is to detect errors in cross-references when assigning IDs.

---

pkg/ccl/storageccl/key_rewriter.go, line 70 at r2 (raw file):

Previously, ajwerner wrote…

		// Use GetTable() (which does not check and set the ModificationTime on the
		// descriptor) because these table descriptors are only used for their index
		// information in the KeyRewriter.
		table := desc.GetTable()

Is this really necessary? Isn't it the case that if the version is <= 1 we don't care about the modification time being set? When is the modification time unset and the version not 0 or 1?

Yeah, you're right. I originally added this prior to resetting the descriptor versions and forgot that we allow an empty ModificationTime on version 1.

---

pkg/sql/catalog/catalogkv/physical_accessor.go, line 81 at r4 (raw file):

Previously, ajwerner wrote…

	if inUnacceptableState :=
		(db.Dropped() && !flags.IncludeDropped) ||
			(db.Offline() && !flags.IncludeOffline); inUnacceptableState {

nit: I could see pulling this into a helperfunc descriptorInAcceptableState(desc catalog.Descriptor, flags tree.CommonLookupFlags) bool so this and elsewhere could be:

if !descriptorInAcceptableState(db, flag.CommonLookupFlags) { ... }

Done.

---

pkg/sql/catalog/catalogkv/physical_accessor.go, line 320 at r4 (raw file):

Previously, ajwerner wrote…

So yeah, what's the deal here?

I think this is legit and we should do it for all the descriptor types. I'm going to do this in a separate PR, though, if you don't mind. I wrote a test for the table case, so I'll just add changes on top of that (#54213).

---

pkg/sql/catalog/descpb/descriptor.go, line 98 at r3 (raw file):

Previously, ajwerner wrote…

// TODO (lucy): Does this method belong on Descriptor? This state does matter
// for descriptor leasing, but arguably we should be upwrapping the descriptor
// to get it.

Maybe nix this TODO and note when it makes sense to use this?

Done. I put the doc comment on GetDescriptorMetadata.

[ajwerner]

Reviewed 30 of 30 files at r6, 4 of 4 files at r7, 15 of 15 files at r8, 7 of 7 files at r9.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/ccl/backupccl/backup_test.go, line 6131 at r5 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Is this more racy than other testing knobs we have?

fair enough

[pbardea]

LGTM

[pbardea]

A small comment here about how we need this check since 20.1 nodes don't update databases from OFFLINE to PUBLIC would be useful.

[thoszhang]

Done.

[thoszhang]

TFTRs

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[thoszhang]

I'll wait until #53928 gets backported to backport this. cc @pbardea