Skip to content

sql: encode and decode sequence regclasses, and allow sequence renaming#59864

Merged
craig[bot] merged 4 commits intocockroachdb:masterfrom
the-ericwang35:ericwang/encode_decode_rename
Feb 18, 2021
Merged

sql: encode and decode sequence regclasses, and allow sequence renaming#59864
craig[bot] merged 4 commits intocockroachdb:masterfrom
the-ericwang35:ericwang/encode_decode_rename

Conversation

@the-ericwang35
Copy link
Copy Markdown

This patch builds upon #59396,
where we overloaded sequence operators to accept a regclass.
Now that sequence operators can accept a regclass,
this patch starts using this change by storing sequences
via their ID::regclass, updating back references to distinguish
between references via names and IDs, and then transforming
these IDs back to their fully qualified names for printing.
This patch also starts allowing sequences that are referenced only
via ID to be renamed.
Lastly, tests were added to demonstrate all of this new functionality.

Release note (sql change): encode and decode sequence regclasses,
and allow renaming sequences

@the-ericwang35 the-ericwang35 requested a review from a team February 5, 2021 20:51
@the-ericwang35 the-ericwang35 requested a review from a team as a code owner February 5, 2021 20:51
@the-ericwang35 the-ericwang35 requested review from adityamaru and removed request for a team February 5, 2021 20:51
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@the-ericwang35 the-ericwang35 removed request for a team and adityamaru February 5, 2021 20:51
@the-ericwang35 the-ericwang35 marked this pull request as draft February 5, 2021 20:51
@the-ericwang35 the-ericwang35 requested a review from a team February 6, 2021 00:32
@the-ericwang35 the-ericwang35 marked this pull request as ready for review February 6, 2021 00:32
Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 21 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)


pkg/sql/catalog/schemaexpr/column.go, line 324 at r1 (raw file):

		// Swap out this node to use the qualified table name for the sequence.
		return false, &tree.AnnotateTypeExpr{
			Type:       types.String,

I think we want this to say regclass.

Copy link
Copy Markdown
Author

@the-ericwang35 the-ericwang35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/catalog/schemaexpr/column.go, line 324 at r1 (raw file):

Previously, ajwerner wrote…

I think we want this to say regclass.

Done, as discussed the formatting now looks something like 'test.public.seq':::STRING::REGCLASS.

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Very close

Reviewed 6 of 21 files at r1, 2 of 4 files at r2, 8 of 16 files at r3, 14 of 18 files at r4, 9 of 9 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @the-ericwang35)


pkg/ccl/backupccl/restore_planning.go, line 1052 at r5 (raw file):

				return err
			}
			*expr = newExpr

you can just do it right here in the same iteration loop.


pkg/sql/rename_database.go, line 317 at r5 (raw file):

Quoted 5 lines of code…
				// We only don't allow this if the database name is in there.
				// This is always the last argument.
				if tree.Name(parsedSeqName.Parts[parsedSeqName.NumParts-1]).Normalize() == tree.Name(dbName).Normalize() {
					return false, column.GetName(), nil
				}

This is pretty 🤦 - I'm inclined to remove it but let's do it as a follow up.


pkg/sql/sequence.go, line 607 at r5 (raw file):

		var seqDesc *tabledesc.Mutable
		var err error

nit: lower the error checking out of this branch.


