sql/schemachanger: support for create sequence inside the declarative schema changer #104350

fqazi · 2023-06-05T18:25:06Z

This patch implements CREATE SEQUENCE in the declarative schema changer. The first three commits can be ignored and are included in (#104348, which should be reviewed and merged first). This patch will:

Skip validation/backfill for descriptors in an added state
Fixes prefix resolution creating new objects, where two-part names were not handled properly before
Adds support for CREATE sequence in the declarative schema changer

Fixes: #104351

cockroach-teamcity · 2023-06-05T18:25:24Z

This change is

postamar

I took an early look at the last 3 commits and left some preliminary comments. Nicely done. Can you add support for ADD COLUMN SERIAL and friends, if that's straightforward? I can't remember too well, but it might be. Don't bother if it's out of scope, but it'd be a nice test case.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_not_null.go

pkg/sql/schemachanger/scpb/elements.proto

fqazi · 2023-06-08T16:04:41Z

I took an early look at the last 3 commits and left some preliminary comments. Nicely done. Can you add support for ADD COLUMN SERIAL and friends, if that's straightforward? I can't remember too well, but it might be. Don't bother if it's out of scope, but it'd be a nice test case.

We have a tracking issue for the remaining ADD COLUMN work, but I'll definitely circle back as a follow on PR

fqazi · 2023-06-13T15:51:57Z

@postamar This is ready for another look, its rebased and now uses TableData elements for decision making so more future proof.

Xiang-Gu · 2023-06-13T17:55:43Z

pkg/sql/schemachanger/scbuild/build.go

+	seenDescriptors := screl.AllTargetDescIDs(ts)
+	// Elements without metadata can only be retained, if some other descriptor
+	// is referencing them. We may have multiple index data and table data's,
+	// but not all of them are relevant if the primary descriptor ID never shows.
+	for _, ex := range extraTargets {
+		if seenDescriptors.Contains(screl.GetDescID(ex.t.Element())) {
+			ts.Targets = append(ts.Targets, ex.t)
+			initial = append(initial, ex.e.initial)
+			current = append(current, ex.e.current)


I didn't understand this part: So in the loop-above, we iterate all elements and populate extraTargets, which are all the elements that have empty metadata but should be retained. Here, we are looping over those extra targets and append them into the TargetState if they are contained in seenDescriptors. Why would any of them be contained in seenDescriptors?

I guess in general those comments are a bit too high level to me still.

// We are going to retain certain elements for metadata purposes, like // TableData/IndexData elements, which will allow the declarative schema // changer to know if a given table is empty. Once these elements exist // the two-version invariant is applied when making mutations to elements. // Only emit data elements for descriptors if they are references with // some transition.

Xiang-Gu · 2023-06-13T17:58:56Z

pkg/sql/schemachanger/scbuild/build.go

 	for _, e := range bs.output {
-		if e.metadata.Size() == 0 {
+		if e.metadata.Size() == 0 && !shouldElementBeRetainedWithoutMetadata(e.element) {


Please help me understand the overall purpose of including those TableData element and IndexData element in the TargetState even if they are not added/dropped in the builder. In other words, how exactly do they help "hinting" if a backfill is needed

They allow us to know if a table is empty, which allows us to skip backfill steps completely. The other alternative is to use the DESCRIPTOR_ADDED state, but that breaks if we enter a more transactional world. Since you should be able to insert on those

Updated the comments here:
// Exceptions are TableData/IndexData elements which allow our planning
// execution to skip certain transitions and fences like the two version
// invariant. We will only keep these for descriptors going through
// a transition.

postamar

Nicely done! I only have minor complaints.

postamar · 2023-06-15T13:03:53Z

pkg/sql/catalog/rewrite/rewrite.go

+			data.TableID = rewrite.ID
+			data.DatabaseID = rewrite.ParentID
+			continue
+		}


This rewrite makes sense but should this be in the switch case below? Not a rethorical question. Or is this case somehow different? If so I can't see it and this deserves commentary perhaps.

@postaar It's because the parentID is not contained within the map that we are using, so it ends up special cased. Let me add the comment:
// Since the parent database ID is never written in the descriptorRewrites
// map we need to special case certain elements that need their ParentID
// re-written.

pkg/sql/schemachanger/scpb/state.go

pkg/sql/schemachanger/scrun/scrun.go

pkg/sql/schemachanger/scbuild/build.go

pkg/sql/schemachanger/scdecomp/decomp.go

postamar · 2023-06-15T13:55:07Z

pkg/sql/schemachanger/testdata/explain/create_sequence

+ │         │    ├── ABSENT → PUBLIC SequenceOption:{DescID: 104 (sq1+), Name: "MAXVALUE"}
+ │         │    ├── ABSENT → PUBLIC SequenceOption:{DescID: 104 (sq1+), Name: "START"}
+ │         │    ├── ABSENT → PUBLIC SequenceOption:{DescID: 104 (sq1+), Name: "VIRTUAL"}
+ │         │    ├── ABSENT → PUBLIC SequenceOption:{DescID: 104 (sq1+), Name: "CACHE"}


This is out of scope for this PR but come to think of it, we'll also have to bind the value to a new Value attr in screl at some point, to handle ALTER SEQUENCE DDLs.

Agree that will need to be revisited

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go

pkg/sql/schemachanger/scexec/scmutationexec/references.go

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msirek, @postamar, and @Xiang-Gu)

pkg/sql/catalog/dbdesc/database_desc_builder.go line 165 at r9 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't the second arg be 0?

Good catch

pkg/sql/schemachanger/scbuild/build.go line 112 at r8 (raw file):

Previously, postamar (Marius Posta) wrote…

Call TargetIsLinkedToSchemaChange instead?

Done.

pkg/sql/schemachanger/scbuild/build.go line 486 at r14 (raw file):

Previously, postamar (Marius Posta) wrote…

I wonder if perhaps we'd be better served by adding a boolean override flag in the metadata protobuf instead, that would be set by scdecomp for TableData/IndexData elements. I'm not sure if it's actually better, perhaps not. Your approach is fine in the meantime, perhaps I'll explore this after this merges.

I think that is pretty reasonable and would generalize this a lot better.

pkg/sql/schemachanger/scdecomp/decomp.go line 245 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

s/keyStr/valueStr/

Done.

pkg/sql/schemachanger/scdecomp/decomp.go line 248 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

You need to also return when the value is 0 (or whatever the default is) for integer values. Perhaps pass an extra defaultValue argument to addSequenceOption and compare the value to that?

Done.

pkg/sql/schemachanger/scexec/dependencies.go line 105 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

This is fine, though FYI I wouldn't have minded simply adding the InitializeSequence method to scexec.Catalog, because I'm not sure how much this interface will grow in the future.

Done.

pkg/sql/schemachanger/scpb/state.go line 99 at r8 (raw file):

Previously, postamar (Marius Posta) wrote…

Call TargetIsLinkedToSchemaChange instead?

Done.

pkg/sql/schemachanger/scpb/state.go line 150 at r8 (raw file):

Previously, postamar (Marius Posta) wrote…

I like this.

Done.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_not_null.go line 34 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

What about other constraints? Shouldn't they also have this check?

Yes, they, need the same check let me update them as well.

Done.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence.go line 42 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

Instead of having a pointer to an int, can we have an extra UseRestartWith boolean field instead? I'm not sure how well an *int64 serializes in YAML (which is how we render EXPLAIN diagrams) and this isn't tested. For consistency, you might want to do the same in the element model.

Good point, that's much cleaner. The element model can have a similar change too.

pkg/sql/schemachanger/scplan/internal/opgen/opgen_sequence_option.go line 1 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

We're in 2023.

Done.

pkg/sql/schemachanger/scplan/internal/scstage/build.go line 345 at r10 (raw file):

Previously, postamar (Marius Posta) wrote…

This change makes sense but I'm a bit concerned about the added computational complexity that this loop adds. Can we pre-compute the anyRemainingOpsCanFail property for each node instead? Define anyRemainingOpsCanFail := make(map[*screl.Node]bool), then for each target, start with the sink node and do a depth-first traversal going backwards along each op edge, if you've verified oe.CanFail() at least once in the path to the sink node then set anyRemainingOpsCanFail[oe.From()] = true and vice-versa.

Done.

pkg/sql/schemachanger/scrun/scrun.go line 277 at r8 (raw file):

Previously, postamar (Marius Posta) wrote…

Call TargetIsLinkedToSchemaChange instead?

Done.

pkg/sql/logictest/testdata/logic_test/crdb_internal line 866 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

Remind me, what triggered this change?

This is triggered by MaybeSplit.* from the primary index. I guess the question is if we want this temporary admin split or not since for a sequence it's only a single row.

pkg/sql/schemachanger/scbuild/testdata/unimplemented_create line 0 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

Hooray :)

Done.

To support the ability of skipping operations when data exists, we will add support for including data targets within plans even if they have no transitions. These elements then can be used by rules to decide if backfill or the two version invariant are required. This patch, will add logic to do this detection. Epic: none Release note: None

Previously, if an existing job was missing the TableData element, the new job would not properly handle the current set of rules in the mixed version state. This patch adds, a migration to inject those elements. Epic: none Release note: None

fqazi · 2023-06-28T02:23:40Z

@postamar ready for another look

postamar

You're missing at least one end-to-end test case involving CREATE SEQUENCE.

postamar · 2023-06-28T17:53:00Z

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go

+	addSequenceOption(tree.SeqOptStart, tempSequenceOpts.Start)
+	addSequenceOption(tree.SeqOptVirtual, tempSequenceOpts.Virtual)
+	addSequenceOption(tree.SeqOptCache, tempSequenceOpts.CacheSize)
+	addSequenceOption(tree.SeqOptAs, tempSequenceOpts.AsIntegerType)


Shouldn't you be reusing the same logic as in scdecomp here for the sequence options?

postamar · 2023-06-28T17:54:47Z

pkg/sql/schemachanger/scdecomp/decomp.go

+				if value == nil {
+					return
+				}
+				keyStr := fmt.Sprintf("%v", value)


This hasn't been fixed.

postamar · 2023-06-28T17:57:40Z

pkg/sql/schemachanger/scplan/internal/scstage/build.go

+			}
+
+		}
+	}


