sql: reference sequences used in views by their IDs

[the-ericwang35]

Followup to #59864, fixes #61205.
Previously, a change was introduced to store sequences
used in DEFAULT expressions to be referenced by their
IDs instead of their names, to allow sequence renaming.
In this patch, we want to do something similar by
rewriting sequence references in views to be
referenced by their IDs. This allows sequences
used in views to be renamed.

Another thing this PR does is change the output of sequences
to look like 's'::REGCLASS rather than 's':::STRING::REGCLASS,
since we're no longer type checking them (since it's unnecessary).

Release note (sql change): reference sequences used in views
by their IDs to allow these sequences to be renamed.

[cockroach-teamcity]

This change is 

[ajwerner]

nice

Reviewed 6 of 20 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)

---

pkg/ccl/backupccl/restore_planning.go, line 128 at r1 (raw file):

// rewriteSequencesInExpr rewrites all sequence IDs in the input expression
// string according to rewrites.
func rewriteSequencesInExpr(expr string, rewrites DescRewriteMap) (string, error) {

did you test this for views? Can you write a test that backs up a view and then restores it? I'm worried we're going to have problems here.

---

pkg/sql/create_view.go, line 357 at r1 (raw file):

}

// startViewWalk prepares to walk the given viewQuery by defining the

this name is weird to me. I'd call it what it does, something about re-writing sequences.

---

pkg/sql/create_view.go, line 385 at r1 (raw file):

	stmt, err := parser.ParseOne(viewQuery)
	if err != nil {
		return "", err

nit: wrap this error for easier debugging

---

pkg/sql/show_create_clauses.go, line 112 at r1 (raw file):

	decodedViewQuery, err := formatViewQueryForDisplay(ctx, semaCtx, desc)
	if err != nil {
		f.WriteString(desc.GetViewQuery())

can you log a warning here?

---

pkg/sql/catalog/schemaexpr/column.go, line 327 at r1 (raw file):

// GetTypeExprAndSeqID takes an expr and looks for a sequence ID in
// this expr. If it finds one, it will return that ID as well as the
// Expr that contains it.

This API is now weird to me. If it returns a non-nil expr it always returns what it was given. How about just strip the expr return value? Also, it doesn't seem like anybody does anything with it anyway.

[the-ericwang35]

Dismissed @ajwerner from a discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

---

pkg/ccl/backupccl/restore_planning.go, line 128 at r1 (raw file):

Previously, ajwerner wrote…

did you test this for views? Can you write a test that backs up a view and then restores it? I'm worried we're going to have problems here.

That's a good point, I've added some additional logic to look for descriptor rewrites in the view query, since it can now contain sequence IDs. I've also added a test to backup and restore a view.

---

pkg/sql/create_view.go, line 357 at r1 (raw file):

Previously, ajwerner wrote…

this name is weird to me. I'd call it what it does, something about re-writing sequences.

Done.

---

pkg/sql/create_view.go, line 385 at r1 (raw file):

Previously, ajwerner wrote…

nit: wrap this error for easier debugging

Done.

---

pkg/sql/show_create_clauses.go, line 112 at r1 (raw file):

Previously, ajwerner wrote…

can you log a warning here?

Done.

---

pkg/sql/catalog/schemaexpr/column.go, line 327 at r1 (raw file):

Previously, ajwerner wrote…

This API is now weird to me. If it returns a non-nil expr it always returns what it was given. How about just strip the expr return value? Also, it doesn't seem like anybody does anything with it anyway.

Good point, removed the expr return value.

[ajwerner]

Reviewed 3 of 20 files at r1, 3 of 14 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

---

pkg/ccl/backupccl/restore_planning.go, line 128 at r1 (raw file):

Previously, the-ericwang35 (Eric Wang) wrote…

That's a good point, I've added some additional logic to look for descriptor rewrites in the view query, since it can now contain sequence IDs. I've also added a test to backup and restore a view.

I saw you added a test for the old views and making sure that that does not get upgraded. Can you also test that for these new views that when we back them up and then restore them that everything is happy -- that the references are updated correctly etc.

I think there are a few versions of that. One is backing up the entire database and restoring it. In that case we'd expect both the sequence and view to get restored and to refer to eachother. Another is restoring the view after dropping it. Another still is restoring both the sequence and the view into an existing database.

---

pkg/sql/show_create_clauses.go, line 112 at r2 (raw file):

	decodedViewQuery, err := formatViewQueryForDisplay(ctx, semaCtx, desc)
	if err != nil {
		log.Warningf(ctx, "error converting sequence IDs to names: %+v", err)

one more, can you put the ID and the name here? If we see this we'll want to be able to figure out which view.

[the-ericwang35]

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

---

pkg/ccl/backupccl/restore_planning.go, line 128 at r1 (raw file):

Previously, ajwerner wrote…

I saw you added a test for the old views and making sure that that does not get upgraded. Can you also test that for these new views that when we back them up and then restore them that everything is happy -- that the references are updated correctly etc.

I think there are a few versions of that. One is backing up the entire database and restoring it. In that case we'd expect both the sequence and view to get restored and to refer to eachother. Another is restoring the view after dropping it. Another still is restoring both the sequence and the view into an existing database.

Yep that's a good idea, added TestBackupRestoreSequencesInViews in backup_test.go. There's already a test called TestBackupRestoreSequence that tests this for sequences used in default expressions, so I didn't add any new tests for those cases.

---

pkg/sql/show_create_clauses.go, line 112 at r2 (raw file):

Previously, ajwerner wrote…

one more, can you put the ID and the name here? If we see this we'll want to be able to figure out which view.

Yep, done.

[ajwerner]

Reviewed 1 of 20 files at r1, 1 of 14 files at r2, 3 of 10 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

[RaduBerinde]

Nice work!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @the-ericwang35)

---

pkg/sql/sem/tree/walk.go, line 1212 at r3 (raw file):

		stmtCopy.Limit = &lCopy
	}
	return &stmtCopy

Need to make a copy of stmt.With - both the slice, and each element (or switch the elements to not be pointers). In general, please double-check that all slices that we're now modifying in walkStmt are getting copied.

---

pkg/sql/sem/tree/walk.go, line 1599 at r3 (raw file):

// called in VisitPre for every node. The visitor stops as soon as an error is
// returned.
type SimpleVisitor struct {

[nit] Why do you need to export this? It's meant to be used via SimpleVisit().

[the-ericwang35]

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

---

pkg/sql/sem/tree/walk.go, line 1212 at r3 (raw file):

Previously, RaduBerinde wrote…

Need to make a copy of stmt.With - both the slice, and each element (or switch the elements to not be pointers). In general, please double-check that all slices that we're now modifying in walkStmt are getting copied.

Ah sorry good catch, added code to make a copy.

---

pkg/sql/sem/tree/walk.go, line 1599 at r3 (raw file):

Previously, RaduBerinde wrote…

[nit] Why do you need to export this? It's meant to be used via SimpleVisit().

SimpleVisit() takes in a tree.Expr, whereas I wanted to pass in a tree.Statement to walk the statement. You're right though, there was no need to export SimpleVisitor, it makes more sense to create a function similar to SimpleVisit() but for statements, so I created a new function SimpleStmtVisit() inside of this file so that I no longer need to export SimpleVisitor.

[ajwerner]

Reviewed 1 of 9 files at r4.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner and @RaduBerinde)

[the-ericwang35]

TFTRs!

bors r=ajwerner

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)