sql: basic sequences functionality

[vilterp]

Implement CREATE SEQUENCE and the nextval function, as the first steps of adding sequences support.

The CREATE SEQUENCE statement:

• creates and saves a table descriptor with its sequence_settings field set. The presence of the sequence_settings field on a descriptor signifies that it represents a sequence, not a table or a view.
• initializes the sequence value at the key /Table/<id>/SequenceValue as a bare int64.

This PR leaves off the AS and OWNED BY options of the CREATE TABLE statement, to be revisited in later PRs which handle bounds checks and the column <=> sequence dependency relationship, respectively.

The nextval function looks up the descriptor by name, calls the Inc KV operation on the key, and returns the result.

RFC: #19196
Tracking issue: #19723 (shows future work)

[cockroach-teamcity]

This change is 

[vilterp]

This is a very specific method to have in this interface. Since this is the first builtin function which makes KV writes, not sure what the best way is to plumb things through.

[vilterp]

cc @awoods187 @robert-s-lee

[justinj]

Is anything going to need to change in pg_catalog or information_schema for this change (adding Java acceptance tests might help you figure out the answer to this)?

---

Review status: 0 of 27 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

---

pkg/sql/create.go, line 1333 at r7 (raw file):

	settings := &sqlbase.TableDescriptor_SequenceSettings{}
	settings.Cache = 1

nit: these can be inlined in the struct literal I think?

---

pkg/sql/create.go, line 1334 at r7 (raw file):

	settings := &sqlbase.TableDescriptor_SequenceSettings{}
	settings.Cache = 1
	settings.Cycle = false

just to be clear, this will be a no-op (since false is the zero value of bool) but I also like the explicitness, I don't particularly care if you set it or not.

---

pkg/sql/create.go, line 1343 at r7 (raw file):

		case parser.IncrementOption:
			settings.Increment = *opt.Increment
		default:

nit: don't need default:. this might be clearer as an if type check than as a switch but I don't really care too much

---

pkg/sql/create.go, line 1831 at r7 (raw file):

	}

	// initialize the sequence value

nit: capitalize and period

---

pkg/sql/create.go, line 1838 at r7 (raw file):

	b := &client.Batch{}
	// TODO(vilterp): setting with Put then incrementing with Inc is not supposed to work, but does.

someone else will have to help you with this one :D

---

pkg/sql/event_log.go, line 59 at r7 (raw file):

	EventLogCreateSequence EventLogType = "create_sequence"
	// EventLogDropSequence is recorded when a sequence is dropped.
	// EventLogDropSequence EventLogType = "drop_sequence"

Is this meant to be commented out?

---

pkg/sql/logictest/testdata/logic_test/sequences, line 3 at r7 (raw file):

# LogicTest: default

# SEQUENCE CREATION

