Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clean up various non-unique indexes #3530

Merged
merged 4 commits into from
Jul 10, 2023
+56 −45
Merged

clean up various non-unique indexes #3530

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9214f15
first cut
davepacheco Jul 7, 2023
47bc117
fix case for consistency
davepacheco Jul 7, 2023
e9c805e
nit
davepacheco Jul 7, 2023
799b931
more comments
davepacheco Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
96 changes: 51 additions & 45 deletions common/src/sql/dbinit.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,13 @@ CREATE TABLE omicron.public.sled (
);

/* Add an index which lets us look up sleds on a rack */
CREATE INDEX ON omicron.public.sled (
rack_id
) WHERE time_deleted IS NULL;

CREATE INDEX ON omicron.public.sled (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This index is superfluous because CockroachDB always has an index on the primary key. (Note that having an index on "rack_id" and one on "id" is not the same as having one on (rack_id, id).)

Copy link
Contributor

Choose a reason for hiding this comment

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

This index also includes a filter predicate time_deleted IS NULL which makes it a partial index no? Perhaps not super relevant for this particular table but in the limit there could be more old/deleted records that a partial index could speed up access for only live records.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for pointing that out. I had missed that. Are you saying that even if CockroachDB already indexes on "id", it might be beneficial to have an index on "id" limited only to live rows because querying it could be faster? Yeah, tThat's definitely possible. I think we probably want to wait and see if that's true, though, since it's somewhat speculative and has downsides (i.e., the extra disk space and memory usage and cost to update it on writes).

Copy link
Contributor

Choose a reason for hiding this comment

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

They have a doc about it that says as much, but as you say, the benefit is an empirical question. They frame the advantages in contrast with a full index, as opposed to no index/one you're stuck with regardless.

CREATE UNIQUE INDEX ON omicron.public.sled (
rack_id,
id
) WHERE
time_deleted IS NULL;
) WHERE time_deleted IS NULL;
Comment on lines +110 to +113
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the cases mentioned in the PR description. If we want to paginate the sleds in a rack, we need the sled id in this index, and then it can also be unique.


