From eb64a92e0509308210e2317248580dddb36fb996 Mon Sep 17 00:00:00 2001
From: Rafi Shamim <rafi@cockroachlabs.com>
Date: Fri, 8 Mar 2024 13:04:23 -0500
Subject: [PATCH] catalog: add maybeAddConstraintIDs to RestoreChanges

All backups taken in versions 22.1 and later should already have
constraint IDs set. Technically, now that we are in v24.1, we no longer
support restoring from versions older than 22.1, but we still have
tests that restore from old versions. Until those fixtures are updated,
we need to handle the case where constraint IDs are not set already when
restoring a backup.

To prove that this works, I've resurrected a different test that
restores from a v21.1 backup, which was earlier deleted in 8b56efd931a.

Release note: None
---
 .../testdata/backup-restore/show_backup       |   9 ++
 .../invalidDependOnBy_21.1/BACKUP_MANIFEST    | Bin 0 -> 2970 bytes
 .../BACKUP_MANIFEST-CHECKSUM                  |   1 +
 .../catalog/post_deserialization_changes.go   |   4 +
 .../catalog/tabledesc/table_desc_builder.go   | 106 ++++++++++++++++++
 5 files changed, 120 insertions(+)
 create mode 100644 pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST
 create mode 100644 pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST-CHECKSUM

diff --git a/pkg/ccl/backupccl/testdata/backup-restore/show_backup b/pkg/ccl/backupccl/testdata/backup-restore/show_backup
index 3328738e5b31..5811ee1f095a 100644
--- a/pkg/ccl/backupccl/testdata/backup-restore/show_backup
+++ b/pkg/ccl/backupccl/testdata/backup-restore/show_backup
@@ -4,6 +4,15 @@
 new-cluster name=s1 allow-implicit-access localities=eu-central-1,eu-north-1,us-east-1
 ----
 
+link-backup cluster=s1 src-path=show_backup_validate,invalidDependOnBy_21.1 dest-path=invalidDependOnBy_21.1
+----
+
+# This backup intentionally has a dangling invalid depend on by reference.
+query-sql regex=invalid\sdepended-on-by
+SELECT * FROM [SHOW BACKUP VALIDATE FROM 'invalidDependOnBy_21.1' IN 'nodelocal://1/'];
+----
+true
+
 link-backup cluster=s1 src-path=show_backup_validate,valid-22.2 dest-path=valid-22.2
 ----
 
