ccl/backupccl: upgrade by-name sequence reference to by-ID during restore

[Xiang-Gu]

In 20.2 and prior, sequences are referenced by-name. It was later
changed to reference-by-ID to enable things like
ALTER SEQUENCE ... RENAME ....

But if a backup is taken in 20.2 and prior, and then the backup is
restored in a newer binary version (where sequence references should
be by-ID), we will need to also be able to upgrade those sequence
references from by-name to by-ID.

fixes: #60942

Release note: None

[cockroach-teamcity]

This change is 

[Xiang-Gu]

For my own tracking: this outstanding PR needs some revisions -- notably, introducing a new change into runRestoreChanges and implementing that in tableDescriptorBuilder (in the tabledescs package). This implementation will use utilities in seqexpr to achieve the upgrade-seq-reference work.

But this is currently infeasible because seqexpr unfortunately depends on tabledesc, which it shouldn't. #81856 will be fixing this.

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

[ajwerner]

#82433 turned out to require a bit more plumbing.

[Xiang-Gu]

The abovementioned revision is done. Furthermore, I added a test for the utility functions added in seqexpr. Ready for a look.

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

[fqazi]

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

---

pkg/sql/catalog/seqexpr/sequence.go line 273 at r2 (raw file):

//		tableName = 't2'
//		return = non-nil error (because neither 'sc1.t' nor 'sc2.t' matches 't2', that is, 0 valid candidate left)
func findUniqueBestMatchingForTableName(

Will there be any case with ambiguities that are valid where the restore would fail here?

[Xiang-Gu]

improved the logic to be more robust and made the change slimmer.

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

---

pkg/sql/catalog/seqexpr/sequence.go line 273 at r2 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Will there be any case with ambiguities that are valid where the restore would fail here?

I've re-worked this function because this (pre-revision) version has an assumption that the final result, if exists, fromallTableNamesByID ">=" the targetTableName. (">=" here means more table name parts).

I restore some other changes (loadSQLDescFromBackup) which I think it's hacky. As a result, one test will surface a case where allTableNamesByID contains a name {"public.s"} while the targetTableName is "test.public.s", which breaks our assumption.

This assumption is not justified so the new version of this function's invariant is that the return name will make sure match each part of targetTableName, if that part exists in both names. Still, we need to error out if there are 0 or >1 such "valid" results.

[postamar]

This is coming along well. Descriptor upgrades are always a bit of an annoying grind so don't despair.

[postamar]

suggestion: consider using an nstree.Catalog instead of nstree.Map for descLookup, this way you can do

err := b.RunRestoreChanges(descLookup.LookupDescriptorEntry)

[postamar]

I don't see any references to this constant anywhere 🤷

[postamar]

I just noticed that this filename had a typo: please rename it from post_derserialization_changes.go to post_deserialization_changes.go if you can.

[postamar]

Shouldn't we check that rel.DependedOnBy[i].ID is non-zero here, at least?

[postamar]

nit: it seems like this function could be inlined at its only call site with no loss of clarity.

[postamar]

I'm wondering if it wouldn't be simpler to have this function take a reverse index of type map[string]descpb.ID instead of allUsedSeqNames map[descpb.ID]*tree.TableName. This would move all of the logic explained in the commentary of findUniqueBestMatchingForTableName to the generation of the reverse index. Lookups could then be done directly via the seqIdentifier.SeqName. Ambiguous strings could simply have no entry (or map to a sentinel value like 0).

[postamar]

I'd like @ajwerner 's opinion on whether it's safe to modify the view query like this or not, just to make sure. Personally I don't see why not.

[postamar]

Can you add tests to cover these cases in the commentary? In particular I'd like to see a rename fail because there's a sequence that's still referenced by name somewhere because the name is ambiguous, preventing it from being upgraded to a reference by ID.

[fqazi]

Reviewed 1 of 6 files at r1, 2 of 9 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @stevendanna, and @Xiang-Gu)

---