CREATE TYPE omicron.public.sled_resource_kind AS ENUM (
-- omicron.public.Dataset
-- omicron.public.dataset
'dataset',
-- omicron.public.service
'service',
Expand Down Expand Up @@ -152,8 +148,9 @@ CREATE TABLE omicron.public.sled_resource (
);

-- Allow looking up all resources which reside on a sled
CREATE INDEX ON omicron.public.sled_resource (
sled_id
CREATE UNIQUE INDEX ON omicron.public.sled_resource (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the cases mentioned in the PR description. If we want tp aginate the sled_resources on a sled, we need the "id" here, too.

sled_id,
id
);

/*
Expand All @@ -178,14 +175,10 @@ CREATE TABLE omicron.public.switch (
);

/* Add an index which lets us look up switches on a rack */
CREATE INDEX ON omicron.public.switch (
rack_id
) WHERE time_deleted IS NULL;

CREATE INDEX ON omicron.public.switch (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another superfluous index because the primary key always has an index.

CREATE UNIQUE INDEX ON omicron.public.switch (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Another of the cases mentioned in the PR description. This one's arguably unnecessary since we don't expect to have many switches in a rack.

rack_id,
id
) WHERE
time_deleted IS NULL;
) WHERE time_deleted IS NULL;

/*
* Services
Expand Down Expand Up @@ -224,12 +217,13 @@ CREATE TABLE omicron.public.service (
);

/* Add an index which lets us look up the services on a sled */
CREATE INDEX ON omicron.public.service (
CREATE UNIQUE INDEX ON omicron.public.service (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already have been unique since it includes the primary key.

sled_id,
id
);

CREATE INDEX ON omicron.public.service (
/* Look up (and paginate) services of a given kind. */
CREATE UNIQUE INDEX ON omicron.public.service (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already have been unique since it includes the primary key.

kind,
id
);
Expand Down Expand Up @@ -263,13 +257,13 @@ CREATE TABLE omicron.public.physical_disk (
)
);

CREATE INDEX ON omicron.public.physical_disk (
CREATE UNIQUE INDEX ON omicron.public.physical_disk (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already have been unique since it includes the primary key.

variant,
id
) WHERE time_deleted IS NULL;

-- Make it efficient to look up physical disks by Sled.
CREATE INDEX ON omicron.public.physical_disk (
CREATE UNIQUE INDEX ON omicron.public.physical_disk (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already have been unique since it includes the primary key.

sled_id,
id
) WHERE time_deleted IS NULL;
Expand Down Expand Up @@ -299,7 +293,7 @@ CREATE TABLE omicron.public.certificate (

-- Add an index which lets us look up certificates for a particular service
-- class.
CREATE INDEX ON omicron.public.certificate (
CREATE UNIQUE INDEX ON omicron.public.certificate (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should already have been unique since it includes the primary key.

service,
id
) WHERE
Expand Down Expand Up @@ -374,7 +368,7 @@ CREATE TABLE omicron.public.virtual_provisioning_resource (
* ZPools of Storage, attached to Sleds.
* These are backed by a single physical disk.
*/
CREATE TABLE omicron.public.Zpool (
CREATE TABLE omicron.public.zpool (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was one of three tables that were capitalized.

/* Identity metadata (asset) */
id UUID PRIMARY KEY,
time_created TIMESTAMPTZ NOT NULL,
Expand Down Expand Up @@ -402,7 +396,7 @@ CREATE TYPE omicron.public.dataset_kind AS ENUM (
/*
* A dataset of allocated space within a zpool.
*/
CREATE TABLE omicron.public.Dataset (
CREATE TABLE omicron.public.dataset (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was one of three tables that were capitalized.

/* Identity metadata (asset) */
id UUID PRIMARY KEY,
time_created TIMESTAMPTZ NOT NULL,
Expand Down Expand Up @@ -430,25 +424,25 @@ CREATE TABLE omicron.public.Dataset (
);

/* Create an index on the size usage for Crucible's allocation */
CREATE INDEX on omicron.public.Dataset (
CREATE INDEX on omicron.public.dataset (
size_used
) WHERE size_used IS NOT NULL AND time_deleted IS NULL AND kind = 'crucible';

/* Create an index on the size usage for any dataset */
CREATE INDEX on omicron.public.Dataset (
CREATE INDEX on omicron.public.dataset (
size_used
) WHERE size_used IS NOT NULL AND time_deleted IS NULL;

/*
* A region of space allocated to Crucible Downstairs, within a dataset.
*/
CREATE TABLE omicron.public.Region (
CREATE TABLE omicron.public.region (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was one of three tables that were capitalized.

/* Identity metadata (asset) */
id UUID PRIMARY KEY,
time_created TIMESTAMPTZ NOT NULL,
time_modified TIMESTAMPTZ NOT NULL,

/* FK into the Dataset table */
/* FK into the dataset table */
dataset_id UUID NOT NULL,

/* FK into the volume table */
Expand All @@ -463,15 +457,17 @@ CREATE TABLE omicron.public.Region (
/*
* Allow all regions belonging to a disk to be accessed quickly.
*/
CREATE INDEX on omicron.public.Region (
volume_id
CREATE UNIQUE INDEX on omicron.public.region (
volume_id,
id
Comment on lines +460 to +462
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the cases mentioned in the PR description. To paginate the regions on a disk, the id needs to be here, and then this can be a UNIQUE index.

);

/*
* Allow all regions belonging to a dataset to be accessed quickly.
*/
CREATE INDEX on omicron.public.Region (
dataset_id
CREATE UNIQUE INDEX on omicron.public.region (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above.

dataset_id,
id
);

/*
Expand Down Expand Up @@ -669,15 +665,15 @@ CREATE TABLE omicron.public.identity_provider (
provider_type omicron.public.provider_type NOT NULL
);

CREATE INDEX ON omicron.public.identity_provider (
id,
silo_id
CREATE UNIQUE INDEX ON omicron.public.identity_provider (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one should already be unique because it contains the primary key.

I changed the order of the fields. As it was, I don't think this index could be used to paginate through the identity providers in a Silo (which I think was the point of this index).

silo_id,
id
) WHERE
time_deleted IS NULL;

CREATE INDEX ON omicron.public.identity_provider (
name,
silo_id
CREATE UNIQUE INDEX ON omicron.public.identity_provider (
silo_id,
name
Comment on lines +679 to +681
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the few functional changes in this PR. This index was not unique and I think that was a bug. See this comment.

I also flipped the order of fields in order to use this index to paginate the IdPs in a Silo by name.

) WHERE
time_deleted IS NULL;

Expand Down Expand Up @@ -709,12 +705,20 @@ CREATE TABLE omicron.public.saml_identity_provider (
group_attribute_name TEXT
);

CREATE INDEX ON omicron.public.saml_identity_provider (
id,
silo_id
CREATE UNIQUE INDEX ON omicron.public.saml_identity_provider (
silo_id,
id
) WHERE
time_deleted IS NULL;
Comment on lines +713 to +717
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same as above -- needed to flip the order of these fields.


CREATE UNIQUE INDEX ON omicron.public.saml_identity_provider (
silo_id,
name
Comment on lines +719 to +721
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added this to ensure that SamlIdentityProvider names are unique within a Silo.

) WHERE
time_deleted IS NULL;



/*
* Users' public SSH keys, per RFD 44
*/
Expand Down Expand Up @@ -861,8 +865,9 @@ CREATE UNIQUE INDEX ON omicron.public.instance (

-- Allow looking up instances by server. This is particularly
-- useful for resource accounting within a sled.
CREATE INDEX ON omicron.public.instance (
active_sled_id
CREATE UNIQUE INDEX ON omicron.public.instance (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the cases mentioned in the PR description. To paginate the instances on a sled, the id needs to be here, and then this can be a UNIQUE index.

active_sled_id,
id
) WHERE
time_deleted IS NULL;

Expand Down Expand Up @@ -976,8 +981,9 @@ CREATE UNIQUE INDEX ON omicron.public.disk (
) WHERE
time_deleted IS NULL;

CREATE INDEX ON omicron.public.disk (
attach_instance_id
CREATE UNIQUE INDEX ON omicron.public.disk (
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is one of the cases mentioned in the PR description. To paginate the disks attached to an instance, the id needs to be here, and then this can be a UNIQUE index.

attach_instance_id,
id
) WHERE
time_deleted IS NULL AND attach_instance_id IS NOT NULL;

Expand Down