This change is 

Some early feedback posted here with agreement from the author.

In random order of importance:

• we usually hard wrap the text of rfcs.
• evolutation -> evolution
• you're missing a conversion class: "Assisted", which is even more general than what you now call "General". I suggest renaming "General" to "Cast-based" and add "Assisted" next to that. The new section "Assisted" is when an explicit conversion expression is specified with USING. I think this will be a pretty common case actually.
• appyling -> applying (may I suggest enabling the spell check in your editor? no judgment here)
• you will get questions about your deferred/completed approach with jobs. It seems orthogonal to this RFC: the mechanism could be equally well applied to any of our other asynchronous schema changes. If you want to argue it is necessary for "alter table set type", then I'd recommend starting this work separately, and complete it before you resume work on this feature.
• Cloumns -> columns
• I think that any value written to the old type after the change has been initiated, must also be done synchronously to the shadow column. We wouldn't know how to pick them up during the backfill otherwise. (Same as adding a new index.)
• "A backfill process is run on each node" -> "A backfill process is distributed across all nodes that have data for the table, like we do for index backfills already"
• " Once the backfill is complete, the column and indexes are upgraded to READ status. Any code that resolves a column or index name to an id should be prepared to look for a READ-mode shadow that has been activated. " - I have no idea what you're talking about here (neither what this is about nor why this is important). You should clarify, with examples.
• there's a missing entry in failure modes: Writes that occur during the backfill (after the change is initiated, but before the new type becomes active) are issued by the client app under the assumption that the old type is active. This means that from those clients' perspective it is OK to write any value valid for the old type but some of these values may be invalid for the new type. You have to outline what happens in that case.
• the final paragraph "Solving for the general case does have the potential to become a science project" doesn't make much sense to me. What are you trying to say? The project is complex, that's true, but so are other projects on CockroachDB. What are the things that require science which will not be completed when the work defined by the RFC is done?

Pro-tips: Bors understands the do-not-merge label, and also you're gonna want to update the PR description before you merge.

@knz, thank you for your near-instant feedback. I have incorporated the changes that you have suggested.

@couchand Fixed.

The RFC also needs to talk about interleaved tables and column families. How does the proposal work with those cases?

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

@knz, as I understand them, both the interleaved tables features and column families are just used to compute the KV key that the data will actually be inserted at to improve data locality. Beyond ensuring that any shadow columns created by a type change are placed in the same column family as the source column, is there some specific problem you're thinking of? Or is this just a topic that should appear in any RFC related to data manipulation?

Review status: all files reviewed at latest revision, 5 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 97 at r1 (raw file):

My thinking is that we get non-PK columns working first to get all of the type-change machinery in place. Once it's possible to change the PK columns for a table, we can merge the two features together, since it seems like making any non-trivial change to a PK column type is effectively the same as changing the PK to a new (shadow) column.

We could certainly support trivial type changes for PK columns as part of this RFC, except that I think we'd need to handle foreign-key references. Let's say the user is doing STRING -> BYTES in table A and table B has an FK constraint. Does that STRING column in table B also need to have a cascaded type change to BYTES?

docs/RFCS/20180411_alter_column_type.md, line 105 at r1 (raw file):

Clarified the sentence.

docs/RFCS/20180411_alter_column_type.md, line 110 at r1 (raw file):

That was left over from an earlier thought about how the two-phase commit would work. It's not actually needed and has been removed.

docs/RFCS/20180411_alter_column_type.md, line 203 at r1 (raw file):

The scenario that I want to avoid looks like:

• Start reading a range to perform the conversion.
• The user issues a write to a row within that range.
• Stomp on the user's write with the backfilled data.

It would be fine, I think to retry that range if we detect a conflict; is there plumbing that already makes this possible?

Comments from Reviewable

Another concern I just thought of is range thrash: since the shadow column is going to duplicate the source column and we want to be able to have them both live in the KV layer at the same time, our encoded column families are going to get (temporarily) larger. This could cause extra range-splitting to happen, right? Is this going to be a problem?