pkg/sql/catalog/seqexpr/sequence.go line 289 at r3 (raw file):

		cs, ts := candidateTableName.Schema(), targetTableName.Schema()
		cdb, tdb := candidateTableName.Catalog(), targetTableName.Catalog()
		if ct != "" && tt != "" && ct != tt {

Will the table name ever be empty?

[Xiang-Gu]

Thanks for your feedback. I've addressed them and it's RFAL again.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @postamar, @stevendanna, and @Xiang-Gu)

---

pkg/ccl/backupccl/restore_planning.go line 884 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

suggestion: consider using an nstree.Catalog instead of nstree.Map for descLookup, this way you can do

err := b.RunRestoreChanges(descLookup.LookupDescriptorEntry)

done. Thanks for the suggestion!

---

pkg/sql/catalog/seqexpr/sequence.go line 201 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

I'm wondering if it wouldn't be simpler to have this function take a reverse index of type map[string]descpb.ID instead of allUsedSeqNames map[descpb.ID]*tree.TableName. This would move all of the logic explained in the commentary of findUniqueBestMatchingForTableName to the generation of the reverse index. Lookups could then be done directly via the seqIdentifier.SeqName. Ambiguous strings could simply have no entry (or map to a sentinel value like 0).

Done. I've refactored the code as your suggested.

---

pkg/sql/catalog/seqexpr/sequence.go line 276 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Can you add tests to cover these cases in the commentary? In particular I'd like to see a rename fail because there's a sequence that's still referenced by name somewhere because the name is ambiguous, preventing it from being upgraded to a reference by ID.

Done. I've moved all those into test cases.

---

pkg/sql/catalog/seqexpr/sequence.go line 289 at r3 (raw file):

Previously, fqazi (Faizan Qazi) wrote…

Will the table name ever be empty?

No they won't be.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 811 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't we check that rel.DependedOnBy[i].ID is non-zero here, at least?

such a reference shouldn't exist but I agree that it's good to be a bit "paranoid". I added a check here

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 842 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: it seems like this function could be inlined at its only call site with no loss of clarity.

done

Code quote:

maybeUpgradeSequenceReferenceInRelation

---

pkg/sql/catalog/post_derserialization_changes.go line 94 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

I don't see any references to this constant anywhere 🤷

removed. Thanks for the catch -- it probably comes in bc I was not careful about merging conflict resolution during rebase.

Code quote:

UpgradedSequenceReference indicates that

---

pkg/sql/catalog/post_derserialization_changes.go line 99 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

I just noticed that this filename had a typo: please rename it from post_derserialization_changes.go to post_deserialization_changes.go if you can.

done

[ajwerner]

Reviewed 1 of 6 files at r1, 1 of 7 files at r3, 5 of 11 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @postamar, @stevendanna, and @Xiang-Gu)

---

pkg/sql/catalog/seqexpr/sequence.go line 197 at r4 (raw file):

// Precondition: `seqByNameRefToID` is assumed to contain an entry for every
// 							 by-name seq reference in expr. Such a mapping can be obtained from
//							 `SeqNameToIDMappingInExpr`.

nit: weird indentation. I think you're trying to align with Precondition but using tabs instead of spaces.

Code quote:

// 							 by-name seq reference in expr. Such a mapping can be obtained from
//							 `SeqNameToIDMappingInExpr`.

---

