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.

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

workload/schemachange: screen for errors in operations #56184

Conversation

jayshrivastava
Copy link
Contributor

@jayshrivastava jayshrivastava commented Oct 31, 2020

workload/schemachange: screen for errors in operations

These changes add error screening for the following (#56119):

  • createEnum op
  • createSchema op
  • dropColumn op
  • dropColumnDefault op
  • dropColumnNotNull op
  • dropSchema op
  • renameColumn op
  • renameTable op
  • renameView op

Also, errors caused by schema changes after writes in the same txn are handled (#56230)

It also resolves a bug related to ambiguity in column names that was caused by views/tables referencing the same columns (#56235).

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@jayshrivastava jayshrivastava changed the title Screen for errors in schemachange workload workload/schemachange: screen for errors in operations Oct 31, 2020
@jayshrivastava jayshrivastava force-pushed the screen-for-errors-in-schemachange-workload branch from 23b6a30 to 2f1f2cc Compare October 31, 2020 21:49
@jayshrivastava jayshrivastava marked this pull request as ready for review November 2, 2020 14:37
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just made it through 2 commits

Reviewed 2 of 2 files at r1, 2 of 2 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @jayshrivastava)


pkg/workload/schemachange/error_screening.go, line 90 at r1 (raw file):

Quoted 13 lines of code…
	return scanBool(tx, `SELECT EXISTS (
		SELECT fd.descriptor_name
		  FROM crdb_internal.forward_dependencies AS fd
		 WHERE fd.descriptor_id =
				(
				SELECT c.oid
				  FROM pg_catalog.pg_class AS c
					JOIN pg_catalog.pg_namespace AS ns
            ON ns.oid = c.relnamespace
         WHERE c.relname = $1
           AND ns.nspname = $2
				)
	)`, tableName.Object(), tableName.Schema())

nit: sqlfum.pt

SELECT EXISTS(
        SELECT fd.descriptor_name
          FROM crdb_internal.forward_dependencies AS fd
         WHERE fd.descriptor_id
               = (
                    SELECT c.oid
                      FROM pg_catalog.pg_class AS c
                      JOIN pg_catalog.pg_namespace AS ns ON
                            ns.oid = c.relnamespace
                     WHERE c.relname = $1 AND ns.nspname = $2
                )
       );

pkg/workload/schemachange/error_screening.go, line 95 at r2 (raw file):

}

func columnIsDependedOn(tx *pgx.Tx, tableName *tree.TableName, columnName string) (bool, error) {

commentary on how this works would be helpful. Namely explain the structure of the data you are picking apart.

Also, it feels like you could do this is sql with only modestly more clunkiness.

Also, this doesn't seem to work on foreign key references :(


pkg/workload/schemachange/error_screening.go, line 154 at r2 (raw file):

func colIsPrimaryKey(tx *pgx.Tx, tableName *tree.TableName, columnName string) (bool, error) {
	return scanBool(tx, `SELECT EXISTS(

nit: sqlfumpt

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, made it through all of it. Mostly just would like to see a bit more commentary where stuff is getting big. Otherwise :lgtm:

Reviewed 1 of 1 files at r11.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jayshrivastava)


pkg/workload/schemachange/error_screening.go, line 95 at r2 (raw file):

Previously, ajwerner wrote…

commentary on how this works would be helpful. Namely explain the structure of the data you are picking apart.

Also, it feels like you could do this is sql with only modestly more clunkiness.

Also, this doesn't seem to work on foreign key references :(

yeah I'm worried you may also need a join on pg_constraint, depending on what you're intending to use this for eventually. Comment that this is about dependencies which prevent drops and not foreign keys.


pkg/workload/schemachange/operation_generator.go, line 567 at r11 (raw file):

	}

	uniqueColumnNames := map[string]bool{}

commentary here is starting to be in order.

@jayshrivastava jayshrivastava force-pushed the screen-for-errors-in-schemachange-workload branch 2 times, most recently from b312e93 to 410a96e Compare November 5, 2020 14:50
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 2 files at r13, 1 of 1 files at r22.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @jayshrivastava)

Duplicate and/or ambiguous columns can exist in createView and createTableAs
statements because tables and views can now be generated from others which may
have columns in common. This commit adds error handling for duplicate columns and also
qualifies column names in createTableAs or createView statements to remove ambiguity.

Release note: None
@jayshrivastava jayshrivastava force-pushed the screen-for-errors-in-schemachange-workload branch from 410a96e to 6ebfb07 Compare November 12, 2020 22:02
@jayshrivastava
Copy link
Contributor Author

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Nov 13, 2020

Build failed:

@jayshrivastava
Copy link
Contributor Author

bors retry

@craig
Copy link
Contributor

craig bot commented Nov 15, 2020

Build succeeded:

@craig craig bot merged commit 117d32e into cockroachdb:master Nov 15, 2020
@jayshrivastava jayshrivastava deleted the screen-for-errors-in-schemachange-workload branch November 15, 2020 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants