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.

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

Interleaving deprecation #10154

Merged
merged 7 commits into from
Mar 31, 2021
Merged

Interleaving deprecation #10154

merged 7 commits into from
Mar 31, 2021

Conversation

ericharmeling
Copy link
Contributor

@ericharmeling ericharmeling commented Mar 23, 2021

Fixes #9974.
Fixes #9622.
Fixes #9267.

This PR:

  • Improves the documentation on deprecation/removal of interleaved tabes/indexes in the 20.2 and 21.1 docs.
  • Removes most of the references/recommendations for interleaved tables/indexes from the 20.2 and 21.1 docs. The exceptions being the actual INTERLEAVE IN PARENT syntax page and the 20.2.0 GA deprecation notices.

Note that this PR does not include guidance on querying crdb_internal.interleaved* tables (as suggested in #9974), as those tables (cockroachdb/cockroach#61629 and cockroachdb/cockroach#62076) have yet to make it to a release. When those PRs make it to a release branch and are part of a published release, we can update the crdb_internal page (also yet to be published: #8909), and then add a short example to interleave-in-parent.html.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ericharmeling ericharmeling changed the title Interleaving deprecation [WIP] Interleaving deprecation Mar 23, 2021
Copy link
Contributor

@vy-ton vy-ton left a comment

Choose a reason for hiding this comment

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

Reviewed 52 of 52 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)


v20.2/backup.md, line 85 at r1 (raw file):