pkg/sql/sequence.go, line 649 at r5 (raw file):

	}

	if len(seqIdentifiers) > 0 && byID {

This probably deserves some commentary explaining what's going on.


pkg/sql/catalog/descpb/structured.proto, line 972 at r5 (raw file):

    repeated uint32 column_ids = 3 [(gogoproto.customname) = "ColumnIDs",
             (gogoproto.casttype) = "ColumnID"];
    // Indicates whether the relation is referenced via its name or its ID.

Maybe a bit more commentary about what it means for it to be referenced by name or by id.


pkg/sql/catalog/schemaexpr/column.go, line 290 at r5 (raw file):

// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with
// the sequence's fully qualified names.

This deserves more commentary explaining what it actually does.


pkg/sql/catalog/schemaexpr/column.go, line 291 at r5 (raw file):

// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with
// the sequence's fully qualified names.
func replaceWithSequenceNames(

There's nothing that makes this actually about sequences... If the expression were 8:::REGCLASS it'd end up here, right? I think that's find and good but means that we should make these function names more generic.


pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):

  uint32 sequence_id = 3 [(gogoproto.customname) = "SequenceID", (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb.ID"];
  Type type = 4;
  bool by_id = 5 [(gogoproto.customname) = "ByID"];

commentary please


pkg/util/sequence/sequence.go, line 127 at r5 (raw file):

Quoted 5 lines of code…
// ReplaceSequenceNamesWithIDs walks the given expression, and replaces
// any sequence names in the expression by their IDs instead.
// e.g. nextval('foo') => nextval(123::regclass)
func ReplaceSequenceNamesWithIDs(
	defaultExpr tree.Expr, nameToID map[string]int64,

This feels very much like the sort of thing that could be easy to unit test. Honestly, so too could the other function in this package. I think it's worth it.

Copy link
Copy Markdown
Author

@the-ericwang35 the-ericwang35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)


pkg/ccl/backupccl/restore_planning.go, line 1052 at r5 (raw file):

Previously, ajwerner wrote…

you can just do it right here in the same iteration loop.

Done.


pkg/sql/sequence.go, line 607 at r5 (raw file):

Previously, ajwerner wrote…

nit: lower the error checking out of this branch.

I realized declaring err is not even needed so I just removed it.


pkg/sql/sequence.go, line 649 at r5 (raw file):

Previously, ajwerner wrote…

This probably deserves some commentary explaining what's going on.

Done.


pkg/sql/catalog/descpb/structured.proto, line 972 at r5 (raw file):

Previously, ajwerner wrote…

Maybe a bit more commentary about what it means for it to be referenced by name or by id.

Done.


pkg/sql/catalog/schemaexpr/column.go, line 290 at r5 (raw file):

Previously, ajwerner wrote…

This deserves more commentary explaining what it actually does.

Done.


pkg/sql/catalog/schemaexpr/column.go, line 291 at r5 (raw file):

Previously, ajwerner wrote…
// replaceWithSequenceNames replaces occurrences of sequence regclasses in an expression with
// the sequence's fully qualified names.
func replaceWithSequenceNames(

There's nothing that makes this actually about sequences... If the expression were 8:::REGCLASS it'd end up here, right? I think that's find and good but means that we should make these function names more generic.

Yep that's a good point, renamed to replaceIDsWithFQNames


pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):

Previously, ajwerner wrote…

commentary please

Done.


pkg/util/sequence/sequence.go, line 127 at r5 (raw file):

Previously, ajwerner wrote…
// ReplaceSequenceNamesWithIDs walks the given expression, and replaces
// any sequence names in the expression by their IDs instead.
// e.g. nextval('foo') => nextval(123::regclass)
func ReplaceSequenceNamesWithIDs(
	defaultExpr tree.Expr, nameToID map[string]int64,

This feels very much like the sort of thing that could be easy to unit test. Honestly, so too could the other function in this package. I think it's worth it.

Yep, I added some basic tests for all of the functions in this file in a new test file sequence_test.go, I can add more later if needed.

Copy link
Copy Markdown
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the typo nit, then bors it!

:lgtm:

Reviewed 1 of 31 files at r10, 4 of 16 files at r11, 9 of 18 files at r12, 20 of 20 files at r13.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)


pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):

Previously, the-ericwang35 (Eric Wang) wrote…

Done.

seqiemce

nit: start this with ByID

Eric Wang added 4 commits February 18, 2021 10:22
This patch builds upon #59396,
where we overloaded sequence operators to accept a regclass.
Now that sequence operators can accept a regclass,
this patch starts using this change by storing sequences
via their `ID::regclass`, updating back references to distinguish
between references via names and IDs, and then transforming
these IDs back to their fully qualified names for printing.
This patch also starts allowing sequences that are referenced only
via ID to be renamed.
Lastly, tests were added to demonstrate all of this new functionality.

Release note (sql change): encode and decode sequence regclasses,
and allow renaming sequences
Before, we did not allow sequences to be renamed if
they were depended on by, since the dependency used
the sequence's name. Now that we've started to use
IDs for references instead, we want to allow renaming
of sequences that are only referenced by their ID.

Release note (sql change): allow renaming of sequences referenced by ID
The previous commit allowed sequences referenced only by IDs
to be renamed.
This patch further adds to this by converting all sequence
references in DEFAULT expressions to be referenced by ID,
even if the expression is given a sequence name string.
This means that now, all new sequences can be renamed.

Release note (sql change): convert sequences referenced by name to ID,
allow all renaming
This patch adds a version gate to this feature,
so that all DEFAULT expressions created in the latest
version will references sequences by ID. And, all
new sequences created will be able to be renamed.
However, existing sequences and expressions created in
previous versions will still be referenced by name, and
cannot be renamed.

Release note (sql change): add version gate to sequence regclass feature
Copy link
Copy Markdown
Author

@the-ericwang35 the-ericwang35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajwerner)


pkg/sql/schemachanger/scpb/scpb.proto, line 86 at r5 (raw file):

Previously, ajwerner wrote…

seqiemce

nit: start this with ByID

Wow that spelling was not even close, fixed.

@the-ericwang35
Copy link
Copy Markdown
Author

TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Feb 18, 2021

Build succeeded:

@craig craig bot merged commit 19726fa into cockroachdb:master Feb 18, 2021
@the-ericwang35 the-ericwang35 deleted the ericwang/encode_decode_rename branch February 18, 2021 17:04
craig bot pushed a commit that referenced this pull request Mar 22, 2021
61439: sql: reference sequences used in views by their IDs r=ajwerner a=the-ericwang35

Followup to #59864, fixes #61205.
Previously, a change was introduced to store sequences
used in DEFAULT expressions to be referenced by their
IDs instead of their names, to allow sequence renaming.
In this patch, we want to do something similar by
rewriting sequence references in views to be
referenced by their IDs. This allows sequences
used in views to be renamed.

Another thing this PR does is change the output of sequences
to look like `'s'::REGCLASS` rather than `'s':::STRING::REGCLASS`,
since we're no longer type checking them (since it's unnecessary).

