sql: Support DISCARD SEQUENCES #86230
Conversation
|
This change is |
e-mbrown
force-pushed
the
eb/discard
branch
2 times, most recently
from
5db6214 to
edca742
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown and @rafiss)
pkg/sql/discard.go line 45 at r1 (raw file):
p.preparedStatements.DeleteAll(ctx) //DISCARD SEQUENCES
nit: add a space after //
pkg/sql/discard.go line 49 at r1 (raw file):
case tree.DiscardModeSequences: p.SessionData().SequenceState = sessiondata.NewSequenceState()
these calls should both be:
p.sessionDataMutatorIterator.applyOnEachMutator(func(m sessionDataMutator) {
m.data.SequenceState = sessiondata.NewSequenceState()
m.initSequenceCache()
})
applyOnEachMutator is needed because it iterates through the stack of sessionData -- when a new transaction is started, a new sessionData is added to the stack.
pkg/sql/logictest/testdata/logic_test/discard line 47 at r1 (raw file):
statement ok DISCARD ALL
can you verify that DISCARD ALL also removes the sequence info too
pkg/sql/logictest/testdata/logic_test/discard line 62 at r1 (raw file):
statement ok CREATE SEQUENCE lastval_test_2 START WITH 10
nit: maybe name it discard_seq_test
pkg/sql/logictest/testdata/logic_test/discard line 67 at r1 (raw file):
SELECT nextval('lastval_test_2') ---- 10
we also want to make sure we match this behavior from postgres:
rafiss@127:postgres> create sequence s2 cache 10;
CREATE SEQUENCE
rafiss@127:postgres> select nextval('s2');
+---------+
| nextval |
|---------|
| 1 |
+---------+
rafiss@127:postgres> discard sequences;
DISCARD SEQUENCES
rafiss@127:postgres> select nextval('s2');
+---------+
| nextval |
|---------|
| 11 |
+---------+
pkg/sql/logictest/testdata/logic_test/discard line 72 at r1 (raw file):
SELECT lastval() ---- 10
can you also check that SELECT currval('lastval_test_2') returns 10 here
pkg/sql/logictest/testdata/logic_test/discard line 78 at r1 (raw file):
statement error pgcode 55000 pq: lastval\(\): lastval is not yet defined in this session SELECT lastval()
and check that SELECT currval('lastval_test_2') returns a "currval of sequence "s" is not yet defined in this session" error here
pkg/sql/sem/tree/discard.go line 28 at r1 (raw file):
// DiscardModeSequences represents a DISCARD SEQUENCES statement DiscardModeSequences DiscardMode = iota
the iota keyword is special. it's defined once in the block, and then it causes everything else to count up from there. so you want
const (
// DiscardModeAll represents a DISCARD ALL statement.
DiscardModeAll DiscardMode = iota
// DiscardModeSequences represents a DISCARD SEQUENCES statement
DiscardModeSequences
)
the type is also added implicitly.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/discard.go line 49 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
these calls should both be:
p.sessionDataMutatorIterator.applyOnEachMutator(func(m sessionDataMutator) { m.data.SequenceState = sessiondata.NewSequenceState() m.initSequenceCache() })
applyOnEachMutatoris needed because it iterates through the stack of sessionData -- when a new transaction is started, a new sessionData is added to the stack.
Done.
pkg/sql/logictest/testdata/logic_test/discard line 47 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can you verify that DISCARD ALL also removes the sequence info too
Done.
pkg/sql/logictest/testdata/logic_test/discard line 67 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
we also want to make sure we match this behavior from postgres:
rafiss@127:postgres> create sequence s2 cache 10; CREATE SEQUENCE rafiss@127:postgres> select nextval('s2'); +---------+ | nextval | |---------| | 1 | +---------+ rafiss@127:postgres> discard sequences; DISCARD SEQUENCES rafiss@127:postgres> select nextval('s2'); +---------+ | nextval | |---------| | 11 | +---------+
Done.
pkg/sql/logictest/testdata/logic_test/discard line 72 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
can you also check that
SELECT currval('lastval_test_2')returns 10 here
Done.
pkg/sql/logictest/testdata/logic_test/discard line 78 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
and check that
SELECT currval('lastval_test_2')returns a "currval of sequence "s" is not yet defined in this session" error here
Done.
pkg/sql/sem/tree/discard.go line 28 at r1 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
the
iotakeyword is special. it's defined once in the block, and then it causes everything else to count up from there. so you wantconst ( // DiscardModeAll represents a DISCARD ALL statement. DiscardModeAll DiscardMode = iota // DiscardModeSequences represents a DISCARD SEQUENCES statement DiscardModeSequences )the type is also added implicitly.
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/logictest/testdata/logic_test/discard line 50 at r2 (raw file):
statement error pgcode 55000 pq: lastval\(\): lastval is not yet defined in this session SELECT lastval()
this test isn't testing the right thing, since there was no sequence used before the call to DISCARD ALL. so we'd want to create and use a sequence before DISCARD ALL
Release note (sql change): We now support DISCARD SEQUENCES, which discards all sequence-related state such as currval/lastval information. DISCARD ALL now also discards sequence-related state. Release justification: Bug Fix
There was a problem hiding this comment.
this lgtm! i'm gonna try to bors it, since the failures seem too be flakes
bors r+
Reviewed 2 of 7 files at r1, 1 of 3 files at r2, 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
Build succeeded: |
Refs #83061
Release note (sql change): We now support DISCARD SEQUENCES, which discards
all sequence-related state such as currval/lastval information. DISCARD ALL
now also discards sequence-related state.
Release justification: Low risk, high benefit changes to existing functionality(DISCARD ALL)