Review status: 0 of 1 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Regarding interleaved tables: the RFC should discuss whether changing the column type on the parent should be blocked (error) or accepted (and propagate automatically to the children tables).

Review status: 0 of 1 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 97 at r1 (raw file):

Good point. Sounds like we should stick with no PK and no FK for now.

docs/RFCS/20180411_alter_column_type.md, line 350 at r2 (raw file):

|                     | `TIMESTAMP`             |                     |
| `STRING COLLATE ab` |                         |                     |
|                     | `STRING COLLATE cd`     | Trivial             |

I thought the collation key affected the on-disk bytes. Does it not?

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 97 at r1 (raw file):

Added a paragraph in the rationale section summarizing this.

docs/RFCS/20180411_alter_column_type.md, line 183 at r1 (raw file):

Clarified this section to describe the ideal condition, pending discussion with DT and core-team about how we can slide sstable updates "underneath" and pre-existing writes to a particular KV key.

docs/RFCS/20180411_alter_column_type.md, line 203 at r1 (raw file):

Moved this into the unresolved-questions section.

docs/RFCS/20180411_alter_column_type.md, line 107 at r2 (raw file):

Added details.

docs/RFCS/20180411_alter_column_type.md, line 176 at r2 (raw file):

It's not going to be necessary for a trivial conversion, since those are "instantaneous", but we'll want a two-phase setup for validated changes.

docs/RFCS/20180411_alter_column_type.md, line 204 at r2 (raw file):

Thanks.

docs/RFCS/20180411_alter_column_type.md, line 268 at r2 (raw file):

Clarified the sentence in which shadow columns are introduced that they will be anonymous.

Comments from Reviewable

@mjibson, @knz I'd like to get to an lgtm to land this as an in-progress rfc and start laying down code. Thanks.

Review status: 0 of 1 files reviewed at latest revision, 10 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 10 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 163 at r3 (raw file):

-- Wait for the job to finish, stop the application, swap the column type.
-- This command would block, returning any output from the conversion job.
-- Any reads that happen-after this command will see the new type, while any

What happens to reads that are submitted to cockroach while the COMPLETE is blocking? Do they block?

docs/RFCS/20180411_alter_column_type.md, line 189 at r3 (raw file):

This could also break views in certain circumstances. We either need to disallow changing the types of columns used in views, or do some hard thinking about how type changes affect them. @knz is the expert here.

docs/RFCS/20180411_alter_column_type.md, line 383 at r3 (raw file):

since a copy-and-rename operation can be performed or users can write
their own migration code. This is, however, a sub-optimal user
experience and will diminish compatibility with ORM or other tooling

Nit that you don't have to bother fixing, but saying that it "will diminish compatibility" is much weaker than saying that it does diminish compatibility and calling out at least one specific important ORM/tool that is hurt by it.

Comments from Reviewable

LGTM modulo Alex's comments.

Review status: all files reviewed at latest revision, 13 unresolved discussions.

Comments from Reviewable

Thanks for pinging me. Overall I'm happy about most of the details addressed but there's still one major point open. See below.

Review status: all files reviewed at latest revision, 13 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 144 at r3 (raw file):
On second read I'm afraid this DEFERRED/COMPLETE business won't do.
Remember, we're investing in this work also partly for compatibility with PostgreSQL clients. It is thus rather important that we invest sufficient time and attention in reusing PostgreSQL's SQL syntax/semantics to achieve this.

1. pg's syntax for ALTER TABLE SET TYPE does not include DEFERRED/COMPLETE
2. we want to introduce compatibility with existing clients/apps which already use ALTER TABLE SET TYPE. These do not know about DEFERRED/COMPLETE.

So where does this leave us.

• I think there should be an alternative to the mechanisms discussed here which does not require these keywords.
• I have made a first comment about this in here: rfc: Alter column type #24703 (comment)