Table with a [sequence](create-sequence.html) | The sequence it uses; however, this dependency can be [removed during the restore](restore.html#skip_missing_sequences).
[Views](views.html) | The tables used in the view's `SELECT` statement.
[Interleaved tables](interleave-in-parent.html) | The parent table in the [interleaved hierarchy](interleave-in-parent.html#interleaved-hierarchy).

We shouldn't delete this since backups will still need to account for dependencies on interleaved tables until the feature is fully removed


v20.2/cockroach-workload.md, line 177 at r1 (raw file):

`--histograms` | The file to write per-op incremental and cumulative histogram data to.<br><br>**Applicable command:** `run`
`--init` | **Deprecated.** Use the `init` command instead.<br><br>**Applicable command:** `run`
`--interleaved` | Use [interleaved tables](interleave-in-parent.html).<br><br>**Applicable commands:** `init` or `run`

cockroachdb/cockroach#62609 tracks removing the flag. It does still exist today.


v20.2/cost-based-optimizer.md, line 169 at r1 (raw file):

{{site.data.alerts.callout_info}}
With queries on [interleaved tables](interleave-in-parent.html), the optimizer might choose to use a merge join to perform a [foreign key](foreign-key.html) check when a lookup join would be more optimal.

Planning of the interleaved join was disabled by default in 20.2. Not sure why this doc compares merge. join vs lookup join for interleaved tables.

Maybe @jordanlewis knows what we should highlight here if anything


v20.2/create-table.md, line 134 at r1 (raw file):

{{site.data.alerts.callout_info}}
Column families, interleavings, partitioning, and foreign key constraints

Probably can't delete this yet, since it's still true for 20.2 (i.e interleaved is deprecated but not removed yet)


v20.2/create-table-as.md, line 93 at r1 (raw file):

## Limitations

Tables created with `CREATE TABLE ... AS` are not [interleaved](interleave-in-parent.html) with other tables.

Still true, prob shouldn't delete yet


v20.2/hash-sharded-indexes.md, line 24 at r1 (raw file):

{{site.data.alerts.callout_info}}
Hash-sharded indexes cannot be [interleaved](interleave-in-parent.html).

still true, can't delete yet


v20.2/import-into.md, line 329 at r1 (raw file):

- After importing into an existing table, [constraints](constraints.html) will be un-validated and need to be [re-validated](validate-constraint.html).
- Imported rows must not conflict with existing rows in the table or any unique secondary indexes.
- `IMPORT INTO` works for only a single existing table, and the table must not be [interleaved](interleave-in-parent.html).

Still true


v20.2/interleave-in-parent.md, line 76 at r1 (raw file):

- Scans over tables or indexes with interleaved, child objects (i.e., interleaved tables or indexes) are much slower than scans over tables and indexes with no child objects, as the scans must traverse the parent object and all of its child objects.
- Database schema changes are slower for interleaved objects and their parents than they are for non-interleaved objects and objects with no interleaved children. For example, if you add or remove a column to a parent or child table, CockroachDB must rewrite the entire interleaved hierarchy for that table and its parents/children.
- [Internal benchmarks](https://github.com/cockroachdb/cockroach/issues/53455) have shown the performance benefits of interleaving tables and indexes are limited to a small number of use cases that do not justify the cost in engineering resources for maintenance.

Lets remove that do not justify the cost in engineering resources for maintenance


v20.2/interleave-in-parent.md, line 82 at r1 (raw file):

After [upgrading to v20.2](upgrade-cockroach-version.html), we recommend that you do the following:

- [Convert any existing interleaved tables to non-interleaved tables](#convert-interleaved-tables).

+1


v20.2/interleave-in-parent.md, line 91 at r1 (raw file):

### Convert interleaved tables

To convert an interleaved table to a non-interleaved table, issue an [`ALTER PRIMARY KEY`](alter-primary-key.html) statement on the table, specifying the existing primary key column(s) for the table, and no `INTERLEAVE IN PARENT` clause. Note that an `ALTER PRIMARY KEY` statement can only convert a child table if that table is not a parent. If your cluster has child tables that are also parents, you must start from the bottom of the interleaving hierarchy and work your way up (i.e., start with child tables that are not parents).

Let's mention that these schema changes are also online


v20.2/restore.md, line 143 at r1 (raw file):

Table with a [sequence](create-sequence.html) | The sequence.
[Views](views.html) | The tables used in the view's `SELECT` statement.
[Interleaved tables](interleave-in-parent.html) | The parent table in the [interleaved hierarchy](interleave-in-parent.html#interleaved-hierarchy).

Keep this


v20.2/sql-feature-support.md, line 171 at r1 (raw file):

-----------|-----------|------|---------
 Column families | ✓ | CockroachDB Extension | [Column Families documentation](column-families.html)
 Interleaved tables | ✓ | CockroachDB Extension | [Interleaved Tables documentation](interleave-in-parent.html)

I would indicated Deprecated somehow. We still support existing interleaved tables until they are removed in 21.2

@vy-ton
Copy link
Contributor

vy-ton commented Mar 25, 2021

New migration guide looks great! We just need to be careful because interleaved tables are not yet removed, so all the docs related to their current behavior likely need to stick around for one more release.

We should be deleting/modifying anything that promotes their use though.


- Interleaved tables cannot be the child of more than 1 parent table. However, each parent table can have many children tables. Children tables can also be parents of interleaved tables.
Interleaving tables and indexes is deprecated in CockroachDB v20.2, and will be permanently disabled in a future release, for the following reasons:
Copy link
Contributor

Choose a reason for hiding this comment

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

@jordanlewis or @ajwerner might be interested in spot checking this

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR @vy-ton !

I think I've addressed all of your comments.

We just need to be careful because interleaved tables are not yet removed, so all the docs related to their current behavior likely need to stick around for one more release.

Good point. I've reinstated some of the syntax/behavior notes that I removed. I will remove these again in 21.2, when interleaving is fully removed.

We should be deleting/modifying anything that promotes their use though.

Agreed - all notes that fall into the category remain deleted/modified.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, and @vy-ton)


v20.2/backup.md, line 85 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

We shouldn't delete this since backups will still need to account for dependencies on interleaved tables until the feature is fully removed

Done.


v20.2/cockroach-workload.md, line 177 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

cockroachdb/cockroach#62609 tracks removing the flag. It does still exist today.

Done.


v20.2/cost-based-optimizer.md, line 169 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Planning of the interleaved join was disabled by default in 20.2. Not sure why this doc compares merge. join vs lookup join for interleaved tables.

Maybe @jordanlewis knows what we should highlight here if anything

Yeah that doesn't make much sense... it probably should have been removed earlier.


v20.2/create-table.md, line 134 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Probably can't delete this yet, since it's still true for 20.2 (i.e interleaved is deprecated but not removed yet)

Done.


v20.2/create-table-as.md, line 93 at r1 (raw file):

Tables created with CREATE TABLE ... AS are not interleaved with other tables.

I agree that this is still true, but I don't find it particularly relevant to anything. Especially if we don't want to encourage users to interleave tables. INTERLEAVE IN PARENT has never been supported in CREATE TABLE AS AFAIK.


v20.2/hash-sharded-indexes.md, line 24 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

still true, can't delete yet

Done.


v20.2/import-into.md, line 329 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Still true

Done.


v20.2/interleave-in-parent.md, line 76 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Lets remove that do not justify the cost in engineering resources for maintenance

Done.


v20.2/interleave-in-parent.md, line 91 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Let's mention that these schema changes are also online

Done.


v20.2/restore.md, line 143 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

Keep this

Done.


v20.2/sql-feature-support.md, line 171 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

I would indicated Deprecated somehow. We still support existing interleaved tables until they are removed in 21.2

Done.

Copy link
Member

@jordanlewis jordanlewis 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 and @vy-ton)


v20.2/cost-based-optimizer.md, line 169 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

Yeah that doesn't make much sense... it probably should have been removed earlier.

I think we can just remove this, I agree


v20.2/interleave-in-parent.md, line 72 at r1 (raw file):

Previously, vy-ton (Vy Ton) wrote…

@jordanlewis or @ajwerner might be interested in spot checking this

LGTM

Copy link
Contributor Author

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

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

TFTR @jordanlewis

Passing to writer review.

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

@ericharmeling ericharmeling merged commit 711fcc6 into master Mar 31, 2021
@ericharmeling ericharmeling deleted the disable-interleaved-tables branch March 31, 2021 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants