sql: consider virtual PK columns as stored in the optimizer

[mgartner]

Allowing virtual primary key columns created a contradiction of two
hard-coded assumptions of the optimizer:

1. Primary key columns are written to the primary index (and all
   secondary indexes).
2. Virtual columns are not written to the primary index.

To prevent this contradiction from causing bugs while planning, the opt
catalog presents columns that are marked as virtual PK columns in the
descriptor as stored columns to the optimizer. This is valid because
virtual computed PK columns behave exactly like stored computed PK
columns: they are written to all indexes.

The test catalog has been updated to mimic this behavior.

Fixes #75147
Fixes #75874

Release note (bug fix): A bug has been fixed that caused internal errors
when querying tables with virtual columns in the primary key. This bug
was only present since version 22.1.0-alpha.1 and does not appear in any
production releases.

[cockroach-teamcity]

This change is 

[RaduBerinde]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

---

pkg/sql/opt_catalog.go, line 664 at r1 (raw file):

	// Determine the primary key columns.
	primaryIndex := desc.GetPrimaryIndex()

[nit] there is a CollectKeyColumnIDs method

---

pkg/sql/logictest/testdata/logic_test/virtual_columns, line 1257 at r1 (raw file):

  INDEX (a)
);

[nit] blank line

[chengxiong-ruan]

thanks for fixing this!

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner and @msirek)

[msirek]

Nice Solution.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @mgartner, @msirek, and @RaduBerinde)

---

pkg/sql/opt/testutils/testcat/create_table.go, line 106 at r1 (raw file):

					def.Nullable.Nullability = tree.NotNull
					if def.Computed.Computed {
						def.Computed.Virtual = false

I saw an error in the hash_sharded index test. Could it be from this line? Maybe this needs something like what you did in opt_catalog.go with pkCols instead of set Virtual explicitly so there are no side effects.

[mgartner]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek and @RaduBerinde)

---

pkg/sql/opt_catalog.go, line 664 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] there is a CollectKeyColumnIDs method

Done.

---

pkg/sql/opt/testutils/testcat/create_table.go, line 106 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

I saw an error in the hash_sharded index test. Could it be from this line? Maybe this needs something like what you did in opt_catalog.go with pkCols instead of set Virtual explicitly so there are no side effects.

The failures are in logic tests, which do not use the test catalog. The test catalog is only used in optimizer tests, which are most tests in pkg/sql/opt/**/testdata/*, except the logic tests that live in pkg/sql/opt/exec/exec/execbuilder. The test catalog mimics the opt catalog by implementing the cat.Table, cat.Index, etc. interfaces, so optimizer tests don't have to start up a full server and deal with real descriptors. One benefit is that optimizer tests run faster than logic tests.

The failing tests are a bit odd in that they are logic tests that print the optimizer catalog. I don't think we typically do this. In any case, to fix them I just needed to update the expected output with the -rewrite flag.

---

pkg/sql/logictest/testdata/logic_test/virtual_columns, line 1257 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] blank line

Done.

[msirek]

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @msirek and @RaduBerinde)

---

pkg/sql/opt/testutils/testcat/create_table.go, line 106 at r1 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

The failures are in logic tests, which do not use the test catalog. The test catalog is only used in optimizer tests, which are most tests in pkg/sql/opt/**/testdata/*, except the logic tests that live in pkg/sql/opt/exec/exec/execbuilder. The test catalog mimics the opt catalog by implementing the cat.Table, cat.Index, etc. interfaces, so optimizer tests don't have to start up a full server and deal with real descriptors. One benefit is that optimizer tests run faster than logic tests.

The failing tests are a bit odd in that they are logic tests that print the optimizer catalog. I don't think we typically do this. In any case, to fix them I just needed to update the expected output with the -rewrite flag.

OK. thanks. I hadn't gotten a chance to look into the code that deeply. I just wanted to make sure in the test catalog path it's not creating the table with the column as virtual, and based on your description it's not actually creating the table with full descriptors, etc.

[mgartner]

TFTRs!

bors r+

[craig]

Canceled.

[mgartner]

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)