sql: don't copy old pk when changing bucket size on sharded index

[rohany]

Fixes #45889.

Release note (sql change): This PR causes primary key changes to
not create a copy of the old primary key if the primary key change
only changes the bucket count of a hash sharded index.

[cockroach-teamcity]

This change is 

[otan]

Just for sanity - why do we not want this behaviour?

[rohany]

The argument is as follows:

We make a copy of the old primary key so that your query performance doesn't regress after the primary key change. However, in the hash sharded case, the change being made is going to affect performance (i.e. what is the bucket count, or what columns are hashed). Keeping around the old primary key means keeping around the old primary key with the incorrect number of buckets etc. What does @ajwerner think about this?

[ajwerner]

I agree with @rohany here, at least for the tested case. I can't see any reason why a user would want to retain the old bucket count.

Is there are case where the user would want to retain the index if they changed the set of columns but both were hashed? That one is less clear to me.

Consider:

CREATE TABLE t (x INT NOT NULL, y INT NOT NULL, ts TIMESTAMPTZ NOT NULL, PRIMARY KEY (ts, x, y) USING HASH WITH BUCKET_COUNT = 2);

then:

ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (ts, y, x) USING HASH WITH BUCKET_COUNT=3

I could see it either way in this case.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @otan)

[otan]

nit: maybe updated comment for hash sharded cases too

[jordanlewis]

Does this want to be merged?

[rohany]

Reviving this PR! I am more satisfied with it now -- I changed it to only not create a copy if only the bucket count of the hash sharded index is changing. PTAL

[otan]

LGTM. Completely optional bikeshedding comment.

[otan]

this was a teensy bit hard to read but makes sense after squinting.

wondering if breaking down to the if statements might be better... but it's personal taste for me.

func shouldCopyPrimaryKey(desc *MutableTableDescriptor, newPK *sqlbase.IndexDescriptor) bool {
  // no primary key means we have nothing to copy
  if !desc.HasPrimaryKey() {
     return false
  }
  // we never want to copy default row id
  if desc.IsPrimaryIndexDefaultRowID() {
    return false
  }
  if oldPK.IsSharded() && newPK.IsSharded() {
     // ...
  }
  return true
}
```

[rohany]

I'm down for this, I find it hard to read as well.

[blathers-crl]

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

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

[rohany]

bors r=otan

[craig]

Build succeeded

• GitHub CI (Cockroach)