diff --git a/pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST b/pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST
new file mode 100644
index 0000000000000000000000000000000000000000..07ed4fc32828774086404aeb74d42592dc461872
GIT binary patch
literal 2970
zcmV;L3uW{liwFP!00000|GZg!Y#hZEpPAjg+Z)@-#v%Av3M?y;oHzDmZ|}pGs|t=&
z+87du&IVMg>ScYqb`H+Fo7=s_=C1&)#DyfPPzxy%ja$(Im5@+Mr8EMmO4XL4@<*$x
zO)Iscs?-liTN(rusYum++}_9D`7U-!{>gf7-pud)esA8qw~N6rrZ4^GgI|CB?1zty
zur!NK)_8RCg#C1?#-mft)9D(IPCHLe)p+!j^YnC$M^B%!pPsGp=vim%Y>h`}Pufqb
zH6E=xV=vZt^rG|hQjJG1Jz+n+QsdDp&e*Fp9=+;3y;kGVYbW>w_Aw0KsK}PQYa*M<
zWGAF-Dw~-|CR2&jMBFqjiKL4R%7H1sOV|h7ZZeTdCdCOU<C;+PWysJC8`rJ1x1s1C
zK!$K^QrLI9%_c1+Q<>eF#O}0X6Q;kzGvO%H*vX|Ru#=e1BqdQycsqRsF_dGI#6Fop
zkEB#OE$w#Eh3GFaFp3)RT#navDNKI^85o6*mBY8ILc3F`bT%vPP9{<w-&+~iB!hjq
z);5XItN%JqF{>AEw(W$^41XGj%oWp8ml(+2gd>=em8Fu#h?vG?Mb|Ax940kgQKN{*
z^y)9aR(tfXA3Qb^gOlJcAiPiDD+H#hrdlzhBnM+K*EDHr9Mkqsn!KbkkR7DM42S{|
z9K7KW;D87Y0|P9B;}AR!!5Ijql6cc$**vT&2W8VdrdJdOv4b@28ErWPI2bgNaHwSF
z^|Hzk>>v%i$&9^&KrP5+Qw`YcBl9{A8<o<MTv@SM=Y5z$8v?;wT(tdV?B!X&ffx`$
z7EFRUU?78dn4|&ct^QR=o`LY&Fhql45Ctzla1MeC5FA1e-hy$R+;<5a@z{NrsMC8K
z!p9+T#<sYjfex@fHV($XIXH%|TacFuQXxQ98*~oE&^h=iJZMi(7JQOCk2oSmoQLE^
z2>u4a6-e5E{{z8s1fE3T8Pp`>IRwrl@FR57>j=Dsz`F<te?j=q2vZO1nrcR=fbKgm
zDk+ZQkePJ!xURF}J8`I}YYU~tK9t@^<R55N4V4tZv+u6YwO40rvpT4xxVm^mO9kH{
z51OsRc}T2}=;uiMEfUkFYFQ<1(Ts)yT#sV1-}5D4>Zn|<st(yb=6)6rlx5Sp&roDb
zRTzRDqAAbf_q21kwrKjX*k~@7o1H#z=gjP$0|$ev#Bs#gadyQ}8M1BH4iZy&t5ssh
zH^GH1G$Rrk3$JxH4f5LS{{_6=R$46B*a{AeLKCt=!(+fY_*oY}+l-%W;O8KRID#W%
z;5>xu3zIhq{04zhwG`bH2#^>i9#U7XIzD=&-9)eW=#+IATE(?-b<*~eO}5uHA7>kq
zd!AW5;GX9?B`#1mt8T|z+^pKlL9-JZ%CD~t;R{2Ud{Cb^qniVYPIOMM!ltTLI#N{&
z75!LAiH&wlx27dqRkNQAFp0y(in@{0KZe7Gyi(R>#X0-U5!GS8aZDQ(eX*jNCPVcO
zz!cu>!7I$K6fFC=g?c8KDG3iXS(Owf+{R)Dn>1#MWx2FeFjdnm>6&du_Xe23TU@h}
zW?HgVRBgLR@9oAhtyPx_m8xbkxcBrprtPS+=Q9KCI(0~~gZV$g4%es~7eZZfp{|_Z
ziz2l1EwoZ81XvP+1PH!wf)9J`{B2N1$Os#>5izha(9iLoGw_Igatf|bg*wWTvdo|;
z;5}dYy00P<i7cE1ol&&zXs3)0JD!1fyR&5*MTHiI90WuZ@a;_UZA^Ly#gG{84y?Vj
z=>qL#gohnym8#}2VmdI$DBuHv1*mC=)FfO`RgrFR*ahr>fsIHXfFFS_1aA5sF1Djm
z)wGhfxFeU#&CKSf@A>=~x1*vSRV!BesG({~U`(!>>Uc#xQdLb$Ra$;4+R)3LejGhS
zUCs5O<6GQnpro`1Ts`?YRU+$g%htJN>qm@j4+pk29P0G}7vjS1&D2PgkoBKADn^}y
z`!VIn!yq;wFB?>j^<`y!sX3a9aF`pg>ydSqtv@C4k4YRU>qXfr>6#hc7P!PjJYfG^
zE?Fyg_g9`sc&Ki1f4^m{lO3&+RaI2y79*I=(P0rBd~T@o{9QI^ZP7I#dmHSd@{hKq
z(`L8$R>Xo2Mrdp!LJ@U6Hvg9b_6U~TkZnR*XaQQN<194T0Bo~oY^!Ii?-|?h>>3od
z$0__Vi3clsSuHH7OY>^QjE;2bf$SOm1kp|$a@Xl6dd6_LWbSb|4ZK|FqrP=O@!ozy
zeW|te?(2dp;BC!}6r4Zm*PFAe2(xGd_gN8-v~1x1@84gZOCrj4xw`w=<Pdo^F?&mM
zmF~F6dMNBDv(w_uc&PrSqb^yU@(tDEr%C(-iRq%gw4`d58I5)dKd1ie<Ap~YNA&pz
z)uLq=gQ%|;BxZHqo=o(fJch#_%0RE(F>_Vh!qq878u)l`YoWfX9l0X0#wFixuG-n6
zJH{cSI$tgo8E)+u7;A|E=ca6+Wj(ZR(rF5aRG~pi-4l+Ck>??`zJT~1fuAS5oTMA>
z{dVg{-+k`HA$P5Q@{-@E6TM%lkom)1-gWZcj$RAC@w{|(x6oL}mz=Bzs@`t9Y7*<d
zKvpC8*$Ccb77wdRwX7-yr>DLxAlD=wsLv^^uep}+kiUYXdLKegwf5K%Z}w`$*>Y>3
z$DW<*7!3L`O|A5C=IwGwQ<vRlk!s(tLvxvIFy|DvZjasbjeLU$b=jymJ$8@2JdGLO
zMo}%fR5ttRu_ZjDE~~|=rB~cn0Fl1FGPpK&Q7l&$-7b6WmlOL2@_%YC^P0j%XjQ(^
zMZ3xfKIuXmokA;9LVyIJ)jy#PdjG9+Uv<SC9;_qv%LyO5NNwo0C$1(pWHIG1VHcfE
zJhMI+wv9**augS`v;H!sbnCEMVIbS8N~7^|RbgAXX9L``t=zM1+<Q$~*lm~W8W*iE
zMZHDfKM?qOMYiO5*;L(X&VS=S)~R#y=X$AX(2HKSYt`W_+HYFAz466?A=sPf&F=JU
z_~5_8b1l~0D&Zq-houQH-@qdH+kDcOPrCW?X-3C3(6K(c4c|gj!;?6os0(tnY$@}M
z)cmCt`CnSmu=Bt3rS*kxUi{(7#+TMPAnX~w1Jj0XT6VY3BzrG$>C0!IKh?6tWrY8Z
zFtQTS6eq=q9Hw7*>gVUa@s1r;+cDGv*ItK|o5PfI1qQ3vz!eFDU-0g0jSF`v0cv8G
zZpDCoYQHucP0HWctUT0M7lPA7Kv&;iL7Gl8APPC>G2=dVQT@zsFI;%Di|YR+@J9qj
zM-$OBC&f?>)0f^m{m=K`z8bp6F?~l-FISheI|7WarB0w-=&cT!m%FGV_!#W+zq3w#
ze`)PosU2-f`5>A!vnzc^wJ*kI?%aRp-dRo?<0S5L`Fjs={&|bL?M2*5jB)vUADD4}
zjxDO1T9GX~*H%iZ8B64Hx#@dmvtwLZwEua3j*gDL4pUv^f9stOPh5VxRetEop9>8G
z7EFOjco%bJ|4H_}mnLK1{axWFTPycpp1TAwy{ISRiEKPQJaSY@#HDy5F`i71FYeCB
z(t?;&7c%TtNtC2rVs@99<PynTGMN)4KGv9!Nu@H`;({cp^J*fIky3Ixosm>UQj)t9
zNnytx!zioV{{8nK;1bDrBA(%5#j>m|jwh0dOk9jR+ongQK}D_{D`}j(q-0V#V(;Gl
zQv-^6R4wa<-7Vb}u4)fy`Y|nk!*VuL$fU+)d*N}{N=n2tadECbXMTV_|I8y4^XkQ$
zqgyLTN}hn=NywPDUzxuCXZPm6|1p1H>K7ybcp1VAa2rhyZ<*S&_wM@+j?c{QnVp`Q
zo!&cRk5gU7LtVx<bs68>Wqezg@sTd$(Jte+b{W5|%lKHA@l==bbcgXbAWd(xYcme~
QF8~1l|Lo_G5i2GD0Eh(6$p8QV

literal 0
HcmV?d00001

diff --git a/pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST-CHECKSUM b/pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST-CHECKSUM
new file mode 100644
index 000000000000..70747a396825
--- /dev/null
+++ b/pkg/ccl/backupccl/testdata/show_backup_validate/invalidDependOnBy_21.1/BACKUP_MANIFEST-CHECKSUM
@@ -0,0 +1 @@
+'Üóÿ
\ No newline at end of file
diff --git a/pkg/sql/catalog/post_deserialization_changes.go b/pkg/sql/catalog/post_deserialization_changes.go
index 191588f12491..5d6b51df449b 100644
--- a/pkg/sql/catalog/post_deserialization_changes.go
+++ b/pkg/sql/catalog/post_deserialization_changes.go
@@ -82,6 +82,10 @@ const (
 	// references.
 	RemovedDuplicateIDsInRefs
 
+	// AddedConstraintIDs indicates that table descriptors had constraint ID
+	// added.
+	AddedConstraintIDs
+
 	// RemovedSelfEntryInSchemas corresponds to a change which occurred in
 	// database descriptors to recover from an earlier bug whereby when
 	// dropping a schema, we'd mark the database itself as though it was the
diff --git a/pkg/sql/catalog/tabledesc/table_desc_builder.go b/pkg/sql/catalog/tabledesc/table_desc_builder.go
index c168f5fdd346..6a72c2e8d1fa 100644
--- a/pkg/sql/catalog/tabledesc/table_desc_builder.go
+++ b/pkg/sql/catalog/tabledesc/table_desc_builder.go
@@ -178,6 +178,16 @@ func (tdb *tableDescriptorBuilder) RunRestoreChanges(
 		tdb.changes.Add(catalog.UpgradedSequenceReference)
 	}
 
+	// All backups taken in versions 22.1 and later should already have
+	// constraint IDs set. Technically, now that we are in v24.1, we no longer
+	// support restoring from versions older than 22.1, but we still have
+	// tests that restore from old versions. Until those fixtures are updated,
+	// we need to handle the case where constraint IDs are not set already.
+	// TODO(rafi): remove this once all fixtures are updated. See: https://github.com/cockroachdb/cockroach/issues/120146.
+	if changed := maybeAddConstraintIDs(tdb.maybeModified); changed {
+		tdb.changes.Add(catalog.AddedConstraintIDs)
+	}
+
 	// Upgrade the declarative schema changer state
 	if scpb.MigrateDescriptorState(version, tdb.maybeModified.ParentID, tdb.maybeModified.DeclarativeSchemaChangerState) {
 		tdb.changes.Add(catalog.UpgradedDeclarativeSchemaChangerState)
@@ -778,6 +788,102 @@ func maybeFixPrimaryIndexEncoding(idx *descpb.IndexDescriptor) (hasChanged bool)
 	return true
 }
 
+// maybeAddConstraintIDs ensures that all constraints have an ID associated with
+// them.
+func maybeAddConstraintIDs(desc *descpb.TableDescriptor) (hasChanged bool) {
+	// Only assign constraint IDs to physical tables.
+	if !desc.IsTable() {
+		return false
+	}
+	// Collect pointers to constraint ID variables.
+	var idPtrs []*descpb.ConstraintID
+	if len(desc.PrimaryIndex.KeyColumnIDs) > 0 {
+		idPtrs = append(idPtrs, &desc.PrimaryIndex.ConstraintID)
+	}
+	for i := range desc.Indexes {
+		idx := &desc.Indexes[i]
+		if !idx.Unique || idx.UseDeletePreservingEncoding {
+			continue
+		}
+		idPtrs = append(idPtrs, &idx.ConstraintID)
+	}
+	checkByName := make(map[string]*descpb.TableDescriptor_CheckConstraint)
+	for i := range desc.Checks {
+		ck := desc.Checks[i]
+		idPtrs = append(idPtrs, &ck.ConstraintID)
+		checkByName[ck.Name] = ck
+	}
+	fkByName := make(map[string]*descpb.ForeignKeyConstraint)
+	for i := range desc.OutboundFKs {
+		fk := &desc.OutboundFKs[i]
+		idPtrs = append(idPtrs, &fk.ConstraintID)
+		fkByName[fk.Name] = fk
+	}
+	for i := range desc.InboundFKs {
+		idPtrs = append(idPtrs, &desc.InboundFKs[i].ConstraintID)
+	}
+	uwoiByName := make(map[string]*descpb.UniqueWithoutIndexConstraint)
+	for i := range desc.UniqueWithoutIndexConstraints {
+		uwoi := &desc.UniqueWithoutIndexConstraints[i]
+		idPtrs = append(idPtrs, &uwoi.ConstraintID)
+		uwoiByName[uwoi.Name] = uwoi
+	}
+	for _, m := range desc.GetMutations() {
+		if idx := m.GetIndex(); idx != nil && idx.Unique && !idx.UseDeletePreservingEncoding {
+			idPtrs = append(idPtrs, &idx.ConstraintID)
+		} else if c := m.GetConstraint(); c != nil {
+			switch c.ConstraintType {
+			case descpb.ConstraintToUpdate_CHECK, descpb.ConstraintToUpdate_NOT_NULL:
+				idPtrs = append(idPtrs, &c.Check.ConstraintID)
+			case descpb.ConstraintToUpdate_FOREIGN_KEY:
+				idPtrs = append(idPtrs, &c.ForeignKey.ConstraintID)
+			case descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX:
+				idPtrs = append(idPtrs, &c.UniqueWithoutIndexConstraint.ConstraintID)
+			}
+		}
+	}
+	// Set constraint ID counter to sane initial value.
+	var maxID descpb.ConstraintID
+	for _, p := range idPtrs {
+		if id := *p; id > maxID {
+			maxID = id
+		}
+	}
+	if desc.NextConstraintID <= maxID {
+		desc.NextConstraintID = maxID + 1
+		hasChanged = true
+	}
+	// Update zero constraint IDs using counter.
+	for _, p := range idPtrs {
+		if *p != 0 {
+			continue
+		}
+		*p = desc.NextConstraintID
+		desc.NextConstraintID++
+		hasChanged = true
+	}
+	// Reconcile constraint IDs between enforced slice and mutation.
+	for _, m := range desc.GetMutations() {
+		if c := m.GetConstraint(); c != nil {
+			switch c.ConstraintType {
+			case descpb.ConstraintToUpdate_CHECK, descpb.ConstraintToUpdate_NOT_NULL:
+				if other, ok := checkByName[c.Check.Name]; ok {
+					c.Check.ConstraintID = other.ConstraintID
+				}
+			case descpb.ConstraintToUpdate_FOREIGN_KEY:
+				if other, ok := fkByName[c.ForeignKey.Name]; ok {
+					c.ForeignKey.ConstraintID = other.ConstraintID
+				}
+			case descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX:
+				if other, ok := uwoiByName[c.UniqueWithoutIndexConstraint.Name]; ok {
+					c.UniqueWithoutIndexConstraint.ConstraintID = other.ConstraintID
+				}
+			}
+		}
+	}
+	return hasChanged
+}
+
 // maybeRemoveDuplicateIDsInRefs ensures that IDs in references to other tables
 // are not duplicated.
 func maybeRemoveDuplicateIDsInRefs(d *descpb.TableDescriptor) (hasChanged bool) {