sql: Add Logical Column ID field to ColumnDescriptor #46992

RichardJCai · 2020-04-03T17:17:38Z

The LogicalColumnID field mimics the ColumnID field however LogicalColumnID may be swapped
between two columns whereas ColumnID cannot. LogicalColumnID is referenced for virtual tables
(pg_catalog, information_schema) and most notably affects column ordering for SHOW COLUMNS.

This LogicalColumnID field support swapping the order of two columns - currently only used for
ALTER COLUMN TYPE when a shadow column is created and swapped with it's original column.

Does not affect existing behaviour.

Release note: None

cockroach-teamcity · 2020-04-03T17:17:45Z

This change is

rohany · 2020-04-06T20:51:43Z

Can you update this PR to include the representation upgrade path on descriptor read?

RichardJCai · 2020-04-06T21:07:49Z

Will do

pkg/sql/sqlbase/structured.go

rohany

Can you also add a test that writes a descriptor to disk without logical column IDs set, and ensures that when we read the descriptor back we get the logical col IDs populated?

RichardJCai · 2020-04-13T15:20:44Z

Can you also add a test that writes a descriptor to disk without logical column IDs set, and ensures that when we read the descriptor back we get the logical col IDs populated?

Created this test in logical_column_id_test.go

rohany · 2020-04-13T15:30:39Z

LGTM barring a review from @jordanlewis and comments on the new field on the column descriptor. I'd like to see:

A description of the new field
When it should be used/changed and what is the difference from column ID
A note that this doesn't exist on old descriptors

blathers-crl · 2020-04-13T22:07:20Z

❌ The GitHub CI (Cockroach) build has failed on 7afcfd0d.

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

blathers-crl · 2020-04-14T01:10:44Z

❌ The GitHub CI (Cockroach) build has failed on 7afcfd0d.

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

RichardJCai · 2020-04-14T14:54:14Z

LGTM barring a review from @jordanlewis and comments on the new field on the column descriptor. I'd like to see:

A description of the new field

When it should be used/changed and what is the difference from column ID

A note that this doesn't exist on old descriptors

Added these.

rohany · 2020-04-14T17:39:49Z

comment looks good, ty

rohany · 2020-04-15T02:52:22Z

Just thought of this -- allocate IDs should set this for all new columns

RichardJCai · 2020-04-15T15:08:10Z

Just thought of this -- allocate IDs should set this for all new columns

It currently does do this - AllocateIDs calls MaybeFillColumnIDs which fills out the ID and LogicalColumnID for new cols.

jordanlewis

Hmm... I don't think that this will work in all cases yet.

Isn't it the case that things will go wrong if you have a table with columns (a, b, c), and you re-type b to a new type, so it gets logical id 2 but physical id 4? What happens in that case if you do SELECT * FROM t or INSERT INTO t VALUES(1,2,3)? Can you write a test for this and see what happens?

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

pkg/sql/sqlbase/structured.proto, line 132 at r4 (raw file):

  // computed column.
  optional string compute_expr = 11;
  // LogicalColumnID is used to order columns visually. This is because

It's not just visually. It's also for the purposes of SELECT * and INSERT INTO without column specifiers, right?

rohany · 2020-04-15T21:29:48Z

It's not just visually. It's also for the purposes of SELECT * and INSERT INTO without column specifiers, right?

I think it's just for display in virtual tables -- we can safely use the column ordinal position in the other cases that you are thinking about (like selects or inserts).

RichardJCai · 2020-04-15T21:29:49Z

Hmm... I don't think that this will work in all cases yet.

Isn't it the case that things will go wrong if you have a table with columns (a, b, c), and you re-type b to a new type, so it gets logical id 2 but physical id 4? What happens in that case if you do SELECT * FROM t or INSERT INTO t VALUES(1,2,3)? Can you write a test for this and see what happens?

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

pkg/sql/sqlbase/structured.proto, line 132 at r4 (raw file):
  // computed column.
  optional string compute_expr = 11;
  // LogicalColumnID is used to order columns visually. This is because