about your deferred/completed approach with jobs. It seems orthogonal to this RFC: the mechanism could be equally well applied to any of our other asynchronous schema changes. If you want to argue it is necessary for "alter table set type", then I'd recommend starting this work separately, and complete it before you resume work on this feature.

I would like to see your reaction to this. It seems to me that you could issue the ALTER TABLE SET TYPE like any of our other async schema changes, ie. queue the mutation and process it upon txn commit (like CREATE INDEX, ALTER TABLE ADD COLUMN, etc). If you were to do that, then we get the compatibility with pg clients "for free".

The mechanism you are proposing here (DEFERRED/COMPLETE) could be considered as well as a CockroachDB-extension, and made available to any ALTER not just this one.

docs/RFCS/20180411_alter_column_type.md, line 189 at r3 (raw file):

Yes it's rather simple really. The RFC must make it clear that changing the type of a column depended on by a view should be simply disallowed, there is nothing else we can do.
The DependedOnBy field on the descriptor indicates which column IDs are "locked" in that way.

docs/RFCS/20180411_alter_column_type.md, line 291 at r3 (raw file):

cast, it will be marked as `(extension)`.

| From                | To                      | Notes               |

This table must be extended to include TIMESTAMPTZ and TIMETZ

I'd also like to make it clear in the Notes which conversion applies. It's not clear to me what it means when the notes column is empty.

docs/RFCS/20180411_alter_column_type.md, line 337 at r3 (raw file):

|                     | `INTERVAL`              |                     |
|                     | `DECIMAL`               |                     |
|                     | `FLOAT`                 |                     |

float is listed twice

Comments from Reviewable

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

Comments from Reviewable

Review status: all files reviewed at latest revision, 13 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 144 at r3 (raw file):

One idea is to introduce the notion of BACKFILL. So without using this keyword cockroach can behave like pg and run all the individual steps for the schema change. With the BACKFILL keyword, it completes all the steps until the backfill is complete. The BACKFILL command can be followed by the plain command that is speeded up because the BACKFILL is complete

Comments from Reviewable

Review status: all files reviewed at latest revision, 13 unresolved discussions.

Comments from Reviewable

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 144 at r3 (raw file):

s/DEFERRED/SHADOW/g

Comments from Reviewable

Based on a conversation with @knz, @mjibson, and @dt we'll take a different approach when the user needs control over exactly when a type change will become visible to clients that cannot support both the old and new column types concurrently. The rough approach looks like the following:

• Add a new computed column from the active, original column that performs the desired type conversion.
• Wait for the computed column to backfill.
• Atomically swap the source and computed column, remove the computed-ness, and optionally drop the original column.

A regular ALTER COLUMN TYPE command is more or less a wrapper around the above using a hidden column and performing the swap when the backfill is ready. This will give us the desired user experience of directed schema evolution and provide an independently-useful feature of being able to drop the computed nature of a column. We'll also make sure that the above workflow is highly visible in the documentation.

Review status: 0 of 1 files reviewed at latest revision, 11 unresolved discussions, some commit checks failed.

Comments from Reviewable

PTAL: I've pushed a new version of the RFC that eliminates the explicit syntax for controlling the two-phase type change for one that builds on top of being able to add new computed columns.

Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions.

docs/RFCS/20180411_alter_column_type.md, line 208 at r5 (raw file):

Done.

docs/RFCS/20180411_alter_column_type.md, line 83 at r6 (raw file):

Done.

docs/RFCS/20180411_alter_column_type.md, line 167 at r6 (raw file):

Reworked this section to talk more generally about not supporting structural dependencies other than indexes.

docs/RFCS/20180411_alter_column_type.md, line 192 at r6 (raw file):

Reworded to be more general.

docs/RFCS/20180411_alter_column_type.md, line 234 at r6 (raw file):

Understood. Updated this paragraph to discuss the alternative error message.