Release note (sql change): reference sequences used in views
by their IDs to allow these sequences to be renamed.

61682: kvserver: teach replicateQueue to replace dead/decommisioning non-voters r=aayushshah15 a=aayushshah15

The PR is very similar to #59403.

Previously, non-voters on dead or decommissioning nodes would not get
removed or replaced. This commit fixes this behaviour.

Resolves #61626

Release justification: fixes limitation where dead/decommissioning
non-voters would not be removed or replaced
Release note: None


61967: kvserver: stop spuriously refusing non-voters in replicateQueue.shouldQueue() r=aayushshah15 a=aayushshah15

Before this commit, the `replicateQueue` would refuse to queue up a
replica into the queue (in its `shouldQueue` method) for the rebalancing
case unless it could verify that a voting replica needed to be
rebalanced. This was an unfortunate oversight since it meant that unless
there was a voting replica to be rebalanced, non-voters would not get
rebalanced by the queue.

This commit fixes this bug.

Noticed while debugging a flakey test for #61682

Release justification: bug fix
Release note: None

62009: build: perform `bazel clean --expunge` on `make clean` r=rickystewart a=rickystewart

Release note: None

62186: sql: ensure user has correct privileges when adding/removing regions r=richardjcai a=arulajmani

Previously we did not account for privileges on database objects when
adding the default locality config on first region add or removing the
locality config on last region drop properly. In particular, we weren't
adding/removing the locality config on any descriptor that wasn't
visible to the user. This is bad because our validation logic expects
only and all objects in multi-region databases to have a valid locality
config. This means future accesses to such descriptors would fail
validation.

The root of this problem was the API choice here, `ForEachTableDesc`,
which filters out invisible descriptors. This patch instead switches
to using `forEachTableInMultiRegionDatabase`. While here, instead of
issuing separate requests for every table, I refactored this thing to
issue a single batch request instead.

Now that we view all the descriptors inside the database, unfiltered,
we perform privilege checks on them before proceeding with the add/drop
operation. In particular, the semantics are:
- admin users are allowed to add/drop regions as they wish.
- non admin-users require the CREATE privilege or must have ownership
on all the objects inside the database.

Closes #61003

Release note (sql change): `ALTER DATABASE .. SET PRIMARY REGION` now
requires both CREATE and ZONECONFIG privilege on all  objects inside
the database when adding the first region to the database. Same for
dropping the last region using `ALTER DATABASE ... DROP REGION`.

Co-authored-by: Eric Wang <ericw@cockroachlabs.com>
Co-authored-by: Aayush Shah <aayush.shah15@gmail.com>
Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>
Co-authored-by: arulajmani <arulajmani@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: sequences created by SERIAL have weird semantics with regards to database

3 participants