Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
99533: sql,backupccl: add more tests for user ID migrations r=rafiss a=andyyang890

**sql: add mixed version test for system.privileges user ID migration**

This patch adds a mixed version logic test that ensures granting of
system privileges continues to work properly in a cluster that has
both 22.2 and 23.1 nodes. The relevant version gate being tested here
is V23_1SystemPrivilegesTableHasUserIDColumn.

Release note: None

---

**sql: add mixed version test for system.database_role_settings user IDs**

This patch adds a mixed version logic test that ensures setting default
session variables continues to work properly in a cluster that has
both 22.2 and 23.1 nodes. The relevant version gate being tested here
is V23_1DatabaseRoleSettingsHasRoleIDColumn.

Release note: None

---

**backupccl: update TestRestoreOldVersions subtest for system.privileges**

This patch updates the TestRestoreOldVersions subtest for the
system.privileges table to also test that a row for the public
role is correctly restored.

Release note: None

---

Part of #87079 
Part of #92342

99761: sql/pgwire: buffer messages during COPY TO and startup r=rafiss a=rafiss

fixes #97314
backport fixes #99546

---

### sql: buffer COPY OUT data 

Rather than sending each COPY result row one-by-one, now the data will
get buffered, then flushed when the buffer size limit is reached or when
sending ReadyForQuery.

This fixes an issue that was causing the ruby-pg test to hang, since it assumes
the data will be buffered.

---

### pgwire: buffer startup messages when creating connection

This avoids sending each ParameterStatus one-by-one.

---

### sql: refactor CopyIn handling

This makes it so we don't need to manually send a CommandComplete.
Instead, when the CopyInResult is closed, CommandComplete will be sent,
similar to how it works for other message types.

Release note: None

99953: sql/schemachanger: address bugs with column families   r=fqazi a=fqazi

This PR addresses the following bugs with column families:

1. On master and 23.1 after the removal of oprules, we have scenarios where not implemented assertions can be hit for column families when rollbacks occur. These changes add a more concrete assertion that just ensures that the column family is cleaned up, and rules to ensure appropriate sequencing for removal.
2. When UPDATE/INSERTs were executed concurrently while adding a new column family, we could end up writing to the old primary key with the new column family. In happy path cases where everything was successful, this didn't matter, but if a rollback occurred we would have values left in the old primary index that runtime couldn't handle.
3. We had no way of validating DML with concurrent schema changes in cases with rollbacks, these modifications add tests and the framework required this case.

Release note (bug fix): Concurrent DML while adding
a new column with a new column family can lead to
corruption in the existing primary index. If a rollback
occurs the table may no longer be accessible.

Co-authored-by: Andy Yang <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>
  • Loading branch information
