RFCS: ALTER COLUMN TYPE for cast conversions

[RichardJCai]

Created a new RFC for ALTER COLUMN TYPE that requires a cast for conversion.

Thought I would create a new one instead of adding to #24703 since this one mainly focuses on ALTER COLUMN TYPE where a cast is required.

Release note: none
Release justification: Added an RFC - no code change.

[cockroach-teamcity]

This change is 

[otan]

just two bits i'm curious about

[rohany]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @otan, and @RichardJCai)

---

docs/RFCS/20200401_alter_column_type.md, line 32 at r2 (raw file):

    - Make c a computed column of c'
        - This is necessary since if one node writes to to the table with the updated TableDescriptor (with c' and c swapped) while one node reads from the version with the old TableDescriptor, the value should still exist.
    - Make c a hidden column, make c' a not hidden column

Hidden isn't the correct terminology. Instead, public and non public are correct. Hidden refers to columns like the implicit rowid col.

---

docs/RFCS/20200401_alter_column_type.md, line 60 at r2 (raw file):

    - Includes enqueuing column drop mutation for the old column.

## Failure during schema change

Mention that it is similar to any other case of a schema change running into a runtime error, like building a unique index on a set of columns that turns out to not be unique.

---

docs/RFCS/20200401_alter_column_type.md, line 88 at r2 (raw file):

In the following block, a id2 is converted from int to string, however in the period when the old column of id2 has not been dropped, we try inserting 'hello' into id2 but run into a parse error.

[email protected]:57338/movr> create table t1 (id int, id2 int, id3 int);

I dont think you need this whole code sample. Instead, say something like "while the old column is in the delete and write only state, we must attempt to write a value to the old column. If the value we are inserting cannot be cast into the type of the old column, we error out."

[knz]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis, @otan, @RichardJCai, and @rohany)

---

docs/RFCS/20200401_alter_column_type.md, line 16 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

You're right - updated to use the correct terminology.

I think I disagree with Oliver.
There are implicit casts in pg that are non-trivial in crdb and need this RFC. For example float8->float4, or float -> decimal.

The summary could be expanded by stating what "trivial" means (does not require changing the data representation in storage) and then say that the RFC is about non-trivial changes, i.e. re-encoding the stored data.

---

docs/RFCS/20200401_alter_column_type.md, line 30 at r2 (raw file):

1. Create a new hidden computed column c' using c::t'
2. Enqueue a column swap mutation, for c and c' where performs the following set of actions:
    - Make c a computed column of c'

nit: this second level of list items is also ordered. Use a. b. c. etc

---

docs/RFCS/20200401_alter_column_type.md, line 32 at r2 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Hidden isn't the correct terminology. Instead, public and non public are correct. Hidden refers to columns like the implicit rowid col.

I think that the Hidden bit could be the right tool in this approach - I don't see reasons why not.