docs/RFCS/20180411_alter_column_type.md, line 239 at r6 (raw file):

Done.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

Agreed. I have clarified that only T -> T[] can be performed implicitly. Any other array-involved conversion T -> S[] or T[] -> T requires an explicit USING expression.

Comments from Reviewable

PTAL: Added EXPLAIN(STEPS) as a way to give DBA's a way to perform preparatory work so the actual ALTER ... COLUMN TYPE can be almost instantaneous.

Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions.

Comments from Reviewable

bors r+

Reviewed 1 of 1 files at r4.
Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions, some commit checks pending.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

I must insist, the conversion T -> T[] is

1. not clearly motivated here
2. not possible via casts
3. hard to justify implementation-wise: there's nothing that guarantees it will be cheap other than currnet implementation happenstance

I would encourage you to not implement this initially at all and instead wait until you have completed the rest of the work to decide whether it is indeed useful (and possibly leave that implementation to a follow-up project).

I could be convinced otherwise if I saw the business case, but the RFC is not presenting that yet.

Comments from Reviewable

Build failed

• GitHub CI (Cockroach)

Review status: 0 of 1 files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

T -> T[] and T[] -> T must be possible if we're implementing a general type-conversion system. I'm assuming that cast-type conversions will be implemented by substituting a default USING clause, which in this case would be USING ARRAY[column_name]. The implicit conversion is a user convenience and doesn't affect the rest of the type system. If it turns into an implementation problem, I'll reconsider allowing array conversions, but, for now, I remain unconvinced that it should be excluded.

Comments from Reviewable

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

T -> T[] and T[] -> T must be possible if we're implementing a general type-conversion system.

1. Why?

We do not support conversion of UUID to/from FLOAT. We do not support conversion of INET to/from INTERVAL. The reason why we do not support them is that they are non-sensical.

There are plenty of non-sensical conversions.

I posit that converting T <-> T[] is also non-sensical. I am willing to change my mind, but you'd need to explain.

2. What is a "general type-conversion system"?

Comments from Reviewable

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

You know the saying about how there's only three numbers that matter: 0, 1, ∞, and everything else is a special case? The way I see it, you should be able to have 0 of something (a never-created or dropped column), 1 of something (a column of type T) or an infinite number of something (a column of type T[]).

Right now, you can achieve the following conversions:

• 0 -> 1 is ALTER TABLE ADD COLUMN t T
• 0 -> ∞ is ALTER TABLE ADD COLUMN t T[]
• 1, ∞ -> 0 is ALTER TABLE DROP COLUMN t

I want to solve for the missing conversion of 1 <--> ∞ by allowing the type of a column to be changed from T -> T[] or T[] ->T.

Comments from Reviewable

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

• T -> T[]: ALTER COLUMN x SET TYPE int[] USING ARRAY[x]
• T[] -> T: ALTER COLUMN x SET TYPE int USING x[1]

What's missing?

In your opinion, why does PostgreSQL not allow writing 123::int::int[]?

In your opinion, why does PostgreSQL now allow writing array[1,2,3]::int?

Also what does T[] -> T mean when there's more than one element?

Comments from Reviewable

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):

T -> T[]: ALTER COLUMN x SET TYPE int[] USING ARRAY[x]
T[] -> T: ALTER COLUMN x SET TYPE int USING x[1]

Exactly. I'm not sure what we're discussing.

Comments from Reviewable

Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.

docs/RFCS/20180411_alter_column_type.md, line 338 at r6 (raw file):
Look at the first sentence in the paragraph:

Any given type T may be implicitly converted to T[].

(emphasis mine)

If you were to rephrase this as "Any given type T may be explicitly converted to T[] with USING."

Then we'd be in agreement.

Comments from Reviewable

@knz Updated doc so that all array-involved conversions require an explicit USING expression.

bors r+

Build succeeded

• GitHub CI (Cockroach)