It's not just visually. It's also for the purposes of SELECT * and INSERT INTO without column specifiers, right?

For the 1st point - I have a test for that in the WIP PR - it's working as intended.
Here's the test

For the 2nd point
No it doesn't affect the SELECT * and INSERT INTO orders - those rely on the ordering of the ColumnDescriptors in the TableDescriptor's columns slice.

jordanlewis

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

pkg/sql/sqlbase/structured.go, line 1215 at r4 (raw file):

// if the ColumnID is set and the LogicalColumnID is not
// it assigns the ColumnID as the LogicalColumnID.
func (desc *TableDescriptor) MaybeFillInLogicalColumnID() {

Rather than have this method, would it be better to only populate these lazily? You could have a method called GetLogicalColumnID that either returns the ID or the LogicalColumnID if it is set.

pkg/sql/sqlbase/structured.proto, line 132 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

For the 1st point - I have a test for that in the WIP PR - it's working as intended. Here's the test

For the 2nd point No it doesn't affect the SELECT * and INSERT INTO orders - those rely on the ordering of the ColumnDescriptors in the TableDescriptor's columns slice.

Ah okay. That test belongs in this PR then. Also, I would explain in this comment why having this ordering is necessary, on top of the ordering provided by the ColumnDescriptor order in the slice. It's too easy to forget.

pkg/sql/sqlbase/system.go, line 705 at r4 (raw file):

		Version:                 1,
		Columns: []ColumnDescriptor{
			{Name: "key", ID: 1, LogicalColumnID: 1, Type: *types.String},

This sort of thing is why I'm not super crazy about requiring LogicalColumnID. I feel like the lazily populated method might avoid this pain.

RichardJCai · 2020-04-15T21:57:27Z

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

pkg/sql/sqlbase/structured.go, line 1215 at r4 (raw file):
// if the ColumnID is set and the LogicalColumnID is not
// it assigns the ColumnID as the LogicalColumnID.
func (desc *TableDescriptor) MaybeFillInLogicalColumnID() {
Rather than have this method, would it be better to only populate these lazily? You could have a method called GetLogicalColumnID that either returns the ID or the LogicalColumnID if it is set.

pkg/sql/sqlbase/structured.proto, line 132 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…
Ah okay. That test belongs in this PR then. Also, I would explain in this comment why having this ordering is necessary, on top of the ordering provided by the ColumnDescriptor order in the slice. It's too easy to forget.

pkg/sql/sqlbase/system.go, line 705 at r4 (raw file):
		Version:                 1,
		Columns: []ColumnDescriptor{
			{Name: "key", ID: 1, LogicalColumnID: 1, Type: *types.String},
This sort of thing is why I'm not super crazy about requiring LogicalColumnID. I feel like the lazily populated method might avoid this pain.

Haha I actually used the lazy method (exactly how you described) in the initial PR.

Regarding having the test in this PR - that logic test depends on the ALTER COLUMN TYPE feature which doesn't exist in this PR. I can't think of an easy way to test the case you described without ALTER COLUMN TYPE right now.

blathers-crl · 2020-04-21T17:14:39Z

❌ The GitHub CI (Cockroach) build has failed on 018e3e2a.

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

rohany

LGTM with comment nits.

rohany · 2020-04-22T15:52:01Z

pkg/ccl/backupccl/restore_planning.go

@@ -389,6 +389,8 @@ func allocateTableRewrites(
 // backup descriptors provided. if skipFKsWithNoMatchingTable is set, FKs whose
 // "other" table is missing from the set provided are omitted during the
 // upgrade, instead of causing an error to be returned.
+// Also assigns LogicalColumnIDs for TableDescriptors that do not have it,


Comment is out of date

rohany · 2020-04-22T15:52:45Z

pkg/sql/sqlbase/structured.go

@@ -387,6 +387,7 @@ func GetTableDescFromIDWithFKsChanged(
 	if err != nil {
 		return nil, false, err
 	}
+


unnecessary changes

pkg/sql/sqlbase/structured.go

rohany · 2020-04-22T15:54:17Z

pkg/sql/sqlbase/structured.proto

@@ -129,6 +129,19 @@ message ColumnDescriptor {
  // Expression to use to compute the value of this column if this is a
  // computed column.
  optional string compute_expr = 11;
+  // LogicalColumnID is lazily set, do not access directly,


[nit]: Say something like "LogicalColumnID must be accessed through the accessor ... it is incorrect to access it directly.

rohany · 2020-04-22T15:54:41Z

pkg/sql/sqlbase/structured.proto

@@ -129,6 +129,19 @@ message ColumnDescriptor {
  // Expression to use to compute the value of this column if this is a
  // computed column.
  optional string compute_expr = 11;
+  // LogicalColumnID is lazily set, do not access directly,
+  // access using GetLogicalColumnID().
+  // LogicalColumnID is used to order columns visually. This is because


I think the example here could be shrunk to just mention ordering in catalog tables, rather than the specific case.

blathers-crl · 2020-04-22T16:28:36Z

❌ The GitHub CI (Cockroach) build has failed on 6bd31e4e.

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

blathers-crl · 2020-04-22T18:42:23Z

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

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

blathers-crl · 2020-04-22T19:07:46Z

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

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

blathers-crl · 2020-04-22T21:56:42Z

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

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

rohany · 2020-04-22T22:36:34Z

have you run make again? its failing because generated proto is different

RichardJCai · 2020-04-22T23:12:39Z

have you run make again? its failing because generated proto is different

Yeah, the proto binary was different cause I added a comment

blathers-crl · 2020-04-29T16:16:07Z

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

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

The LogicalColumnID field mimics the ColumnID field however LogicalColumnID may be swapped between two columns whereas ColumnID cannot. LogicalColumnID is referenced for virtual tables (pg_catalog, information_schema) and most notably affects column ordering for SHOW COLUMNS. This LogicalColumnID field support swapping the order of two columns - currently only used for ALTER COLUMN TYPE when a shadow column is created and swapped with it's original column. Does not affect existing behaviour. Release note: None

RichardJCai · 2020-04-29T18:52:08Z

bors r=rohany

craig · 2020-04-29T20:13:04Z

Build succeeded

GitHub CI (Cockroach)

RichardJCai force-pushed the column_descriptor_logical_id branch from 85fe761 to fed431e Compare April 3, 2020 21:38

rohany reviewed Apr 7, 2020

View reviewed changes

pkg/sql/sqlbase/structured.go Outdated Show resolved Hide resolved

rohany requested a review from jordanlewis April 7, 2020 18:10

RichardJCai force-pushed the column_descriptor_logical_id branch from 33d9dbc to 07ed952 Compare April 7, 2020 21:45

RichardJCai changed the title ~~WIP: Test Add Logical Column ID field to ColumnDescriptor~~ sql: Add Logical Column ID field to ColumnDescriptor Apr 7, 2020

rohany reviewed Apr 8, 2020

View reviewed changes

pkg/sql/sqlbase/structured.go Outdated Show resolved Hide resolved

rohany reviewed Apr 8, 2020

View reviewed changes

RichardJCai force-pushed the column_descriptor_logical_id branch from 07ed952 to 538ff8f Compare April 9, 2020 18:18

RichardJCai force-pushed the column_descriptor_logical_id branch from 538ff8f to 7afcfd0 Compare April 13, 2020 21:58

jordanlewis reviewed Apr 15, 2020

View reviewed changes

RichardJCai force-pushed the column_descriptor_logical_id branch from 7afcfd0 to 018e3e2 Compare April 21, 2020 17:03

RichardJCai mentioned this pull request Apr 21, 2020

sql (bug): adnum in pg_attrdef and attnum in pg_attribute do not match. #46799

Closed

RichardJCai requested review from jordanlewis and rohany April 22, 2020 00:05

RichardJCai force-pushed the column_descriptor_logical_id branch from 018e3e2 to 6bd31e4 Compare April 22, 2020 15:44

rohany approved these changes Apr 22, 2020

View reviewed changes

RichardJCai force-pushed the column_descriptor_logical_id branch from 6bd31e4 to a3f3cd9 Compare April 22, 2020 18:26

RichardJCai force-pushed the column_descriptor_logical_id branch from a3f3cd9 to 4e53cd8 Compare April 22, 2020 19:03

rohany approved these changes Apr 22, 2020

View reviewed changes

RichardJCai force-pushed the column_descriptor_logical_id branch from 4e53cd8 to fc8e63b Compare April 22, 2020 21:47

RichardJCai force-pushed the column_descriptor_logical_id branch from fc8e63b to 83b0073 Compare April 22, 2020 23:12

RichardJCai force-pushed the column_descriptor_logical_id branch 2 times, most recently from d473a31 to a0d0616 Compare April 29, 2020 15:50

RichardJCai force-pushed the column_descriptor_logical_id branch from a0d0616 to 3afec70 Compare April 29, 2020 17:35

craig bot merged commit 456d01b into cockroachdb:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: Add Logical Column ID field to ColumnDescriptor #46992

sql: Add Logical Column ID field to ColumnDescriptor #46992

RichardJCai commented Apr 3, 2020 •

edited

Loading

cockroach-teamcity commented Apr 3, 2020

rohany commented Apr 6, 2020

RichardJCai commented Apr 6, 2020

rohany left a comment

RichardJCai commented Apr 13, 2020

rohany commented Apr 13, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 14, 2020

RichardJCai commented Apr 14, 2020

rohany commented Apr 14, 2020

rohany commented Apr 15, 2020

RichardJCai commented Apr 15, 2020

jordanlewis left a comment

rohany commented Apr 15, 2020

RichardJCai commented Apr 15, 2020

jordanlewis left a comment

RichardJCai commented Apr 15, 2020

blathers-crl bot commented Apr 21, 2020

rohany left a comment

rohany Apr 22, 2020

rohany Apr 22, 2020

rohany Apr 22, 2020

rohany Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

rohany commented Apr 22, 2020

RichardJCai commented Apr 22, 2020

blathers-crl bot commented Apr 29, 2020

RichardJCai commented Apr 29, 2020

craig bot commented Apr 29, 2020

sql: Add Logical Column ID field to ColumnDescriptor #46992

sql: Add Logical Column ID field to ColumnDescriptor #46992

Conversation

RichardJCai commented Apr 3, 2020 • edited Loading

cockroach-teamcity commented Apr 3, 2020

rohany commented Apr 6, 2020

RichardJCai commented Apr 6, 2020

rohany left a comment

Choose a reason for hiding this comment

RichardJCai commented Apr 13, 2020

rohany commented Apr 13, 2020

blathers-crl bot commented Apr 13, 2020

blathers-crl bot commented Apr 14, 2020

RichardJCai commented Apr 14, 2020

rohany commented Apr 14, 2020

rohany commented Apr 15, 2020

RichardJCai commented Apr 15, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

rohany commented Apr 15, 2020

RichardJCai commented Apr 15, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

RichardJCai commented Apr 15, 2020

blathers-crl bot commented Apr 21, 2020

rohany left a comment

Choose a reason for hiding this comment

rohany Apr 22, 2020

Choose a reason for hiding this comment

rohany Apr 22, 2020

Choose a reason for hiding this comment

rohany Apr 22, 2020

Choose a reason for hiding this comment

rohany Apr 22, 2020

Choose a reason for hiding this comment

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

blathers-crl bot commented Apr 22, 2020

rohany commented Apr 22, 2020

RichardJCai commented Apr 22, 2020

blathers-crl bot commented Apr 29, 2020

RichardJCai commented Apr 29, 2020

craig bot commented Apr 29, 2020

Build succeeded

RichardJCai commented Apr 3, 2020 •

edited

Loading