Please move this logic into a top-level function which you'll call in the struct initialization above: bc := buildContext{ ... }.

postamar · 2023-06-28T18:00:35Z

pkg/sql/schemachanger/testdata/explain_verbose/create_sequence

+            │     SequenceID: 104
+            │
+            └── • MarkDescriptorAsPublic
+                  DescriptorID: 104


This file shouldn't exist.

postamar · 2023-06-28T18:01:01Z

...hemachanger/testdata/end_to_end/drop_column_create_index_separate_statements/create_sequence

+                ├── SetIndexName {"IndexID":1,"Name":"primary","TableID":104}
+                ├── MakeValidatedPrimaryIndexPublic {"IndexID":1,"TableID":104}
+                ├── InitSequence {"SequenceID":104}
+                └── MarkDescriptorAsPublic {"DescriptorID":104}


This file shouldn't exist.

postamar · 2023-06-28T18:01:13Z

...emachanger/testdata/end_to_end/create_schema_drop_schema_separate_statements/create_sequence

+EXPLAIN (ddl, shape) CREATE SEQUENCE sq1 MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 32;
+----
+Schema change plan for CREATE SEQUENCE ‹defaultdb›.‹public›.‹sq1› MINVALUE 1 MAXVALUE 9223372036854775807 INCREMENT 1 START 32;
+ └── execute 1 system table mutations transaction