pkg/sql/catalog/seqexpr/sequence.go line 203 at r4 (raw file):

	parsedExpr, err := parser.ParseExpr(*expr)
	if err != nil {
		return hasUpgraded, err

nit: I prefer return false, err here

---

pkg/sql/catalog/seqexpr/sequence.go line 209 at r4 (raw file):

	newExpr, err := ReplaceSequenceNamesWithIDs(parsedExpr, seqByNameRefToID)
	if err != nil {
		return hasUpgraded, err

nit: I prefer return false, err here

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 851 at r4 (raw file):

			if err != nil {
				return hasUpgraded, err
			}

given you always call these as a pair and seqexpr.UpgradeSequenceReferenceInExpr seems pretty low-level, can we just change it so the only exported function is one that calls this pair?

Code quote:

			seqNameToIDMappingInExpr, err := seqexpr.SeqNameToIDMappingInExpr(*col.DefaultExpr, usedSequenceIDToNames)
			if err != nil {
				return hasUpgraded, err
			}
			hasUpgradedInDefault, err := seqexpr.UpgradeSequenceReferenceInExpr(col.DefaultExpr, seqNameToIDMappingInExpr)
			if err != nil {
				return hasUpgraded, err
			}

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 857 at r4 (raw file):

		// Upgrade sequence reference in ON UPDATE expression, if any.
		if col.HasOnUpdate() {
			seqNameToIDMappingInExpr, err := seqexpr.SeqNameToIDMappingInExpr(*col.DefaultExpr, usedSequenceIDToNames)

Shouldn't this be *col.OnUpdateExpr?

Code quote:

			seqNameToIDMappingInExpr, err := seqexpr.SeqNameToIDMappingInExpr(*col.DefaultExpr, usedSequenceIDToNames)
			if err != nil {
				return hasUpgraded, err
			}
			hasUpgradedInOnUpdate, err := seqexpr.UpgradeSequenceReferenceInExpr(col.OnUpdateExpr, seqNameToIDMappingInExpr)

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 942 at r4 (raw file):

			continue
		}
		tableDesc, ok := d.(catalog.TableDescriptor)

nit: you don't need the first if. nil will fail the type assertion

Code quote:

		if d == nil {
			continue
		}
		tableDesc, ok := d.(catalog.TableDescriptor)

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 950 at r4 (raw file):

		dbName := ""
		d = descLookupFn(tableDesc.GetParentID())
		if d != nil {

nit: you don't need the outer if

Code quote:

		if d != nil {
			if dbDesc, ok := d.(catalog.DatabaseDescriptor); ok {

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 969 at r4 (raw file):

				scName = tree.PublicSchema
			}
		}

if neither of these are true, then what? Should we return an error?

[ajwerner]

Also, you need to regenerate the bazel files

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi, @postamar, @stevendanna, and @Xiang-Gu)

[Xiang-Gu]

Thank you @ajwerner for your review. Per your feedback, I've
1). made only one exported method from seqexpr -- UpgradeSequenceReferenceInExpr
2). added more unit tests for UpgradeSequenceReferenceInExpr

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @fqazi, @postamar, @stevendanna, and @Xiang-Gu)

---

pkg/sql/catalog/seqexpr/sequence.go line 197 at r4 (raw file):

Previously, ajwerner wrote…

nit: weird indentation. I think you're trying to align with Precondition but using tabs instead of spaces.

Fixed. Taking this opportunity, I wanna ask how do you usually do indentation? I know GoLand will automatically indentate for you whenever we hit 'Enter' to move to the next line, but in cases like this where I want to some custom indentation, do you just hit 'spaces' to align them?

I did have the habit of using tab for indentation, and in this particular case, GoLand renders it differently so it looks all aligned on my GoLand but showed this weird indentation on a general text editor.

Code quote:

UpgradeSequenceReferenceInExpr

---

pkg/sql/catalog/seqexpr/sequence.go line 203 at r4 (raw file):

Previously, ajwerner wrote…

nit: I prefer return false, err here

done

---

pkg/sql/catalog/seqexpr/sequence.go line 209 at r4 (raw file):

Previously, ajwerner wrote…

nit: I prefer return false, err here

done

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 851 at r4 (raw file):

Previously, ajwerner wrote…

given you always call these as a pair and seqexpr.UpgradeSequenceReferenceInExpr seems pretty low-level, can we just change it so the only exported function is one that calls this pair?

good idea. Done!

Code quote:

hasUpgradedInDefault, err := seqexpr.UpgradeSequenceReferenceInExpr(col.DefaultExpr, seqNameToIDMappingInExpr)

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 857 at r4 (raw file):