(In fact, even though you could play with the "public" bit to achieve this, it would require a new schema change mutation type that's able to flip the public bits atomically for two columns at a time. Do-able, but not trivial. It would need to be spelled out in detail.)

---

docs/RFCS/20200401_alter_column_type.md, line 140 at r2 (raw file):

### Possible solution
A solution to this is disallowing inserts to the column while the schema change is happening - however this is effectively an offline schema change (for the column).

That seems unfortunate.

Is it not possible to make these INSERT/UPDATE/etc update both the old and the new columns side-by-side?

[rohany]

I think that the Hidden bit could be the right tool in this approach - I don't see reasons why not.
(In fact, even though you could play with the "public" bit to achieve this, it would require a new schema change mutation type that's able to flip the public bits atomically for two columns at a time. Do-able, but not trivial. It would need to be spelled out in detail.)

This is how the primary key change operation works, so there is precedent behind it.

[RichardJCai]

Possible solution

A solution to this is disallowing inserts to the column while the schema change is happening - however this is effectively an offline schema change (for the column).
That seems unfortunate.

Is it not possible to make these INSERT/UPDATE/etc update both the old and the new columns side-by-side?

@knz I'm unsure we'll be able to find a value to INSERT into the old column at all. For example if the new column is built off the old column using an expression, (ie when doing int -> bool, the expression can be if x > 0 true, else false) we don't necessarily have a value to insert into the old int column. Also we can't always put NULL since the column may be non-nullable.

[jordanlewis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

---

docs/RFCS/20200401_alter_column_type.md, line 20 at r3 (raw file):

# Motivation
Fairly common use case to change a columns data type, especially in ORMs. Highly requested feature.

nit: s/columns/column's/

---

docs/RFCS/20200401_alter_column_type.md, line 27 at r3 (raw file):

# Detailed Explanation
## A. Converting a column that is not part of an index (simple case)
If were trying to convert column **c** to type **t**

s/were/we're/

---

docs/RFCS/20200401_alter_column_type.md, line 63 at r3 (raw file):

When converting column c (type t) to c' (type t'). During the creation of c' as a computed column, if a value x in c cannot be converted to type t' using the conversion function, the column creation fails and we stop the schema change. This state is easy to roll back from as we simply drop the new column c'. This case happens before the swap mutation.

Example, if we try converting a column from string -> int, and we run into a value 'hello', then the computed column cannot be created and a rollback will happen (dropping the column).

s/Example/For example,/

---

docs/RFCS/20200401_alter_column_type.md, line 66 at r3 (raw file):

This is similar to any other case of a schema change running into a runtime error.

If the swap mutation has happened - the type of the column is effectively changed, only thing left to do is drop the old column.

s/only/the only/

---

docs/RFCS/20200401_alter_column_type.md, line 72 at r3 (raw file):

## Out of order ColumnIDs/OrdinalPositions/attnum
When performing a column swap, due to ColumnIDs being a part of the value encoding for columns, we cannot swap ColumnIDs of the columns being swapped. Thus for the proposed method of performing a swap by adding a new column and dropping the old one, the ColumnIDs will be "out of order". This results in places that currently depend on ColumnIDs to also be out of order. For example in information_schema.columns, ordinal_position is ColumnID. (Similarly attnum and adnum in pg_catalog also correpsond to ColumnID).
This results in SHOW COLUMNS being out of order as it currently depends on column id. Du

Incomplete sentence

---

docs/RFCS/20200401_alter_column_type.md, line 76 at r3 (raw file):

One solution is adding a "ordinal position" slice field to the TableDescriptor to handle the "logical id" of a column for virtual tables. As it currently stands, this ordinal position value can correspond to the Column ID, it would only change when doing this column swap, so when a swap is performed, the ordinal positions of the columns change. We can store ordinal position in the ColumnDescriptor or TableDescriptor. If we use a slice for OrdinalPositions in table descriptor, each index can correspond to the Columns field in TableDescriptor, thus when doing a swap, between two columns, we wouldn't have to change the OrdinalPositions.

Interesting page on ALTER COLUMN POSITION idea for Postgres.

Make this into a reference and incorporate it into the paragraph below.

---

docs/RFCS/20200401_alter_column_type.md, line 87 at r3 (raw file):

Example: When a column is converted from int -> string, but the value inserted cannot be converted back to int.
- ie, if we insert 'hello' into the converted column from int -> string while the old int column has not been dropped yet, it will give a parse error since it will try to parse 'hello' as an int to support insert into the old column.

s/ie/i.e./

---

docs/RFCS/20200401_alter_column_type.md, line 100 at r3 (raw file):

Similarly to the previous problem, if one node performs a read from the table with the old TableDescriptor after one node does an insert into the table with a new version of the TableDescriptor, what value can we give for the read?

For example, if we're converting a column from int to bool and using the expression f(x) = true if x > 0 and false otherwise, how do we get an inverse for this function?

We need to have the inserts be validated by both of the types until the switch is completely finished.

[RichardJCai]

Addressed outstanding comments

[thoszhang]

Reviewed 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

---

docs/RFCS/20200401_alter_column_type.md, line 140 at r2 (raw file):

I'm unsure we'll be able to find a value to INSERT into the old column at all. For example if the new column is built off the old column using an expression, (ie when doing int -> bool, the expression can be if x > 0 true, else false) we don't necessarily have a value to insert into the old int column. Also we can't always put NULL since the column may be non-nullable.

I'm pretty worried about this problem. (Sorry if I end up rehashing discussions that have happened outside of this RFC. I'm thinking about it for the first time.)

(First of all, just to be explicit about my assumptions, though this is probably already obvious to everyone else: This problem arising from the function essentially being one-way seems fundamental to having to switch to the new type in a single descriptor version change, and doesn't depend on the physical layout of the columns. What the state-based schema changer framework would require, in order for this to work, would be to have an additional intermediate state where we can only read from the t' column, but we have to write to the t column, which is impossible.)

I think the approach of always filling in missing values with some placeholder value is more promising than trying to trying to come up with an inverse value. It seems logical that casting to a different type is not an invertible operation in general, and trying to make an attempt at casting in the opposite direction would be pushing some potentially surprising behavior onto users. But, elaborating on your point, I don't think it's possible to come up with a "placeholder value" to insert into a column in general; even if we inserted the non-null zero value for the type, for instance, we could still violate other constraints on the column. (This has also come up in discussions about how to backfill a column if we started dropping it but need to roll that back.)

What exactly are users expecting from an online ALTER TYPE schema change? How would an application handle the type of a column changing in some non-trivial way while they are reading from and writing to it? (I could imagine something to handle reading either type easily, but how do they switch over to the new type for writes?) Knowing that would help me think about this problem, at least.

---

docs/RFCS/20200401_alter_column_type.md, line 112 at r4 (raw file):

# Alternatives
One alternative is to create a whole new table, this simplifies the cases as we redefine the new column, indexes and primary key.

@ajwerner mentioned that there's the possibility of writing an entirely new index in all cases, not just when one of the columns is in the primary key. Should that go into the RFC?

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

---

docs/RFCS/20200401_alter_column_type.md, line 69 at r4 (raw file):

# Problems

Another thing I realized:

• what if the column is part of a CHECK condition - we must ensure both old and new value validate the CHECK condition
• what if the column is part of a FK constraint - we must ensure that both old and new value validate the FK constraint

Both of these cases need specific optimizer support. Where is this discussed in the RFC?

[RichardJCai]

What exactly are users expecting from an online ALTER TYPE schema change? How would an application handle the type of a column changing in some non-trivial way while they are reading from and writing to it? (I could imagine something to handle reading either type easily, but how do they switch over to the new type for writes?) Knowing that would help me think about this problem, at least.

For the case that the insert is not valid for both the old and the new columns, it should be okay to disallow and return an error on that insert until the schema change is finalized.

@ajwerner mentioned that there's the possibility of writing an entirely new index in all cases, not just when one of the columns is in the primary key. Should that go into the RFC?

Yeah I can add a section on that.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

docs/RFCS/20200401_alter_column_type.md, line 69 at r4 (raw file):

# Problems

Another thing I realized:

• what if the column is part of a CHECK condition - we must ensure both old and new value validate the CHECK condition
• what if the column is part of a FK constraint - we must ensure that both old and new value validate the FK constraint

Both of these cases need specific optimizer support. Where is this discussed in the RFC?

I think the case of CHECK CONSTRAINTS may be really weird / tricky. After an ALTER COLUMN TYPE, the constraint may not necessarily make sense for the new type right?

Also theoretically, if the constraint stays the same and makes sense to apply to the new column, then I don't think it would be necessary to validate the constraint on both the old and new value. Since at any given time, one column is computed based off the other - by checking one constraint for the "active" (whichever is not computed) column, the insert is also validated for the computed column. So theoretically if the same constraint applies to both columns, then I don't think it's necessary to check both constraints. Although I'm not entirely sure if I'm right here. I think it does make sense to validate two check statements though.

[thoszhang]

About constraints: I very briefly looked into Postgres's behavior because I was interested in how constraints would affect the schema change implementation. Here's what the documentation says:

Indexes and simple table constraints involving the column will be automatically converted to use the new column type by reparsing the originally supplied expression.

The USING option of SET DATA TYPE can actually specify any expression involving the old values of the row; that is, it can refer to other columns as well as the one being converted. This allows very general conversions to be done with the SET DATA TYPE syntax. Because of this flexibility, the USING expression is not applied to the column's default value (if any); the result might not be a constant expression as required for a default. This means that when there is no implicit or assignment cast from old to new type, SET DATA TYPE might fail to convert the default even though a USING clause is supplied. In such cases, drop the default with DROP DEFAULT, perform the ALTER TYPE, and then use SET DEFAULT to add a suitable new default. Similar considerations apply to indexes and constraints involving the column.

https://www.postgresql.org/docs/current/sql-altertable.html

I tried a few things, and it seems like Postgres naively tries to preserve all constraints on the column and returns an error if anything doesn't work anymore. Everything I observed seems compatible with the approach we've been discussing.

• Postgres will try to preserve not null constraints and unique constraints (by rebuilding the index). The ALTER statement returns with a could not create unique index error if the values are no longer unique after type conversion, e.g.,

  lucy=# create table t (a text unique);
  CREATE TABLE
  lucy=# insert into t values ('1'), ('01');
  INSERT 0 2
  lucy=# alter table t alter a type int using a::integer;
  ERROR:  could not create unique index "t_a_key"
  DETAIL:  Key (a)=(1) is duplicated.
  

• For check constraints, a (somewhat cryptic) error is returned if the expression no longer typechecks with the new column type, e.g.,

  lucy=# create table t (a int check (a > 0));
  CREATE TABLE
  lucy=# alter table t alter a type text using a::text;
  ERROR:  operator does not exist: text > integer
  HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.
  

  (The hint is a bit misleading; I'm pretty sure that the real problem is the check constraint, since this error goes away if I remove the constraint.)
• For foreign keys, you get the error you'd expect:

  lucy=# create table referenced (a int primary key); insert into referenced values (1);
  CREATE TABLE
  INSERT 0 1
  lucy=# create table t (a int references referenced); insert into t values (1);
  CREATE TABLE
  INSERT 0 1
  lucy=# alter table t alter a type text using a::text;
  ERROR:  foreign key constraint "t_a_fkey" cannot be implemented
  DETAIL:  Key columns "a" and "a" are of incompatible types: text and integer.
  

I think what this means is that we should just try to create the new column with all the constraints copied over, and then let the schema change fail if anything doesn't work with the new column. Also, it seems like we should be enforcing both sets of constraints for both columns during the phase where we're still adding the new column, since the validity of the constraint for the old column doesn't imply validity for the new column and vice versa, even when the constraint still somehow makes sense for both columns. (The USING function could be non-injective, impure, ....)

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 614d6ef5.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[RichardJCai]

I think what this means is that we should just try to create the new column with all the constraints copied over, and then let the schema change fail if anything doesn't work with the new column. Also, it seems like we should be enforcing both sets of constraints for both columns during the phase where we're still adding the new column, since the validity of the constraint for the old column doesn't imply validity for the new column and vice versa, even when the constraint still somehow makes sense for both columns. (The USING function could be non-injective, impure, ....)

I agree with everything you said about constraints. Also as a note, when an expression is provided via USING for ALTER COLUMN TYPE, Postgres doesn't apply the using expression to the default expression/constraint/indexes. If we follow that, it makes creating the new set of constraints a little easier.

I created an issue for supporting constraints here with the general steps of what to do to implement support.
#48288

The USING option of SET DATA TYPE can actually specify any expression involving the old values of the row; that is, it can refer to other columns as well as the one being converted. This allows very general conversions to be done with the SET DATA TYPE syntax. Because of this flexibility, the USING expression is not applied to the column's default value (if any); the result might not be a constant expression as required for a default. This means that when there is no implicit or assignment cast from old to new type, SET DATA TYPE might fail to convert the default even though a USING clause is supplied. In such cases, drop the default with DROP DEFAULT, perform the ALTER TYPE, and then use SET DEFAULT to add a suitable new default. Similar considerations apply to indexes and constraints involving the column.

[thoszhang]

Thanks Richard.

This is mostly addressed to the other reviewers: Since there's already a PR out to implement this feature (#46933), should we try to merge this RFC soon? (I'd feel more comfortable giving the PR a more detailed review knowing that there are no more loose ends to tie up right now, and that open questions to be settled later are clearly defined.) I just have some minor comments remaining.

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

---

docs/RFCS/20200401_alter_column_type.md, line 71 at r5 (raw file):

If the swap mutation has happened - the type of the column is effectively changed, the only thing left to do is drop the old column.
    
# Index backfill method

Probably good to link to #47989 and indicate that the first iteration will just use 2 column backfills, since there are difficulties with interleaved tables with the index backfill approach.

---

docs/RFCS/20200401_alter_column_type.md, line 136 at r5 (raw file):

Inserts need to be validated by both the old and the new until the schema change is complete - meaning the old column is dropped and every node has the version of the TableDescriptor with the new column type. 

### Possible Solutions

I think this section could use a little more expansion. What are the pros and cons of each approach?

(There's also another approach: We could continue disallowing ALTER COLUMN TYPE on columns with constraints, and just fill in NULL values in the old column, which is in no danger of violating any constraints. But this is not very extensible.)

As I understand it, the current implementation PR takes the approach of disallowing writes, but it'd be good to clarify somewhere that this may not be the permanent solution we end up going with.

[RichardJCai]

Thanks Richard.

This is mostly addressed to the other reviewers: Since there's already a PR out to implement this feature (#46933), should we try to merge this RFC soon? (I'd feel more comfortable giving the PR a more detailed review knowing that there are no more loose ends to tie up right now, and that open questions to be settled later are clearly defined.) I just have some minor comments remaining.

Reviewed 1 of 1 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @otan, @RichardJCai, and @rohany)