This file shouldn't exist.

fqazi · 2023-06-28T18:08:20Z

You're missing at least one end-to-end test case involving CREATE SEQUENCE.

Ah, the directory structure changed for this stuff, let me move it over and regenerate it.

postamar

At this point if the CI's happy I'm happy. I've looked at enough iterations of this PR that I'm starting to lose track of things. I don't want to block this, though, and I'm feeling pretty good about this change.. Feel free to have this looked at by a fresh pair of eyes, depending on how confident you are about this change. It's up to you.

postamar · 2023-06-29T13:19:52Z

pkg/sql/schemachanger/testdata/end_to_end/create_sequence/create_sequence.definition

+persist all catalog changes to storage
+adding table for stats refresh: 104
+# end PreCommitPhase
+commit transaction #1


You don't actually need an expected output in the .definition file. Please remove.

fqazi · 2023-06-29T13:28:51Z

At this point if the CI's happy I'm happy. I've looked at enough iterations of this PR that I'm starting to lose track of things. I don't want to block this, though, and I'm feeling pretty good about this change.. Feel free to have this looked at by a fresh pair of eyes, depending on how confident you are about this change. It's up to you.

Thanks for the patience in reviewing this, and I appreciate the feedback it made the entire thing much better. I'm gonna make sure CI is clear then merge it with the remaining fixes.