Previously, ajwerner wrote…

Shouldn't this be *col.OnUpdateExpr?

good catch! Fixed

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 942 at r4 (raw file):

Previously, ajwerner wrote…

nit: you don't need the first if. nil will fail the type assertion

Good to learn that! Thank you!

Code quote:

		if d == nil {
			continue
		}
		tableDesc, ok := d.(catalog.TableDescriptor)

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 950 at r4 (raw file):

Previously, ajwerner wrote…

nit: you don't need the outer if

fixed

Code quote:

		dbName := ""
		d = descLookupFn(tableDesc.GetParentID())

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 969 at r4 (raw file):

Previously, ajwerner wrote…

if neither of these are true, then what? Should we return an error?

In this case, scName will be empty -- this is intentional because I was paranoid about not being to fully resolve every table's name during restore (which certainly will happen, in cases like, RESTORE testdb.* FROM ..., where the testdb descriptor will not be present during restore) so if we cannot find the db or schema name, we leave it as empty.

Then, later in findUniqueBestMatchingForTableName where we do the name matching, we expect and handle names that are not fully resolved. See the comments for findUniqueBestMatchingForTableName for some examples.

[ajwerner]

when you get the build green.

Reviewed 2 of 7 files at r3, 3 of 11 files at r4, 7 of 7 files at r5, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @fqazi, @postamar, @stevendanna, and @Xiang-Gu)

---

pkg/sql/catalog/seqexpr/sequence.go line 323 at r5 (raw file):

		cs, ts := candidateTableName.Schema(), targetTableName.Schema()
		cdb, tdb := candidateTableName.Catalog(), targetTableName.Catalog()
		if ct != "" && tt != "" && ct != tt {

nit: I don't think these three conditionals are better than an ||, but that's just me. Another option is a switch and a ,

switch {
case ct != "" && tt != "" && ct != tt,
    cs != "" && ts != "" && cs != ts,
    cdb != "" && tdb != "" && cdb != tdb:
   // not a match
case match != descpb.InvalidID:
   return descpb.InvalidID, errors.AssertionFailedf("more than 1 matches found for %v",
	targetTableName.String())
default:
    match = id
}

Code quote:

		if ct != "" && tt != "" && ct != tt {
			continue
		}
		if cs != "" && ts != "" && cs != ts {
			continue
		}
		if cdb != "" && tdb != "" && cdb != tdb {

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 924 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

I'd like @ajwerner 's opinion on whether it's safe to modify the view query like this or not, just to make sure. Personally I don't see why not.

I think it's fine.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 969 at r4 (raw file):

Previously, Xiang-Gu (Xiang Gu) wrote…

In this case, scName will be empty -- this is intentional because I was paranoid about not being to fully resolve every table's name during restore (which certainly will happen, in cases like, RESTORE testdb.* FROM ..., where the testdb descriptor will not be present during restore) so if we cannot find the db or schema name, we leave it as empty.

Then, later in findUniqueBestMatchingForTableName where we do the name matching, we expect and handle names that are not fully resolved. See the comments for findUniqueBestMatchingForTableName for some examples.

Ack. might be worth a note in commentary.

---

pkg/sql/catalog/tabledesc/table_desc_builder.go line 820 at r5 (raw file):

		}
	} else {
		return hasUpgraded, errors.AssertionFailedf("expect a table descriptor")

nit: this assertion error message could be better, maybe: "table descriptor %v (%d) is not a table, view, or sequence" and give its name and ID?

[ajwerner]

It seems like you're missing a bazel generate:
/home/roach/.cache/bazel/_bazel_roach/c5a4e7d36696d9cd970af2045211a7df/sandbox/processwrapper-sandbox/3759/execroot/com_github_cockroachdb_cockroach/pkg/storage/mvcc_test.go: import of "github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"

[postamar]

LGTM, CI is failing only due to the linter not being satisfied.

[Xiang-Gu]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)