sql: disallow cross-database sequence references

[fqazi]

Fixes: #69992

Previously, we incorrectly allowed cross DB references
for sequences. This was inadequate because we are going
to drop support for cross DB references. To address this
this patch will disable support for cross DB sequence
references by default.

Release note (sql change): Disallow cross DB references for sequences by
default. This can be enabled with the cluster setting
sql.cross_db_sequence_references.enabled.

[cockroach-teamcity]

This change is 

[ajwerner]

Reviewed 3 of 10 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

---

pkg/sql/rename_table.go, line 337 at r1 (raw file):

	// lead to a cross DB reference, and an appropriate
	// error is generated.
	checkDepForCrossDbRef := func(depID descpb.ID, isOwner bool) error {

my preference is that you create some consts like:

type crossDBDepType int
const owner, reference crossDBDepType = 0, 1

or something like that.

---

pkg/sql/rename_table.go, line 353 at r1 (raw file):

		// For tables return an error based on if we are depending
		// on a view or sequence.
		if tableDesc.IsTable() {

this cascade of conditionals is pretty hard to follow. Can we rework the conditions, perhaps in a switch or nested switches, to be clearer?

[ajwerner]

Seems like this change broke a test:

=== CONT  TestLogic/local/rename_table/rename_table_using_types_to_different_database_not_allowed_55709
    logic.go:1867: 
         
        testdata/logic_test/rename_table:432: 
        expected "this sequence will be OWNED BY a table \"t1s1ref\" in a different database after rename", but no error occurred
--- done: testdata/logic_test/rename_table with config local: 114 tests, 1 failures

Reviewed 1 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @fqazi)

---

pkg/sql/rename_table.go, line 413 at r2 (raw file):

Quoted 11 lines of code…

		case tableDesc.IsSequence() && depType == reference:
			// For sequences dependent objects can only be
			// a relations.
			return errors.WithHintf(
				pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
					"this sequence will be referenced BY a table %q in a different database after rename "+
						"(see the '%s' cluster setting)",
					dependentObject.GetName(),
					allowCrossDatabaseSeqReferencesSetting),
				crossDBReferenceDeprecationHint(),
			)

I think you lost the case for this sequence will be OWNED BY.

---

pkg/sql/rename_table.go, line 464 at r2 (raw file):

		// check if any column owns a sequence.
		for _, columnDesc := range tableDesc.Columns {
			if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {

this two-level setting checking feels like overkill. You already check the setting in checkDepForCrossDbRef, let that be the only check.

---

pkg/sql/rename_table.go, line 467 at r2 (raw file):

					err := checkDepForCrossDbRef(ownsSequenceID, owner)
					if err != nil {

uber-nit: you can inline the error definitions of all of these checks.

[fqazi]

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

---

pkg/sql/rename_table.go, line 413 at r2 (raw file):

Previously, ajwerner wrote…

		case tableDesc.IsSequence() && depType == reference:
			// For sequences dependent objects can only be
			// a relations.
			return errors.WithHintf(
				pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
					"this sequence will be referenced BY a table %q in a different database after rename "+
						"(see the '%s' cluster setting)",
					dependentObject.GetName(),
					allowCrossDatabaseSeqReferencesSetting),
				crossDBReferenceDeprecationHint(),
			)

I think you lost the case for this sequence will be OWNED BY.

Yes, it was accidentally dropped from the switch, can't believe I missed that 🤦‍♂️

[ajwerner]

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

---

pkg/sql/rename_table.go, line 475 at r3 (raw file):

		// check if any column owns a sequence.
		for _, columnDesc := range tableDesc.Columns {
			if !allowCrossDatabaseSeqOwner.Get(&p.execCfg.Settings.SV) {

I'm not super fond of the mixing of levels where we check the settings.

[fqazi]

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

---

pkg/sql/rename_table.go, line 475 at r3 (raw file):

Previously, ajwerner wrote…

I'm not super fond of the mixing of levels where we check the settings.

Done.

[ajwerner]

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

[fqazi]

@ajwerner TFTR!

bors r+

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build failed (retrying...):

• GitHub CI (Cockroach)

[craig]

Build succeeded:

• GitHub CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 3b8d7f7 to blathers/backport-release-21.2-70581: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[rafiss]

@fqazi @ajwerner friendly ping - does this still need to be backported?