These tests are great - could you also add some integration tests? For Java at the minimum, since JDBC tends to do some wackier stuff than other drivers. I can help you with this if you want, check out pkg/acceptance/testdata/java (also node or even others if you're feeling ambitious, but I think those are all less important than JDBC)

---

pkg/sql/logictest/testdata/logic_test/sequences, line 8 at r7 (raw file):

CREATE SEQUENCE foo

# can't create again

nit: comments are generally capitalized and have a period (this might be less strictly enforced in logic tests but I think it's a good thing to do)

---

pkg/sql/logictest/testdata/logic_test/sequences, line 18 at r7 (raw file):

# can't create with 0 increment
statement error INCREMENT must not be zero
CREATE SEQUENCE zero_test INCREMENT 0

We don't do so in many existing logic tests, but I think using pgerror _____ to assert the pg code in error cases is a good thing to do!

---

pkg/sql/parser/create.go, line 857 at r7 (raw file):

	FormatNode(buf, f, &node.Name)
	for _, option := range node.Options {
		buf.WriteRune(' ')

nit: we use WriteByte elsewhere in this file, so might as well use it here for consistency

---

pkg/sql/parser/create.go, line 863 at r7 (raw file):

// SequenceOption represents an option on a CREATE SEQUENCE statement.
type SequenceOption interface {

I'm not sure these all need to be their own structs, seems like we could maybe get by with just a SequenceOption struct that had all these as fields (you can indicate presence/nonpresence by using a pointer)

---

pkg/sql/parser/sql.y, line 215 at r7 (raw file):

}
func (u *sqlSymUnion) intVal() *int64 {
	return u.val.(*int64)

Reviewable is showing me some kind of red arrow here that I'm not sure about the implication of, this might have tabs instead of spaces (sql.y is spaces for some reason)?

err, mostly, there are two lines in it with tabs instead of spaces D: we should fix those

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

%type <[]SequenceOption> sequence_option_list opt_sequence_option_list
%type <SequenceOption> sequence_option_elem
%type <*int64> just_an_int

Postgres calls this NumericOnly and also accepts floats, will we have any problems if we don't accept floats (and then assert that they're whole numbers)?

If not, is there a reason we need a new nonterminal rather than reusing signed_iconst?

---

pkg/sql/parser/sql.y, line 3125 at r7 (raw file):

| CACHE just_an_int       { $$.val = CacheOption{Cache: $2.intVal()} }
| CYCLE                   { $$.val = CycleOption{Cycle: true} }
| NO CYCLE								{ $$.val = CycleOption{Cycle: false} }

nit: tabs instead of spaces, we should really lint sql.y

---

Comments from Reviewable

[vilterp]

I'm going to add stuff to pg_catalog and information_schema in a separate PR (just in the spirit of small PRs). Added a Java test that makes a sequence and uses it; it worked without changing those internal tables.

---

Reviewed 5 of 24 files at r1, 2 of 17 files at r4, 20 of 20 files at r8, 1 of 11 files at r11, 2 of 5 files at r14, 1 of 1 files at r15.
Review status: 14 of 28 files reviewed at latest revision, 15 unresolved discussions, some commit checks pending.

---

pkg/sql/create.go, line 1333 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: these can be inlined in the struct literal I think?

Done.

---

pkg/sql/create.go, line 1334 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

just to be clear, this will be a no-op (since false is the zero value of bool) but I also like the explicitness, I don't particularly care if you set it or not.

Done. Leaving it in to be explicit (but in a struct literal)

---

pkg/sql/create.go, line 1343 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: don't need default:. this might be clearer as an if type check than as a switch but I don't really care too much

Done.

---

pkg/sql/create.go, line 1831 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: capitalize and period

Done.

---

pkg/sql/create.go, line 1838 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

someone else will have to help you with this one :D

Done. (Realized I could just use Inc)

---

pkg/sql/event_log.go, line 59 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Is this meant to be commented out?

Done. Will add that in a later PR when I do the DROP command

---

pkg/sql/logictest/testdata/logic_test/sequences, line 3 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

These tests are great - could you also add some integration tests? For Java at the minimum, since JDBC tends to do some wackier stuff than other drivers. I can help you with this if you want, check out pkg/acceptance/testdata/java (also node or even others if you're feeling ambitious, but I think those are all less important than JDBC)

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 8 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: comments are generally capitalized and have a period (this might be less strictly enforced in logic tests but I think it's a good thing to do)

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 18 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

We don't do so in many existing logic tests, but I think using pgerror _____ to assert the pg code in error cases is a good thing to do!

Done.

---

pkg/sql/parser/create.go, line 857 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: we use WriteByte elsewhere in this file, so might as well use it here for consistency

Done.

---

pkg/sql/parser/create.go, line 863 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

I'm not sure these all need to be their own structs, seems like we could maybe get by with just a SequenceOption struct that had all these as fields (you can indicate presence/nonpresence by using a pointer)

Done.

---

pkg/sql/parser/sql.y, line 215 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Reviewable is showing me some kind of red arrow here that I'm not sure about the implication of, this might have tabs instead of spaces (sql.y is spaces for some reason)?

err, mostly, there are two lines in it with tabs instead of spaces D: we should fix those

Done.

---

pkg/sql/parser/sql.y, line 3125 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit: tabs instead of spaces, we should really lint sql.y

Done.

---

Comments from Reviewable

[justinj]

but @knz should probably take a look as well.

---

Review status: 14 of 28 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

pkg/sql/planner.go, line 269 at r15 (raw file):

	}
	seqValueKey := keys.MakeSequenceKey(uint32(descriptor.ID))
	res, err := client.IncrementValRetryable(

nit i missed before: you could just return this directly

---

pkg/sql/parser/create.go, line 863 at r7 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done.

Sorry if I was unclear, I meant that we could have a single struct that gets threaded through like

type SequenceOptions struct {
    Cycle bool
    Increment *int
    ...
}

etc. but on second thought that might be more difficult than I'm imagining with the grammar, maybe check what another statement does here, it might just be what you had before? Sorry if that's the case!

---

Comments from Reviewable

[justinj]

Actually, would you mind including the Java test in this PR as well (and then the README change isn't so out-of-place :D)?

---

Review status: 14 of 28 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[vilterp]

Reviewed 2 of 12 files at r9, 4 of 11 files at r11, 1 of 4 files at r13, 7 of 8 files at r16, 3 of 3 files at r17.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

pkg/sql/planner.go, line 269 at r15 (raw file):

Previously, justinj (Justin Jaffray) wrote…

nit i missed before: you could just return this directly

Done.

---

pkg/sql/parser/create.go, line 863 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Sorry if I was unclear, I meant that we could have a single struct that gets threaded through like

type SequenceOptions struct {
    Cycle bool
    Increment *int
    ...
}

etc. but on second thought that might be more difficult than I'm imagining with the grammar, maybe check what another statement does here, it might just be what you had before? Sorry if that's the case!

The current implementation (SequenceOption with IntVal and BoolVal) isn't too awkward — mimics the PG grammar, and some other rules that thread through a slice. Maybe I could thread that struct through in a similar way; will give it a try.

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

Previously, justinj (Justin Jaffray) wrote…

Postgres calls this NumericOnly and also accepts floats, will we have any problems if we don't accept floats (and then assert that they're whole numbers)?

If not, is there a reason we need a new nonterminal rather than reusing signed_iconst?

Renamed it to iconst_no_err and added an explanatory comment. @knz, is there a better way to factor this?

---

pkg/sql/sqlbase/structured.go, line 325 at r17 (raw file):

// KindName returns what kind of database object this descriptor describes.
func (desc *TableDescriptor) KindName() string {

Wasn't sure what to call this, so I said Kind. (TypeName was already taken, and always returns "relation"). Does this name seem reasonable? Do we already have a term for "sequence"/"table"/"view"?

---

Comments from Reviewable

[knz]

I'm mostly good with this too.
Just one error remaining in the name resolution, and some missing tests.

---

Reviewed 2 of 24 files at r1, 13 of 20 files at r18, 5 of 11 files at r21, 1 of 4 files at r23, 2 of 5 files at r24, 4 of 7 files at r25, 3 of 3 files at r26, 5 of 5 files at r27, 2 of 2 files at r28.
Review status: all files reviewed at latest revision, 19 unresolved discussions, some commit checks failed.

---

pkg/sql/planner.go, line 256 at r18 (raw file):

func (p *planner) IncrementSequence(
	ctx context.Context, seqName string,

seqName must be a *parser.TableName, not string.

---

pkg/sql/planner.go, line 258 at r18 (raw file):

	ctx context.Context, seqName string,
) (int64, error) {
	tableName, err := parser.ParseTableName(seqName)

call ParseTableName in the builtin definition for nextval, not here.

---

pkg/sql/planner.go, line 262 at r18 (raw file):

		return 0, err
	}
	tableName.DatabaseName = parser.Name(p.session.Database)

That doesn't seem right. As per the pg doc (https://www.postgresql.org/docs/10/static/functions-sequence.html), the sequence name is determined using the search path.
Use searchAndQualifyDatabase instead.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 84 at r28 (raw file):

SELECT nextval('baz')
----
7

Add tests that check that sequences can be found in other databases via the search path.
Also add at least one test that checks sequence names can use special characters.

---

pkg/sql/parser/builtins.go, line 1148 at r28 (raw file):

			fn: func(evalCtx *EvalContext, args Datums) (Datum, error) {
				seqName := MustBeDString(args[0])
				res, err := evalCtx.Planner.IncrementSequence(evalCtx.Ctx(), string(seqName))

as I explain in a previous comment, the translation from string to TableName must occur here, not in IncrementSequence.

---

pkg/sql/parser/parse_test.go, line 224 at r28 (raw file):

		{`CREATE VIEW a AS TABLE b`},

		{`CREATE SEQUENCE a`},

Some IF NOT EXISTS test is missing here.

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Renamed it to iconst_no_err and added an explanatory comment. @knz, is there a better way to factor this?

I don't understand the question; what is the problem to be solved here?

Also I'd use %type <int64> iconst_no_err - no need for a pointer.

---

Comments from Reviewable

[nvanbenschoten]

Reviewed 4 of 24 files at r1, 2 of 17 files at r4, 3 of 20 files at r8, 1 of 20 files at r18, 2 of 12 files at r19, 4 of 11 files at r21, 1 of 4 files at r23, 1 of 5 files at r24, 4 of 7 files at r25, 3 of 3 files at r26, 5 of 5 files at r27, 2 of 2 files at r28.
Review status: all files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

pkg/keys/keys.go, line 620 at r18 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You need to consider how these keys will be formatted by the pretty-printer. Because you're using the table prefix, I'm suspicious this will be garbled because the second component is not an index ID. Worthwhile to add a test for how sequence keys pretty-print.

Also, sort of a small item, but we usually use shorter constants and we use lowercase. Perhaps s/SequenceValue/seqval/g.

"seqval" should also be pulled into constants.go

---

pkg/sql/create.go, line 1389 at r28 (raw file):

			settings.Cache = *option.IntVal
		case parser.SeqOptCycle:
			settings.Cycle = option.BoolVal

Should we be throwing errors for the options that are not yet supported?

---

pkg/sql/logictest/testdata/logic_test/sequences, line 1 at r28 (raw file):

# LogicTest: default

Why only the default config?

---

pkg/sql/parser/sql.y, line 3149 at r28 (raw file):

opt_sequence_option_list:
  sequence_option_list
| /* EMPTY */          { $$.val = []SequenceOption{} }

s/[]SequenceOption{}/[]SequenceOption(nil)/

---

pkg/sql/sqlbase/structured.go, line 326 at r28 (raw file):

// KindName returns what kind of database object this descriptor describes.
func (desc *TableDescriptor) KindName() string {
	if desc.IsTable() {

nit: I'd do something like:

switch {
case desc.IsTable():
    return ...
case desc.IsView():
    ...
case desc.IsSequence():
    ...
default:
    panic("what is going on")
}

to catch any logic errors.

---

pkg/sql/sqlbase/structured.go, line 336 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This method seems kind of unnecessary to me, since the plural is always formed by adding s to the end, which you can do in the corresponding format string.

+1

---

pkg/sql/sqlbase/structured.proto, line 611 at r28 (raw file):

  repeated MutationJob mutationJobs = 27 [(gogoproto.nullable) = false];

  message SequenceSettings {

These fields should be commented.

I'd also push to rename this to SequenceOpts or SequenceDescriptor, as the word "settings" has a specific meaning in Cockroach (session settings, cluster settings, etc.) and has not traditionally been used in this way.

---

Comments from Reviewable

[vilterp]

Reviewed 20 of 20 files at r18, 1 of 12 files at r19, 1 of 5 files at r27, 30 of 30 files at r29.
Review status: 9 of 37 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

---

pkg/keys/keys.go, line 620 at r18 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

"seqval" should also be pulled into constants.go

Done.

---

pkg/keys/keys.go, line 619 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Kinda silly, but this probably should be tested somehow if possible. Changing this function later should cause tests to fail, as it would represent a backward incompatible change to sequences (old sequences would become inaccessible).

Done.

---

pkg/sql/create.go, line 1389 at r28 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Should we be throwing errors for the options that are not yet supported?

Done. The parser now throws errors for the options we don't support

---

pkg/sql/planner.go, line 256 at r18 (raw file):

Previously, knz (kena) wrote…

seqName must be a *parser.TableName, not string.

Done.

---

pkg/sql/planner.go, line 258 at r18 (raw file):

Previously, knz (kena) wrote…

call ParseTableName in the builtin definition for nextval, not here.

Done.

---

pkg/sql/planner.go, line 262 at r18 (raw file):

Previously, knz (kena) wrote…

That doesn't seem right. As per the pg doc (https://www.postgresql.org/docs/10/static/functions-sequence.html), the sequence name is determined using the search path.
Use searchAndQualifyDatabase instead.

Done.

---

pkg/sql/planner.go, line 266 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Please add a logic test for this if you can - wouldn't want it to regress later.

Done.

---

pkg/sql/update.go, line 56 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

this seems like a stray line :)

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 1 at r28 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Why only the default config?

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 29 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

You should find a more appropriate pgerror code for this one.

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 44 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

What do you mean by Test-Script syntax error? I think you can put statement ok on top of it and it'll work, maybe?

Done. Changed this to return an error until we actually support it.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 84 at r28 (raw file):

Previously, knz (kena) wrote…

Add tests that check that sequences can be found in other databases via the search path.
Also add at least one test that checks sequence names can use special characters.

Done.

---

pkg/sql/parser/builtins.go, line 1148 at r28 (raw file):

Previously, knz (kena) wrote…

as I explain in a previous comment, the translation from string to TableName must occur here, not in IncrementSequence.

Done.

---

pkg/sql/parser/eval.go, line 1850 at r3 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I'm surprised the linter isn't failing this - but exported methods should have comments.

Done.

---

pkg/sql/parser/parse_test.go, line 224 at r28 (raw file):

Previously, knz (kena) wrote…

Some IF NOT EXISTS test is missing here.

Done.

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

Previously, knz (kena) wrote…

I don't understand the question; what is the problem to be solved here?

Also I'd use %type <int64> iconst_no_err - no need for a pointer.

The problem is that it seems weird to have the suffix _no_err in a rule name. Rules should just be named for what we are — if there's an error, the caller should move on to the next option. So I guess I was wondering if signed_iconst should just behave like this, and then iconst_no_err would not have to exist. Would have to refactor numeric_only; maybe I'll take a run at that, but didn't feel like messing with it.

I can't seem to change it to plain int64 without strange build errors of the form sqlDollar[2].int64 undefined (type sqlSymType has no field or method int64). Not sure what's going on there.

---

pkg/sql/parser/sql.y, line 3149 at r28 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

s/[]SequenceOption{}/[]SequenceOption(nil)/

Done.

---

pkg/sql/parser/sql.y, line 3174 at r28 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

This seems like a useful rule! Could you add some tests for it in parse_test? There is a section for examples that are supposed to return errors, I believe.

Not quite sure how to add tests for an individual rule… I see some tests for syntax errors, but they all seem to be parsing statements from the top level.

---

pkg/sql/sqlbase/structured.go, line 325 at r17 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Kind seems reasonable enough to me, but maybe just call it Kind instead of KindName? We don't have a type named kind so it's not ambiguous.

Done.

---

pkg/sql/sqlbase/structured.go, line 326 at r28 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: I'd do something like:

switch {
case desc.IsTable():
    return ...
case desc.IsView():
    ...
case desc.IsSequence():
    ...
default:
    panic("what is going on")
}

to catch any logic errors.
</blockquote></details>

Done.

---

*[pkg/sql/sqlbase/structured.go, line 336 at r28](https://reviewable.io:443/reviews/cockroachdb/cockroach/19664#-KxyvGkS8U7S-VBnvlII:-KyRiazWRJcAfcgyy5LG:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/4f452130d6b3a5b175ee4fe7a9b114bd0743141d/pkg/sql/sqlbase/structured.go#L336)):*
<details><summary><i>Previously, nvanbenschoten (Nathan VanBenschoten) wrote…</i></summary><blockquote>

+1
</blockquote></details>

Done.

---

*[pkg/sql/sqlbase/structured.proto, line 611 at r28](https://reviewable.io:443/reviews/cockroachdb/cockroach/19664#-KyHsOiUFnDVky7mhnB3:-KySoulLqLzse1HdhdCv:b-896fix) ([raw file](https://github.com/cockroachdb/cockroach/blob/4f452130d6b3a5b175ee4fe7a9b114bd0743141d/pkg/sql/sqlbase/structured.proto#L611)):*
<details><summary><i>Previously, nvanbenschoten (Nathan VanBenschoten) wrote…</i></summary><blockquote>

These fields should be commented.

I'd also push to rename this to `SequenceOpts` or `SequenceDescriptor`, as the word "settings" has a specific meaning in Cockroach (session settings, cluster settings, etc.) and has not traditionally been used in this way.
</blockquote></details>

Done.

---


*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/19664)*
<!-- Sent from Reviewable.io -->

[knz]

Reviewed 9 of 24 files at r30, 14 of 15 files at r31, 4 of 5 files at r32, 7 of 7 files at r33, 3 of 3 files at r34.
Review status: all files reviewed at latest revision, 23 unresolved discussions, all commit checks successful.

---

pkg/sql/planner.go, line 262 at r18 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done.

See my comment on nextval.

---

pkg/sql/planner.go, line 269 at r33 (raw file):

// IncrementSequence increments the given sequence and returns the result.
// Returns an error if the given name is not a sequence.

Given how you've modified the code here, the comment must be extended with: The caller must ensure that seqName is fully qualified already.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 124 at r34 (raw file):

statement ok
USE other_db

We don't typically use USE in logic tests -- SET DATABASE instead. Consistency is desirable here as the editor of a test will often grep for SET DATABASE to determine what to expect.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 133 at r34 (raw file):

query I
SELECT nextval('other_db.other_db_test')

This is OK but insufficient.

This must work:

CREATE DATABASE foo
CREATE DATABASE bar
CREATE SEQUENCE foo.x
SET DATABASE = bar
SET SEARCH_PATH = foo
SELECT nextval('x')

---

pkg/sql/parser/builtins.go, line 1148 at r28 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Done.

I don't think so -- QualifyWithDatabase does not use the search path. See my comment on the test for details of what needs to work.

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

Previously, vilterp (Pete Vilter) wrote…

The problem is that it seems weird to have the suffix _no_err in a rule name. Rules should just be named for what we are — if there's an error, the caller should move on to the next option. So I guess I was wondering if signed_iconst should just behave like this, and then iconst_no_err would not have to exist. Would have to refactor numeric_only; maybe I'll take a run at that, but didn't feel like messing with it.

I can't seem to change it to plain int64 without strange build errors of the form sqlDollar[2].int64 undefined (type sqlSymType has no field or method int64). Not sure what's going on there.

Oh I know what happened. I had forgotten something :)
Here's the trick: using %type <int64> here only works if you also add the following function definition alongside the others:

func (u *sqlSymUnion) int64() int64 {
   return u.val.(int64)
}

---

pkg/sql/parser/sql.y, line 3174 at r28 (raw file):

Previously, vilterp (Pete Vilter) wrote…

Not quite sure how to add tests for an individual rule… I see some tests for syntax errors, but they all seem to be parsing statements from the top level.

I had to look again at the entire patch to grok what's going on here. :)
FYI the reason why signed_iconst cannot be constrainted to 64 bits, is that we also use it for DECIMAL literal values which can be larger.

Now since this is what it does really, what you currently call iconst_no_err would be better called signed_iconst64!

This is a good rule to have indeed! And documented as such:

// signed_iconst64 is a variant of signed_iconst which only accepts (signed) integer literals that fit in an int64.

Then:

1. have the non-terminal produce a int64 as I explain in my other comment above
2. review/audit the rest of the grammar for calls to AsInt64(), I suspect that those other uses correspond to a use of ICONST / signed_iconst that can be replaced by iconst_no_err signed_iconst64. For example index_hints_params, opt_index_hints etc. (You might need to add another rule iconst64 (not signed) while you're at it)

---

Comments from Reviewable

[vilterp]

Reviewed 24 of 24 files at r30, 37 of 37 files at r35.
Review status: 0 of 38 files reviewed at latest revision, 23 unresolved discussions.

---

pkg/sql/planner.go, line 269 at r33 (raw file):

Previously, knz (kena) wrote…

Given how you've modified the code here, the comment must be extended with: The caller must ensure that seqName is fully qualified already.

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 124 at r34 (raw file):

Previously, knz (kena) wrote…

We don't typically use USE in logic tests -- SET DATABASE instead. Consistency is desirable here as the editor of a test will often grep for SET DATABASE to determine what to expect.

Done.

---

pkg/sql/logictest/testdata/logic_test/sequences, line 133 at r34 (raw file):

Previously, knz (kena) wrote…

This is OK but insufficient.

This must work:

CREATE DATABASE foo
CREATE DATABASE bar
CREATE SEQUENCE foo.x
SET DATABASE = bar
SET SEARCH_PATH = foo
SELECT nextval('x')

Done.

---

pkg/sql/parser/builtins.go, line 1148 at r28 (raw file):

Previously, knz (kena) wrote…

I don't think so -- QualifyWithDatabase does not use the search path. See my comment on the test for details of what needs to work.

QualifyWithDatabase calls searchAndQualifyWithDatabase, which uses the search path. I used it because it was already on the EvalPlanner interface, and if we were going to pass a TableName instead of a string to IncrementSequence (as you suggested), it seemed desirable to ensure that that TableName was completely ready to go use.

---

pkg/sql/parser/sql.y, line 709 at r7 (raw file):

Previously, knz (kena) wrote…

Oh I know what happened. I had forgotten something :)
Here's the trick: using %type <int64> here only works if you also add the following function definition alongside the others:

func (u *sqlSymUnion) int64() int64 {
   return u.val.(int64)
}

Done. Thanks!

---

pkg/sql/parser/sql.y, line 3174 at r28 (raw file):

Previously, knz (kena) wrote…

I had to look again at the entire patch to grok what's going on here. :)
FYI the reason why signed_iconst cannot be constrainted to 64 bits, is that we also use it for DECIMAL literal values which can be larger.

Now since this is what it does really, what you currently call iconst_no_err would be better called signed_iconst64!

This is a good rule to have indeed! And documented as such:

// signed_iconst64 is a variant of signed_iconst which only accepts (signed) integer literals that fit in an int64.

Then:

1. have the non-terminal produce a int64 as I explain in my other comment above
2. review/audit the rest of the grammar for calls to AsInt64(), I suspect that those other uses correspond to a use of ICONST / signed_iconst that can be replaced by iconst_no_err signed_iconst64. For example index_hints_params, opt_index_hints etc. (You might need to add another rule iconst64 (not signed) while you're at it)

Done. Thanks for your direction; I suspected a refactor like this was a good idea! Put the introduction of iconst64 and signed_iconst64 in a separate commit at the beginning of the PR.

---

Comments from Reviewable

[knz]

with nit

---

Reviewed 10 of 10 files at r36, 14 of 15 files at r37, 4 of 5 files at r38, 7 of 7 files at r39, 3 of 3 files at r40.
Review status: all files reviewed at latest revision, 21 unresolved discussions, some commit checks failed.

---

pkg/sql/parser/builtins.go, line 1148 at r28 (raw file):

Previously, vilterp (Pete Vilter) wrote…

QualifyWithDatabase calls searchAndQualifyWithDatabase, which uses the search path. I used it because it was already on the EvalPlanner interface, and if we were going to pass a TableName instead of a string to IncrementSequence (as you suggested), it seemed desirable to ensure that that TableName was completely ready to go use.

Oh interesting. TIL. Thanks!

---

pkg/sql/parser/eval.go, line 2004 at r39 (raw file):

	// QualifyWithDatabase resolves a possibly unqualified table name into a
	// normalized table name that is qualified by database.

Extend the comment to explain that QualifyWithDatabase also uses the search path. :)

---

Comments from Reviewable

[vilterp]

Curious for some advice from you, @knz:

In the above test, we create a new sequence and then create a table which uses it in a DEFAULT expression. However, when we insert the first row into the table, the primary key comes out as 2, not 1 (where the sequence is supposed to start). This is because default expressions are run once at table creation time to see if they work:

cockroach/pkg/sql/sqlbase/table.go

Lines 198 to 205 in d73de92

	// Try to evaluate once. If it is aimed to succeed during a
	// backfill, it must succeed here too. This tries to ensure that
	// we don't end up failing the evaluation during the schema change
	// proper.
	if _, err := typedExpr.Eval(evalCtx); err != nil {
	return nil, nil, err
	}
	d.DefaultExpr.Expr = typedExpr

This increments the sequence before a row is inserted into the table, which we don't want.

Would it be reasonable to flag functions which have side effects (e.g. writing to the database, as in this case), and exclude them from being run here? (Note also that we can't run it in a transaction and roll it back, because the nextval function operates in its own transaction)

If the solution is introduce a new flag on Builtin and use it to mark nextval as side effectful, maybe using the same terminology as postgres's volatility categories (https://www.postgresql.org/docs/8.2/static/xfunc-volatility.html) for these flags would be appropriate.

Also FWIW, I think this could be fixed in another PR if all else is good to go.

[jordanlewis]

So we do already have an impure flag on builtins and it does already propagate to those volatility categories, FWIW.

[knz]

We shouldn't be evaluating expressions "just to see if they work". That's just wrong thinking. I'm OK with removing that evaluation. If for any reason a user discovers a bug that makes a default expression invalid, they can use ALTER to change it. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[vilterp]

@knz I agree; happy to remove it. Asked you because you added the check. Is there some case we're forgetting? The logic tests still pass with it gone. Anyway, I'm going to make that a separate PR; we can discuss there.

[knz]

I didn't add it -- I merely moved it from where it was (once per backfill processor) to another place (once for the entire backfill). It shouldn't have been there in the first place.

[jordanlewis]

LGTM!

[jordanlewis]

i didn't totally follow the discussion around this one but is it intentional that this rule isn't used?

[vilterp]

Added comment with more explanation. It's used in a later commit than the one that introduces it.

[jordanlewis]

If we're not planning to support the cache option anytime soon we probably don't need to add this to the proto, especially not as a non-nullable field. An extra int64 per descriptor obviously won't kill us, though. And since descriptors are cached this might just not be worth worrying about.

[vilterp]

Agreed. Removed it.

[jordanlewis]

So we do already have an impure flag on builtins and it does already propagate to those volatility categories, FWIW.