sql: fix reporting of stored and key columns in pg_attribute

[e-mbrown]

Epic: CRDB-23454
Resolves #88106
Resolves #88107

The primary key was stored in pg_attribute using its table attribute number and stored columns weren't included in pg_attribute. This commit changes the behavior to match Postgres.

Release note (bug fix): Fixed the calculation of the
pg_attribute.attnum column for indexes so that the attnum is always
based on the order the column appears in the index. Also fixed the
pg_attribute table so that it includes stored columns in secondary indexes.

[cockroach-teamcity]

This change is 

[rafiss]

nice work! just suggestions on comments and test changes

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown)

---

pkg/sql/pg_catalog.go line 470 at r1 (raw file):

		columnIdxMap := catalog.ColumnIDToOrdinalMap(table.PublicColumns())
		return catalog.ForEachIndex(table, catalog.IndexOpts{}, func(index catalog.Index) error {
			for i := 0; i < index.NumKeyColumns(); i++ {

nit: you can move idxID := h.IndexOid(table.GetID(), index.GetID()) to be above this loop, so you can reuse the variable in both loops

---

pkg/sql/pg_catalog.go line 474 at r1 (raw file):

				idxID := h.IndexOid(table.GetID(), index.GetID())
				column := table.PublicColumns()[columnIdxMap.GetDefault(colID)]
				if err := addColumn(column, idxID, uint32(i+1)); err != nil {

nit: add a comment saying the attnum for columns in an index is just the order it appears in the index definition, and is not related to the attnum the column has in the table

---

pkg/sql/pg_catalog.go line 479 at r1 (raw file):

			}

			for i := 0; i < index.NumSecondaryStoredColumns(); i++ {

nit: can you add a comment saying that pg_attribute only includes stored columns for secondary indexes, not for primary indexes

---

pkg/sql/pg_catalog.go line 483 at r1 (raw file):

				idxID := h.IndexOid(table.GetID(), index.GetID())
				column := table.PublicColumns()[columnIdxMap.GetDefault(colID)]
				if err := addColumn(column, idxID, uint32(i+1+index.NumKeyColumns())); err != nil {

nit: add a comment saying the attnum for columns in an index is just the order it appears in the index definition, and is not related to the attnum the column has in the table

---

pkg/sql/logictest/testdata/logic_test/pg_catalog line 6075 at r1 (raw file):

query OT colnames
SELECT i.indexrelid, c.relname FROM pg_index i

nit: we can skip this. the IDs aren't going to be reliable anyway; see below

---

pkg/sql/logictest/testdata/logic_test/pg_catalog line 6085 at r1 (raw file):

query TI colnames
select attname, attnum from pg_attribute where attrelid = 3975378684;

nit: let's use:

select attnum, attname from pg_attribute a join pg_class c on c.oid = a.attrelid where c.relname = 't1_pkey';

---

pkg/sql/logictest/testdata/logic_test/pg_catalog line 6091 at r1 (raw file):

query TI colnames
select attname, attnum from pg_attribute where attrelid = 2630301677;

nit: let's use:

select attnum, attname from pg_attribute a join pg_class c on c.oid = a.attrelid where c.relname = 't2_pkey';

---

pkg/sql/logictest/testdata/logic_test/pg_catalog line 6097 at r1 (raw file):

query TI colnames
select attname, attnum from pg_attribute where attrelid = 2370565405;

nit: let's use:

select attnum, attname from pg_attribute a join pg_class c on c.oid = a.attrelid where c.relname = 't3idx';

[rafiss]

could you update the release note? then lgtm!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown)

---

-- commits line 8 at r2:
nit: should be a bug fix, and something like "Fixed the calculation of the pg_attribute.attnum column for indexes so that the attnum is always based on the order the oclumn appears in the index. Also fixed the pg_attribute table so that it includes stored columns in secondary indexes."

[rafiss]

bors r=e-mbrown

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[rafiss]

bors r+

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[rafiss]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @e-mbrown)

---

pkg/sql/logictest/testdata/logic_test/pg_catalog line 6075 at r3 (raw file):

query IT colnames
select attnum, attname from pg_attribute a join pg_class c on c.oid = a.attrelid where c.relname = 't1_pkey';

looks like these are flaking in CI. could you add an order by attnum to all of these?

[e-mbrown]

bors r+

[craig]

👎 Rejected by code reviews

[rafiss]

lgtm!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from eddbc84 to blathers/backport-release-22.1-90287: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

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