docs/RFCS/20200401_alter_column_type.md, line 71 at r5 (raw file):

If the swap mutation has happened - the type of the column is effectively changed, the only thing left to do is drop the old column.
    
# Index backfill method

Probably good to link to #47989 and indicate that the first iteration will just use 2 column backfills, since there are difficulties with interleaved tables with the index backfill approach.

docs/RFCS/20200401_alter_column_type.md, line 136 at r5 (raw file):

Inserts need to be validated by both the old and the new until the schema change is complete - meaning the old column is dropped and every node has the version of the TableDescriptor with the new column type. 

### Possible Solutions

I think this section could use a little more expansion. What are the pros and cons of each approach?

(There's also another approach: We could continue disallowing ALTER COLUMN TYPE on columns with constraints, and just fill in NULL values in the old column, which is in no danger of violating any constraints. But this is not very extensible.)

As I understand it, the current implementation PR takes the approach of disallowing writes, but it'd be good to clarify somewhere that this may not be the permanent solution we end up going with.

For your last point, in my opinion writing NULL is questionable, as you said it's not extensible - we run into the issue if the column is not null or needs to be unique.

I will add more on the pros and cons and add more about constraints.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 4c0731b6.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[rohany]

I think that overall there aren't any open questions left in this RFC. After a reread, i just have nits about some presentation of details in the RFC.

[rohany]

[nit]: Complete sentences here.

[blathers-crl]

❌ The GitHub CI (Cockroach) build has failed on 9cb5189a.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[RichardJCai]

@lucy-zhang @rohany @jordanlewis
Anyone have any more thoughts about this RFC? Trying to get this RFC merged.

[rohany]

Thanks for all the cleanup -- LGTM.

[jordanlewis]

LGTM let's merge it!

[RichardJCai]

bors r=jordanlewis, rohany

[craig]

Build succeeded

• GitHub CI (Cockroach)