sql: populate Oid on type upgrade #47234
Conversation
|
This change is |
pkg/sql/types/types.go
Outdated
| @@ -1630,6 +1630,7 @@ func (t *T) upgradeType() error { | |||
| if t.InternalType.Precision > 0 { | |||
| t.InternalType.TimePrecisionIsSet = true | |||
| } | |||
| t.InternalType.Oid = familyToOid[t.Family()] | |||
| case IntervalFamily: | |||
There was a problem hiding this comment.
We also need this same thing for Intervals.
There was a problem hiding this comment.
Actually, I have a better idea. Can you instead remove the default case, and move it into an block after the switch statement, conditional on the Oid still being 0? That would protect us from future problems like this.
There was a problem hiding this comment.
should be able to add tests to this as well (there are upgradeType tests in types_test.go)
There was a problem hiding this comment.
I added tests that test an empty type with just the family always gets the correct Oid set - this was failing on timestamps, intervals and arrays before. I also added a test that the Oid for a BitFamily type with visible varbit was not set to the default family ID, but was set to the correct one.
|
❌ The GitHub CI (Cockroach) build has failed on 4c9ae226. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. I am experimental - my owner is otan. |
pkg/sql/types/types.go
Outdated
| @@ -1630,6 +1630,7 @@ func (t *T) upgradeType() error { | |||
| if t.InternalType.Precision > 0 { | |||
| t.InternalType.TimePrecisionIsSet = true | |||
| } | |||
| t.InternalType.Oid = familyToOid[t.Family()] | |||
| case IntervalFamily: | |||
There was a problem hiding this comment.
should be able to add tests to this as well (there are upgradeType tests in types_test.go)
pbardea
force-pushed
the
oid-timestamp
branch
5 times, most recently
from
d60054a
to
6ab365f
Compare
When types are unmarshalled, the types are upgraded and the Oid of the internal type should be set. In 19.1, timestamps received an Oid and they were populated by the default case in upgradeTypes. However, a case to handle timestamps was added, but the Oids were no longer set (as they didn't need to be anymore). However, this meant that default case was not triggered anymore, so the Oid was no longer set. This commit moves the setting of the Oid from the default case to after the switch statement and sets the Oid based on the type family if none has been set yet. Release note (bug fix): Restoring a backup from 2.1 to 20.1 with a timestamp column would result in incomplete type data. This would crash commands such as SHOW COLUMNS FROM <new_table>. This is now fixed.
pbardea
changed the title
|
TFTRs! |
Build succeeded |
When types are unmarshalled, the types are upgraded and the Oid of the
internal type should be set. In 19.1, timestamps received an Oid and
they were populated by the default case in upgradeTypes. However,
a case to handle timestamps was added, but the Oids were no longer
set (as they didn't need to be anymore). However, this meant that
default case was not triggered anymore, so the Oid was no longer
set. This commit also sets the Oid in the timestamp case.
Fixes #44453.
Release note (bug fix): Restoring a backup from 2.1 to 20.1 with a
timestamp column would result in incomplete type data. This would
crash commands such as SHOW COLUMNS FROM <new_table>. This is now
fixed.