sql: warn users that sequences are slower than using UUID

[biradarganesh25]

Even though the documentation tells the user to use
UUIDs for auto-generating unique IDs, many users might
not read the docs. This warning will make users aware of
better alternatives than using sequences.

Release note (sql change): Warn users that sequences are
slower than using UUID.

Fixes #41258

[blathers-crl]

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

• @awoods187 (author of sql: warn users that sequences are slower than using UUID #41258)
• @rafiss (commented on sql: warn users that sequences are slower than using UUID #41258)
• @knz (commented on sql: warn users that sequences are slower than using UUID #41258)

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

[cockroach-teamcity]

This change is 

[biradarganesh25]

Hey @rafiss could you take a look? I've followed your advice on the issue. I have a feeling I am not checking for the "nextva" properly, but couldn't think of any another way. Lemme know if i gotta change it, thanks

[knz]

A few style nits. Leaving correctness review to @rafiss

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @biradarganesh25, and @rafiss)

---

pkg/sql/create_table.go, line 213 at r1 (raw file):

}
func hasPrimaryKeySerialType(params runParams, colDef *tree.ColumnTableDef) (bool, error) {

nit: remove empty line

---

pkg/sql/create_table.go, line 242 at r1 (raw file):

	for _, def := range n.n.Defs {

nit: remove empty line

---

pkg/sql/create_table.go, line 246 at r1 (raw file):

		case *tree.UniqueConstraintTableDef:
			if v.PrimaryKey {

nit: remove empty line

---

pkg/sql/create_table.go, line 251 at r1 (raw file):

				}
			}
		case *tree.ColumnTableDef:

nit: you can add an empty line above case here

---

pkg/sql/create_table.go, line 255 at r1 (raw file):

				colsWithPrimaryKeyConstraint[v.Name] = true
			}

nit: remove empty line

---

pkg/sql/logictest/testdata/logic_test/create_table, line 488 at r1 (raw file):

query T noticetrace
CREATE TABLE serial_test2 (id SERIAL PRIMARY KEY, temp string )

nit: stray space at end

---

pkg/sql/logictest/testdata/logic_test/create_table, line 493 at r1 (raw file):

query T noticetrace
CREATE TABLE serial_test3 (id SERIAL , temp string, PRIMARY KEY (id))

nit: stray space before comma

[blathers-crl]

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

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

[biradarganesh25]

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

---

pkg/sql/create_table.go, line 213 at r1 (raw file):

Previously, knz (kena) wrote…

nit: remove empty line

Done.

---

pkg/sql/create_table.go, line 242 at r1 (raw file):

Previously, knz (kena) wrote…

nit: remove empty line

Done.

---

pkg/sql/create_table.go, line 246 at r1 (raw file):

Previously, knz (kena) wrote…

nit: remove empty line

Done.

---

pkg/sql/create_table.go, line 251 at r1 (raw file):

Previously, knz (kena) wrote…

nit: you can add an empty line above case here

Done.

---

pkg/sql/create_table.go, line 255 at r1 (raw file):

Previously, knz (kena) wrote…

nit: remove empty line

Done.

---

pkg/sql/logictest/testdata/logic_test/create_table, line 488 at r1 (raw file):

Previously, knz (kena) wrote…

nit: stray space at end

Done.

---

pkg/sql/logictest/testdata/logic_test/create_table, line 493 at r1 (raw file):

Previously, knz (kena) wrote…

nit: stray space before commit

Done.

[biradarganesh25]

Thanks @knz! I've updated the file. Also, I realized I had not synced with upstream before raising PR, which is why a lot of CI test failed (I'm guessing). I've updated that also. Got a small doubt: I'd like to run only the create_table logictest but this command is failing as no test files found: make test PKG=./pkg/sql/logictest TESTS=TestLogic TESTFLAGS="-config local -d create_table"
If you see something obviously wrong in the command please let me know, the entire logictest takes quite a bit of time to run. Thanks!

[biradarganesh25]

Ah never mind, you need to give full path. Just figured it out.

[knz]

Try this, much simpler and faster:

make testbaselogic FILES=create_table TESTCONFIG=local

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• Please ensure your git commit message contains a release note.
• When CI has completed, please ensure no errors have appeared.

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

[rafiss]

looks pretty good! i have small requests, and also your change needs to be rebased on top of master to address a merge conflict.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @biradarganesh25, and @knz)

---

pkg/sql/create_table.go, line 215 at r4 (raw file):

	if colDef.IsSerial {
		return true, nil
	}

can you add one more case here?

	if colDef.IsSerial || colDef.GeneratedIdentity.IsGeneratedAsIdentity {
		return true, nil
	}

(generated as identity is a newer feature that is essentially an alias for SERIAL)

---

pkg/sql/create_table.go, line 224 at r4 (raw file):

			name = t.Name
		case *tree.UnresolvedName:
			name = t.String()

this case should be

		case *tree.UnresolvedName:
			fn, err := t.ResolveFunction(params.SessionData().SearchPath)
			if err != nil {
				return false, err
			}
			name = fn.Name

---

pkg/sql/create_table.go, line 266 at r4 (raw file):

				if primaryKeySerial {
					params.p.BufferClientNotice(

sending the notice should be done outside of the loop -- we only want to send one notice per create-stmt

[biradarganesh25]

Done.

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

---

pkg/sql/create_table.go, line 215 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

can you add one more case here?

	if colDef.IsSerial || colDef.GeneratedIdentity.IsGeneratedAsIdentity {
		return true, nil
	}

(generated as identity is a newer feature that is essentially an alias for SERIAL)

Done.

---

pkg/sql/create_table.go, line 224 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

this case should be

		case *tree.UnresolvedName:
			fn, err := t.ResolveFunction(params.SessionData().SearchPath)
			if err != nil {
				return false, err
			}
			name = fn.Name

I knew I had messed up here - wasn't sure where to get the serach path, thanks. Made the change.

---

pkg/sql/create_table.go, line 266 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

sending the notice should be done outside of the loop -- we only want to send one notice per create-stmt

There's a break after the notice is added (inside the if condition itself).... would that be enough?

[biradarganesh25]

Also, sorry for leaving the PR for too long, too many thing going on (masters).

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

[rafiss]

no problem, thanks for your work on this! lgtm

bors r+

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @awoods187, @biradarganesh25, @knz, and @rafiss)

---

pkg/sql/create_table.go, line 266 at r4 (raw file):

Previously, biradarganesh25 (Ganesh Biradar) wrote…

There's a break after the notice is added (inside the if condition itself).... would that be enough?

ah didn't notice that; yeah that's fine

[craig]

Build succeeded:

• GitHub CI (Cockroach)