Previously, we would always generate operations to backfill and validate on newly created tables (with data elements). This will prevent us from being able to generate single stage plans for creating relations. This patch removes those checks for added tables, and allows data elements to be emitted as targets. Epic: none Release note: None

Previously, the two version invariant was enforced for all non-dropped descriptors. When creating new descriptors if they are going to be decomposed into individual elements, then similar logic needs to skip added descriptors. To address this, this patch disables these rules for added descriptors. Epic: none Release note: None

Previously, when trying to resolve prefixes for newly created objects, we didn't handle two parts names properly in the declarative schema changer. This, patch fixes this logic to match the resolver. Epic: none Release note: None

Previously, the CREATE SEQUENCE command only worked in the legacy schema changer. This patch, adds support for creating sequences in declarative schema changer. Epic: None Release note: None

Previously, our CREATE SEQUENCE support for the declarative schema changer fell back if an OWNED BY clause is specified. To address this patch adds support for owned by clause. Epic: none Release note: None

fqazi

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @msirek, @postamar, and @Xiang-Gu)

a discussion (no related file):

Previously, postamar (Marius Posta) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

You don't actually need an expected output in the .definition file. Please remove.

Done.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go line 110 at r14 (raw file):

Previously, postamar (Marius Posta) wrote…

This seems like it should be in the previous commit.

Done.

pkg/sql/schemachanger/scbuild/internal/scbuildstmt/create_sequence.go line 135 at r35 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't you be reusing the same logic as in scdecomp here for the sequence options?

Done.

pkg/sql/schemachanger/scdecomp/decomp.go line 245 at r13 (raw file):

Previously, postamar (Marius Posta) wrote…

This hasn't been fixed.

Ugh, I fixed the comment but forgot to fix the variable name.

pkg/sql/schemachanger/scplan/internal/scstage/build.go line 100 at r35 (raw file):

Previously, postamar (Marius Posta) wrote…

Please move this logic into a top-level function which you'll call in the struct initialization above: bc := buildContext{ ... }.

Done.

pkg/sql/schemachanger/testdata/end_to_end/create_schema_drop_schema_separate_statements/create_sequence line 7 at r35 (raw file):

Previously, postamar (Marius Posta) wrote…

This file shouldn't exist.

Done.

pkg/sql/schemachanger/testdata/end_to_end/drop_column_create_index_separate_statements/create_sequence line 130 at r35 (raw file):

Previously, postamar (Marius Posta) wrote…

This file shouldn't exist.

Done.

pkg/sql/schemachanger/testdata/explain_verbose/create_sequence line 778 at r35 (raw file):

Previously, postamar (Marius Posta) wrote…

This file shouldn't exist.

Done.

fqazi · 2023-06-29T15:46:15Z

bors r+

craig · 2023-06-29T16:23:10Z

Build succeeded:

Bazel Essential CI (Cockroach)

fqazi changed the title ~~sql/schemachanger: infrastructure for supporting CREATE statements #104348~~ sql/schemachanger: support for create sequence inside the declarative schema changer Jun 6, 2023

fqazi marked this pull request as ready for review June 6, 2023 16:00

fqazi requested a review from a team as a code owner June 6, 2023 16:00

postamar reviewed Jun 6, 2023

View reviewed changes

pkg/sql/schemachanger/scplan/internal/opgen/opgen_column_not_null.go Show resolved Hide resolved

pkg/sql/schemachanger/scpb/elements.proto Show resolved Hide resolved

fqazi force-pushed the createSequenceProto branch 2 times, most recently from 838ce29 to fad8be8 Compare June 8, 2023 20:02

fqazi requested a review from postamar June 8, 2023 20:03

fqazi force-pushed the createSequenceProto branch from fad8be8 to aea6cc0 Compare June 9, 2023 23:06

postamar mentioned this pull request Jun 12, 2023

sql/schemachanger: infrastructure for supporting CREATE statements #104348

Closed

fqazi marked this pull request as draft June 12, 2023 17:52

fqazi force-pushed the createSequenceProto branch from aea6cc0 to 64eb7f3 Compare June 13, 2023 15:49

fqazi marked this pull request as ready for review June 13, 2023 15:50

fqazi requested a review from a team as a code owner June 13, 2023 15:50

fqazi requested a review from msirek June 13, 2023 15:50

fqazi force-pushed the createSequenceProto branch from 64eb7f3 to 66d8186 Compare June 13, 2023 15:51

Xiang-Gu reviewed Jun 13, 2023

View reviewed changes

