sql/schemachanger: enable declarative CREATE SEQUENCE #117793
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
fqazi
force-pushed
the
createSequenceFin
branch
3 times, most recently
from
9ccf6a1
to
ba41c22
Compare
There was a problem hiding this comment.
nice work! just had tiny nits
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
-- commits line 21 at r2:
are the next two commits after this examples of things that would have failed before?
pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go line 48 at r2 (raw file):
// descriptorDataIsBeingAdded indicates that descriptor data is being // added and was not public earlier. This is mainly used to detect if an
this could lead to confusion. the comment has "descriptorDataIsBeingAdded indicates that descriptor data is being added" but the function is named descriptorDataIsNotBeingAdded. it looks like descriptorDataIsNotBeingAdded is correct.
pkg/sql/schemachanger/sctest/backup.go line 117 at r5 (raw file):
func backupSuccess(t *testing.T, factory TestServerFactory, cs CumulativeTestCaseSpec) { maybeRandomlySkip(t) // Skip comparing outputs, if there are any newly created objects.
why only for these objects? what about functions, schemas, types, and databases?
pkg/sql/schemachanger/sctest/backup.go line 482 at r5 (raw file):
sqlutils.MatrixToStr(backupContents), pe.GetExplain()) } else { t.Log("Skipping comparison of SQL afterward's as requested.")
super nit: "afterwards"
fqazi
force-pushed
the
createSequenceFin
branch
2 times, most recently
from
233d983
to
319323c
Compare
There was a problem hiding this comment.
Reviewable status:
Previously, rafiss (Rafi Shamim) wrote…
are the next two commits after this examples of things that would have failed before?
Yes, and I re-ordered the commits to make enabling last.
pkg/sql/schemachanger/scplan/internal/rules/current/helpers.go line 48 at r2 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
this could lead to confusion. the comment has "descriptorDataIsBeingAdded indicates that descriptor data is being added" but the function is named
descriptorDataIsNotBeingAdded. it looks likedescriptorDataIsNotBeingAddedis correct.
Done.
Good point, and reworded this to:
// descriptorDataIsNotBeingAdded indicates if we are operating on a descriptor
// that already exists and was not created in the current transaction. This is
// determined by detecting if the data element is public, and not going from
// absent to public which newly created descriptors will.
pkg/sql/schemachanger/sctest/backup.go line 117 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
why only for these objects? what about functions, schemas, types, and databases?
Good point add all other object types here.
pkg/sql/schemachanger/sctest/backup.go line 482 at r5 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
super nit: "afterwards"
Done.
Previously, when adding / dropping a relation in a single transaction, we did not properly account for data elements. This could lead to DROP operations doing work when they shouldn't in the same transaction. This would happen because normally the DROP would no-op steps because they were added by the create. To address this, this patch will add data elements for CREATE SEQUENCE, and modify rules to skip the two version invariant allowing these cases to work properly. Release note: None
Previously, we only had coverage for a creating a sequence on its own in the declarative schemachanger. This patch adds, a test for creating / dropping sequences in a single txn. Release note: None
Previously, we did not have a test case to cover the scenario where a sequence was created and used by a column in a single txn. To address this, this patch will add such a test. Release note: None
Previously, the backup and restore tests ran on all of the end to end tests file, however when we are creating new objects the intermediate states are never backed up. As a result, these tests are expected to not restore perfectly. To address this, this patch will avoid comparing CREATE statements if we have a multi-statement end to end file where new objects are made. Release note: None
Previously, CREATE SEQUENCE was disabled in the declarative schema changer because we were missing sufficient test coverage and not comfortable enabling it by default. This patch enables, CREATE SEQUENCE in the declarative schema changer again. Release note (sql change): CREATE SEQUENCE is now enabled by default in the declarative schema changer.
fqazi
force-pushed
the
createSequenceFin
branch
from
319323c
to
80870ce
Compare
|
@rafiss TFTR! bors r+ |
|
Build succeeded: |
Previously, we did fully implement CREATE SEQUENCE in the declarative schema changer but had it disabled by default since we were missing this PR will do the following:
Informs: #117648
Epic: CRDB-31282