Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

sql/schemachanger: Resolve timing hole in with primary key schema changes in schemachange/random-load workload #66663

Closed
ajstorm opened this issue Jun 21, 2021 · 2 comments
Closed

sql/schemachanger: Resolve timing hole in with primary key schema changes in schemachange/random-load workload #66663

ajstorm opened this issue Jun 21, 2021 · 2 comments
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@ajstorm
Copy link
Collaborator

ajstorm commented Jun 21, 2021 •
edited by cockroach-jira-scripts
Loading

There are certain schema change operations (referred to as sc-disallowed in the remainder of this description) which are not permitted in the presence of existing alter primary key schema changes (e.g. making a column nullable, making a column not nullable). The way we test this in schemachange/random-load is by querying the jobs table before making the sc-disallowed call and determining if there's another PK change in progress. If there is a PK change in progress, then we add the expected error to the list of errors to look out for when executing sc-disallowed.

The problem however, is that there seems to be a timing hole here. If we detect a PK change in progress which then completes before sc-disallowed is executed, sc-disallowed will succeed and the test will fail (as the expected error is not encountered). This is blocking progress on cleaning up schemachanger/random-load so I've disabled these known cases for now. They should be reenabled with this issue.

Jira issue: CRDB-8169
@ajstorm ajstorm added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. A-schema-changes T-sql-schema-deprecated Use T-sql-foundations instead labels Jun 21, 2021
ajstorm added a commit to ajstorm/cockroach that referenced this issue Jun 21, 2021
@ajstorm
sql: First round of cleanup of schemachange/random-load
fe31478 
This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:

- Makes several errors pgcodes, so that they can be properly added to
  the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
  times in an index definition.
- Allows for column type changes, which must have the experimental flag
  enable_experimental_alter_column_type_general flag set.

It also disables the testing of setColumnType (tracked with cockroachdb#66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with cockroachdb#66663).

Release note: None.
@ajstorm ajstorm mentioned this issue Jun 21, 2021
Merged
ajstorm added a commit to ajstorm/cockroach that referenced this issue Jun 22, 2021
@ajstorm
sql: First round of cleanup of schemachange/random-load
e1393ea 
This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:

- Makes several errors pgcodes, so that they can be properly added to
  the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
  times in an index definition.
- Allows for column type changes, which must have the experimental flag
  enable_experimental_alter_column_type_general flag set.

It also disables the testing of setColumnType (tracked with cockroachdb#66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with cockroachdb#66663).

Release note: None.
ajstorm added a commit to ajstorm/cockroach that referenced this issue Jun 22, 2021
@ajstorm
sql: First round of cleanup of schemachange/random-load
46baa75 
This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:

- Makes several errors pgcodes, so that they can be properly added to
  the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
  times in an index definition.
- Allows for column type changes, which must have the experimental flag
  enable_experimental_alter_column_type_general flag set.

It also disables the testing of setColumnType (tracked with cockroachdb#66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with cockroachdb#66663).

Release note: None.
craig bot pushed a commit that referenced this issue Jun 22, 2021
@ajstorm
Merge #66599 #66664
2cafdfb 
66599: sql: enable adding columns which are not written to the current primary index r=postamar a=postamar

    row,schemachanger: use StoreColumnIDs/Names in primary index
    
    In a recent commit, the StoreColumnIDs and StoreColumnNames slices in
    primary indexes were populated when previously they had simply been
    empty. We simply assumed that all non-virtual columns in a table would
    be stored in the primary index: primary key columns in the key, the rest
    in the value.
    
    This commit breaks that assumption by using the StoreColumnIDs slice to
    determine what goes into the primary index. This makes it possible for
    the new schema changer to add columns safely, preventing unwanted writes
    to the old primary index while the schema change is underway.
    
    Fixes #59149.
    
    Release note: None


    sql,tabledesc: add new IndexDescriptorVersion for primary indexes
    
    Previously, the IndexDescriptorVersion type was only used to describe
    the encoding of secondary indexes. This commit adds a new value for
    use in primary indexes, PrimaryIndexWithStoredColumnsVersion, to signify
    that the StoredColumnIDs and StoredColumnNames slices are populated
    correctly.
    
    Previously, these slices did not need to be populated at all. This is
    because the set of columns comprising the primary index of a table is
    assumed to be all non-virtual columns of that table. Our upcoming work on
    the new schema changer will require us to violate that assumption
    however. This commit is in preparation of that change.
    
    In our effort to make meaningful the concept of stored columns in
    primary indexes, this commit also changes the contents of the
    information_schema.statistics table. As a result, SHOW INDEXES and SHOW
    COLUMNS behave the same way regardless of whether an index is primary or
    secondary.
    
    Release note (sql change): The contents of the statistics table in the
    information schema have changed, therefore so have the results of SHOW
    INDEX and SHOW COLUMNS. A column which is not in the primary key will
    now be listed as belonging to the primary index as a stored column.
    Previously, it was simply not listed as belonging to the primary index.

66664: sql: First round of cleanup of schemachange/random-load r=ajwerner,otan a=ajstorm

This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:

- Makes several errors pgcodes, so that they can be properly added to
  the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
  times in an index definition.
- Allows for column type changes, which must have the experimental flag
  enable_experimental_alter_column_type_general flag set.

It also disables the testing of setColumnType (tracked with #66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with #66663).

Release note: None.

Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Adam Storm <[email protected]>
ajstorm added a commit to ajstorm/cockroach that referenced this issue Jul 14, 2021
@ajstorm
sql: First round of cleanup of schemachange/random-load
4137aed 
This issue addresses several issues uncovered in running the randomized
schema changer. Specifically:

- Makes several errors pgcodes, so that they can be properly added to
  the expected errors list in the randomized schema changer.
- Detects cases where the region column (crdb_region) is used multiple
  times in an index definition.
- Allows for column type changes, which must have the experimental flag
  enable_experimental_alter_column_type_general flag set.

It also disables the testing of setColumnType (tracked with cockroachdb#66662) as
well as making a column nullable/non-nullable due to a timing hole
(tracked with cockroachdb#66663).

Release note: None.
@ajwerner
Copy link
Contributor

@fqazi I have to imagine that we've figured this out by now. Can this be closed?

@fqazi
Copy link
Collaborator

fqazi commented Aug 16, 2022

Yes this one has been fixed.

@fqazi fqazi closed this as completed Aug 16, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-schema-changes C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fqazi @ajwerner @ajstorm