Skip to content

Commit

Permalink
sql/catalog/tabledesc: permit zero-valued column IDs in DependedOnBy
Browse files Browse the repository at this point in the history
In 21.1 there was a bug whereby we would store a 0-value column ID in the
sequence's DependedOnBy because we'd add the depedency before allocating
an ID to the column. This bug was fixed in 21.2. Below is a reproduction
I've used to play with this bug:

```bash
roachprod wipe local
roachprod stage local release v21.1.3
roachprod run local -- mv cockroach cockroach-v21.1.3
roachprod stage local release v21.2.10
roachprod run local -- mv cockroach cockroach-v21.2.10
roachprod stage local release v22.1.1
roachprod run local -- mv cockroach cockroach-v22.1.1
roachprod start local --binary cockroach-v21.1.3
roachprod sql local -- -e "create table t1( i int primary key);
                           create table t2(i int primary key);
                           create sequence s1;
                           create sequence s2;
                           ALTER TABLE t1 ADD c1 BIGINT DEFAULT nextval('s1') NOT NULL;
                           ALTER TABLE t1 ADD c2 BIGINT DEFAULT nextval('s2') NOT NULL;
                           ALTER TABLE t2 ADD c1 BIGINT DEFAULT nextval('s1') NOT NULL;"
roachprod stop local
roachprod start local --binary cockroach-v21.2.10
while ! { roachprod sql local -- -e 'show cluster setting version' | grep 21.2 ; }; do sleep 1; done

roachprod sql local -- -e "alter table t1 add column c3 int default nextval('s1');
                           create table t3 (i int primary key);
                           alter table t3 add column c1 int default nextval('s1');"

roachprod stop local
roachprod start local --binary cockroach-v22.1.1
while ! { roachprod sql local -- -e 'show cluster setting version' | grep 22.1 ; }; do sleep 1; done
```

For now, for the rest of 22.1 we'll let this slide. As follow-up work, we'll
perform a migration to repair this situation and add back this validation once
the migration has been performed.

Release note (bug fix): In earlier 22.1 releases of cockroach, added validation
could cause problems for descriptors which carried invalid backreferences due
to an earlier bug in 21.1. This stricter validation could result in a variety
of query failures. This patch weakens the validation to permit the corruption
as we know it. A subsequent patch in 22.2 will be created to repair the invalid
reference.
  • Loading branch information
ajwerner committed Jun 13, 2022
1 parent 34d3248 commit 5f38e71
Show file tree
Hide file tree
Showing 2 changed files with 41 additions and 0 deletions.
7 changes: 7 additions & 0 deletions pkg/sql/catalog/tabledesc/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,13 @@ func (desc *wrapper) validateInboundTableRef(
// descriptor is for a sequence. In this case, they refer to the columns
// in the referenced descriptor instead.
for _, colID := range by.ColumnIDs {
// Skip this check if the column ID is zero. This can happen due to
// bugs in 20.2.
//
// TODO(ajwerner): Make sure that a migration in 22.2 fixes this issue.
if colID == 0 {
continue
}
col, _ := backReferencedTable.FindColumnWithID(colID)
if col == nil {
return errors.AssertionFailedf("depended-on-by relation %q (%d) does not have a column with ID %d",
Expand Down
34 changes: 34 additions & 0 deletions pkg/sql/catalog/tabledesc/validate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2491,6 +2491,40 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
}},
},
{ // 14
// This case deals with a bug in version 21.1 and prior when
// ALTER TABLE ... ADD COLUMN ... DEFAULT nextval(...) would set the
// backreference ID to be 0 because it set up the backreference before
// calling AllocateIDs.
desc: descpb.TableDescriptor{
Name: "foo",
ID: 51,
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
SequenceOpts: &descpb.TableDescriptor_SequenceOpts{
Increment: 1,
},
DependedOnBy: []descpb.TableDescriptor_Reference{
{ID: 52, ColumnIDs: []descpb.ColumnID{0}},
},
},
otherDescs: []descpb.TableDescriptor{{
Name: "bar",
ID: 52,
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
PrimaryIndex: descpb.IndexDescriptor{
ID: 1,
Name: "primary",
KeyColumnIDs: []descpb.ColumnID{1},
KeyColumnNames: []string{"a"},
},
Columns: []descpb.ColumnDescriptor{
{Name: "a", ID: 1, Type: types.Int},
},
DependsOn: []descpb.ID{51},
}},
},
}

for i, test := range tests {
Expand Down

0 comments on commit 5f38e71

Please sign in to comment.