fqazi force-pushed the createSequenceProto branch 6 times, most recently from 247eba1 to 32a2927 Compare June 14, 2023 14:09

fqazi requested a review from Xiang-Gu June 14, 2023 14:13

postamar suggested changes Jun 15, 2023

View reviewed changes

fqazi force-pushed the createSequenceProto branch 2 times, most recently from be18379 to d194d3f Compare June 16, 2023 13:43

fqazi force-pushed the createSequenceProto branch from 3c06e15 to 248f090 Compare June 27, 2023 03:23

fqazi commented Jun 27, 2023

View reviewed changes

fqazi force-pushed the createSequenceProto branch 3 times, most recently from 8fe8cf4 to abe3515 Compare June 27, 2023 17:37

fqazi added 2 commits June 27, 2023 16:04

fqazi force-pushed the createSequenceProto branch 2 times, most recently from 39bfbae to 44153a6 Compare June 28, 2023 01:32

postamar suggested changes Jun 28, 2023

View reviewed changes

fqazi force-pushed the createSequenceProto branch 4 times, most recently from 75611df to fb0f4c9 Compare June 29, 2023 13:21

postamar approved these changes Jun 29, 2023

View reviewed changes

fqazi force-pushed the createSequenceProto branch from fb0f4c9 to 827a139 Compare June 29, 2023 13:31

fqazi added 5 commits June 29, 2023 09:33

sql/schemachanger: add support for the CREATE SEQUENCE

7be2d93

Previously, the CREATE SEQUENCE command only worked in the legacy schema changer. This patch, adds support for creating sequences in declarative schema changer. Epic: None Release note: None

sql/schemachanger: add support for OWNED BY on CREATE SEQUENCE

74edfa9

Previously, our CREATE SEQUENCE support for the declarative schema changer fell back if an OWNED BY clause is specified. To address this patch adds support for owned by clause. Epic: none Release note: None

fqazi force-pushed the createSequenceProto branch from 827a139 to 74edfa9 Compare June 29, 2023 13:34

fqazi commented Jun 29, 2023

View reviewed changes

craig bot merged commit 2b66f95 into cockroachdb:master Jun 29, 2023

rmloveland mentioned this pull request Jul 10, 2024

Add CREATE SEQUENCE to declarative schema changer cockroachdb/docs#18727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/schemachanger: support for create sequence inside the declarative schema changer #104350

sql/schemachanger: support for create sequence inside the declarative schema changer #104350

fqazi commented Jun 5, 2023 •

edited

Loading

cockroach-teamcity commented Jun 5, 2023

postamar left a comment

fqazi commented Jun 8, 2023

fqazi commented Jun 13, 2023

Xiang-Gu Jun 13, 2023

fqazi Jun 13, 2023

Xiang-Gu Jun 13, 2023

fqazi Jun 13, 2023

fqazi Jun 13, 2023

postamar left a comment

postamar Jun 15, 2023

fqazi Jun 16, 2023 •

edited

Loading

postamar Jun 15, 2023

fqazi Jun 16, 2023

fqazi left a comment

fqazi commented Jun 28, 2023

postamar left a comment

postamar Jun 28, 2023

postamar Jun 28, 2023

postamar Jun 28, 2023

postamar Jun 28, 2023

postamar Jun 28, 2023

postamar Jun 28, 2023

fqazi commented Jun 28, 2023

postamar left a comment

postamar Jun 29, 2023

fqazi commented Jun 29, 2023

fqazi left a comment

fqazi commented Jun 29, 2023

craig bot commented Jun 29, 2023

+              			}
+              		}
+              	}

sql/schemachanger: support for create sequence inside the declarative schema changer #104350

sql/schemachanger: support for create sequence inside the declarative schema changer #104350

Conversation

fqazi commented Jun 5, 2023 • edited Loading

cockroach-teamcity commented Jun 5, 2023

postamar left a comment

Choose a reason for hiding this comment

fqazi commented Jun 8, 2023

fqazi commented Jun 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi left a comment

Choose a reason for hiding this comment

fqazi commented Jun 28, 2023

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi commented Jun 28, 2023

postamar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fqazi commented Jun 29, 2023

fqazi left a comment

Choose a reason for hiding this comment

fqazi commented Jun 29, 2023

craig bot commented Jun 29, 2023

fqazi commented Jun 5, 2023 •

edited

Loading

fqazi Jun 16, 2023 •

edited

Loading