4 people committed Mar 30, 2023
4 parents 331a672 + 3b3b573 + f7d1256 + 594ad87 commit 96e8593
Show file tree
Hide file tree
Showing 104 changed files with 9,187 additions and 302 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/backupccl/restore_old_versions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,6 +366,7 @@ func fullClusterRestoreSystemPrivilegesWithoutIDs(exportDir string) func(t *test
sqlDB.Exec(t, fmt.Sprintf("RESTORE FROM '%s' WITH UNSAFE_RESTORE_INCOMPATIBLE_VERSION", localFoo))

sqlDB.CheckQueryResults(t, "SELECT * FROM system.privileges", [][]string{
{"public", "/vtable/crdb_internal/tables", "{}", "{}", "4"},
{"testuser1", "/global/", "{VIEWACTIVITY}", "{}", "100"},
{"testuser2", "/global/", "{MODIFYCLUSTERSETTING}", "{}", "101"},
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,5 @@ CREATE USER testuser2;
GRANT SYSTEM VIEWACTIVITY TO testuser1;

GRANT SYSTEM MODIFYCLUSTERSETTING TO testuser2;

REVOKE SELECT ON crdb_internal.tables FROM public;
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -1 +1 @@
U�IS
����
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
B:�|
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
�w
Binary file not shown.

This file was deleted.

Binary file not shown.

This file was deleted.

10 changes: 10 additions & 0 deletions pkg/ccl/schemachangerccl/backup_base_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions pkg/ccl/schemachangerccl/backup_base_mixed_generated_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -97,6 +96,7 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── MarkDescriptorAsDropped {"DescriptorID":104}
│ ├── DrainDescriptorName {"Namespace":{"DatabaseID":104,"DescriptorID":105,"Name":"public"}}
│ ├── NotImplementedForPublicObjects {"DescID":105,"ElementType":"scpb.Owner"}
Expand Down Expand Up @@ -234,7 +234,6 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -257,6 +256,7 @@ Schema change plan for DROP DATABASE ‹multi_region_test_db› CASCADE;
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── RemoveColumnNotNull {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.TablePartit..."}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -62,7 +61,8 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── RemoveDroppedColumnType {"ColumnID":2,"TableID":108}
│ └── UpdateTableBackReferencesInTypes {"BackReferencedTableID":108}
│ ├── UpdateTableBackReferencesInTypes {"BackReferencedTableID":108}
│ └── AssertColumnFamilyIsRemoved {"TableID":108}
├── PreCommitPhase
│ ├── Stage 1 of 2 in PreCommitPhase
│ │ ├── 27 elements transitioning toward ABSENT
Expand Down Expand Up @@ -130,7 +130,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.TablePartit..."}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -155,6 +154,7 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── RemoveColumnNotNull {"ColumnID":2,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":2,"TableID":108}
│ ├── MakeDeleteOnlyColumnAbsent {"ColumnID":4294967295,"TableID":108}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -48,7 +47,8 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── DrainDescriptorName {"Namespace":{"DatabaseID":104,"DescriptorID":108,"Name":"table_regional_b...","SchemaID":105}}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ └── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ └── AssertColumnFamilyIsRemoved {"TableID":108}
├── PreCommitPhase
│ ├── Stage 1 of 2 in PreCommitPhase
│ │ ├── 20 elements transitioning toward ABSENT
Expand Down Expand Up @@ -101,7 +101,6 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── MarkDescriptorAsDropped {"DescriptorID":108}
│ ├── RemoveObjectParent {"ObjectID":108,"ParentSchemaID":105}
│ ├── RemoveBackReferenceInTypes {"BackReferencedDescriptorID":108}
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.ColumnFamil..."}
│ ├── MakePublicColumnWriteOnly {"ColumnID":1,"TableID":108}
│ ├── SetColumnName {"ColumnID":1,"Name":"crdb_internal_co...","TableID":108}
│ ├── MakePublicColumnNotNullValidated {"ColumnID":1,"TableID":108}
Expand All @@ -115,6 +114,7 @@ Schema change plan for DROP TABLE ‹multi_region_test_db›.‹public›.‹tab
│ ├── NotImplementedForPublicObjects {"DescID":108,"ElementType":"scpb.Owner"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"admin"}
│ ├── RemoveUserPrivileges {"DescriptorID":108,"User":"root"}
│ ├── AssertColumnFamilyIsRemoved {"TableID":108}
│ ├── RemoveColumnNotNull {"ColumnID":1,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967295,"TableID":108}
│ ├── MakeWriteOnlyColumnDeleteOnly {"ColumnID":4294967294,"TableID":108}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,17 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → WRITE_ONLY
Expand Down Expand Up @@ -407,10 +416,6 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ TypeIDs:
│ │ - 106
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -503,6 +508,9 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ DescriptorID: 108
│ │ User: root
│ │
│ ├── • AssertColumnFamilyIsRemoved
│ │ TableID: 108
│ │
│ ├── • MarkDescriptorAsDropped
│ │ DescriptorID: 104
│ │
Expand Down Expand Up @@ -997,8 +1005,17 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ ├── • ColumnFamily:{DescID: 108, Name: primary, ColumnFamilyID: 0}
│ │ │ │ PUBLIC → ABSENT
│ │ │ │
│ │ │ └── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ ├── • Precedence dependency from DROPPED Table:{DescID: 108}
│ │ │ │ rule: "descriptor dropped before dependent element removal"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 1}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ ├── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967295}
│ │ │ │ rule: "column type removed before column family"
│ │ │ │
│ │ │ └── • Precedence dependency from ABSENT ColumnType:{DescID: 108, ColumnFamilyID: 0, ColumnID: 4294967294}
│ │ │ rule: "column type removed before column family"
│ │ │
│ │ ├── • Column:{DescID: 108, ColumnID: 1}
│ │ │ │ PUBLIC → ABSENT
Expand Down Expand Up @@ -1181,10 +1198,6 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ TypeIDs:
│ │ - 106
│ │
│ ├── • NotImplementedForPublicObjects
│ │ DescID: 108
│ │ ElementType: scpb.ColumnFamily
│ │
│ ├── • MakePublicColumnWriteOnly
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down Expand Up @@ -1277,6 +1290,9 @@ EXPLAIN (ddl, verbose) DROP DATABASE multi_region_test_db CASCADE;
│ │ DescriptorID: 108
│ │ User: root
│ │
│ ├── • AssertColumnFamilyIsRemoved
│ │ TableID: 108
│ │
│ ├── • RemoveColumnNotNull
│ │ ColumnID: 1
│ │ TableID: 108
Expand Down
Loading

0 comments on commit 96e8593

Please sign in to comment.