sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version #39173

jordanlewis · 2019-07-30T16:18:56Z

This PR adds the new FK representation to the descriptor protobufs, adds a new cluster version for the FK representation upgrade and introduces methods that upgrade and downgrade descriptors between the two representations.

See individual commits for details.

cockroach-teamcity · 2019-07-30T16:19:05Z

This change is

jordanlewis · 2019-07-30T18:17:17Z

Note: I am going to add unit tests for upgrade/downgrade idempotency next.

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 856 at r4 (raw file):

		}

		// XXX: Lucy, don't we want to *avoid* changing other table descriptors here?

Unless I'm missing something here, this doesn't change the other table descriptors; it puts the FK we just created on the appropriate index on the same table descriptor.

pkg/sql/sqlbase/structured.proto, line 49 at r4 (raw file):

Extra newline?

jordanlewis

I added tests.

One question - @lucy-zhang do you think we should nix the new OriginIndex and ReferencedIndex fields now that we have the UpgradedFromOriginReference and UpgradedFromReferencedReference fields? :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 856 at r4 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Unless I'm missing something here, this doesn't change the other table descriptors; it puts the FK we just created on the appropriate index on the same table descriptor.

Resolved offline.

pkg/sql/sqlbase/structured.proto, line 49 at r4 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Extra newline?

Done.

thoszhang

Would we just get the index IDs from the other struct? That seems fine.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 265 at r8 (raw file):

// ProtoGetter is a sub-interface of client.Txn that can fetch protobufs in a
// transaction.
type ProtoGetter interface {

Are there plans to start using this interface elsewhere, besides what we need to test right now? I think something like this is necessary but I'm concerned about introducing disorder if we don't use it in a principled way everywhere.

pkg/sql/sqlbase/structured.go, line 856 at r8 (raw file):

		return false, desc, nil
	}
	// TODO (lucy): add check that all fields are actually populated

I'm not sure we actually need this now. If we get into a situation where this FK is not populated (and I can't think of a plausible bug/mistake that would cause this), the later validation will (indirectly) catch it. Maybe I'm forgetting why we originally wanted to do this.

pkg/sql/sqlbase/structured_test.go, line 1369 at r8 (raw file):

// This test exercises the foreign key representation upgrade and downgrade
// methods that were introduced in 19.2 to move foriegn key descriptors from

foreign

pkg/sql/sqlbase/structured_test.go, line 1424 at r8 (raw file):

							OriginColumnIDs:     ColumnIDs{1},
							ReferencedColumnIDs: ColumnIDs{2},
							ReferencedTableID:   2,

nit: is there a reason to have the fields in this order? I would have expected ReferencedColumnIDs and ReferencedTableID to go in the opposite order.

pkg/sql/sqlbase/structured_test.go, line 1710 at r8 (raw file):

				// The upgraded proto will also have a copy of the old foreign key
				// reference attached for each foreign key. Delete that for the purposes of
				// verifying equality.

Is there any downside to just keeping these extra fields (aside from the fact that it makes the test structs even more annoying and huge)?

yuzefovich

This stuff seems quite tricky :)

Reviewed 1 of 1 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, 5 of 5 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, 2 of 2 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jordanlewis and @lucy-zhang)

pkg/sql/sqlbase/structured.go, line 812 at r3 (raw file):

// maybeDowngradeForeignKeyRepresentation non-destructively downgrades the
// receiver into the old foreign key representation (the ForeignKey
// and ReferencedBy fields on IndexDescriptor if and only if the cluster version

[nit]: missing closing parenthesis.

Release note: None

The new descriptor format for FKs lives on TableDescriptor, not IndexDescriptor, and doesn't pin the foreign keys to any particular index on the origin or referenced tables. This paves the way toward lifting the constraint that all outgoing foreign keys must have an origin index, and that foreign keys may not overlap on their reference columns. Release note: None

Release note: None

jordanlewis

Would we just get the index IDs from the other struct? That seems fine.

Nevermind, this is not possible because the reference doesn't encode both origin and referenced index id. I left that alone for simplicity's sake.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lucy-zhang and @yuzefovich)

pkg/sql/sqlbase/structured.go, line 812 at r3 (raw file):

Previously, yuzefovich wrote…

[nit]: missing closing parenthesis.

Done.

pkg/sql/sqlbase/structured.go, line 265 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Are there plans to start using this interface elsewhere, besides what we need to test right now? I think something like this is necessary but I'm concerned about introducing disorder if we don't use it in a principled way everywhere.

No, I don't have any plans to do that. I agree that this is sketchy. I asked about it in #core and nobody had anything particularly good to say, so unless you really object, I'll leave this.

pkg/sql/sqlbase/structured.go, line 856 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

I'm not sure we actually need this now. If we get into a situation where this FK is not populated (and I can't think of a plausible bug/mistake that would cause this), the later validation will (indirectly) catch it. Maybe I'm forgetting why we originally wanted to do this.

Done. I agree that it didn't seem necessary.

pkg/sql/sqlbase/structured_test.go, line 1369 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

foreign

Done.

pkg/sql/sqlbase/structured_test.go, line 1424 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

nit: is there a reason to have the fields in this order? I would have expected ReferencedColumnIDs and ReferencedTableID to go in the opposite order.

Nope, just happenstance / carelessness - fixed everywhere.

pkg/sql/sqlbase/structured_test.go, line 1710 at r8 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Is there any downside to just keeping these extra fields (aside from the fact that it makes the test structs even more annoying and huge)?

Just the latter. It doesn't seem particularly important to me - I think messing this up would mess up the verification too.

dt

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @jordanlewis, @lucy-zhang, and @yuzefovich)

pkg/sql/sqlbase/structured.go, line 265 at r8 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

No, I don't have any plans to do that. I agree that this is sketchy. I asked about it in #core and nobody had anything particularly good to say, so unless you really object, I'll leave this.

any reason for it to be exported?

pkg/sql/sqlbase/structured.proto, line 92 at r12 (raw file):

  // is in a mixed-version state containing VersionTopLevelForeignKeys) was done
  // without creating any accidental changes to the foreign key references.
  optional uint32 origin_index = 10 [(gogoproto.nullable) = false, (gogoproto.casttype) = "IndexID", deprecated = true];

I might name these legacy_ since otherwise OriginIndex looks like a perfect valid thing to use. AFAIK the deprecated annotation does nothing, right?

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @lucy-zhang, and @yuzefovich)

pkg/sql/sqlbase/structured.go, line 265 at r8 (raw file):

Previously, dt (David Taylor) wrote…

any reason for it to be exported?

Nope. Done.

pkg/sql/sqlbase/structured.proto, line 92 at r12 (raw file):

Previously, dt (David Taylor) wrote…

I might name these legacy_ since otherwise OriginIndex looks like a perfect valid thing to use. AFAIK the deprecated annotation does nothing, right?

The deprecated annotation makes the field crossed out in my editor.

thoszhang

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @jordanlewis, @lucy-zhang, and @yuzefovich)

pkg/sql/sqlbase/structured.go, line 265 at r8 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Nope. Done.

It's fine with me.

pkg/sql/sqlbase/structured.proto, line 92 at r12 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

The deprecated annotation makes the field crossed out in my editor.

Yeah, I think prefixing with legacy_ couldn't hurt.

pkg/sql/sqlbase/structured_test.go, line 1710 at r8 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

Just the latter. It doesn't seem particularly important to me - I think messing this up would mess up the verification too.

Seems fine.

This commit adds two methods that upgrade and downgrade TableDescriptors to and from the new, top-level FK representation. Release note: None

jordanlewis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt, @jordanlewis, @lucy-zhang, and @yuzefovich)

pkg/sql/sqlbase/structured.proto, line 92 at r12 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

Yeah, I think prefixing with legacy_ couldn't hurt.

Done.

jordanlewis · 2019-08-03T02:41:25Z

TFTRs!

bors r+

39173: sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version r=jordanlewis a=jordanlewis This PR adds the new FK representation to the descriptor protobufs, adds a new cluster version for the FK representation upgrade and introduces methods that upgrade and downgrade descriptors between the two representations. See individual commits for details. Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>

craig · 2019-08-03T03:10:04Z

Build succeeded

GitHub CI (Cockroach)

jordanlewis requested review from thoszhang and a team July 30, 2019 16:18

jordanlewis force-pushed the fk-upgrade-version branch from 0ebb4f2 to f38ccda Compare July 30, 2019 16:54

jordanlewis changed the title ~~cluster: new version for the FK repr upgrade~~ sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version Jul 30, 2019

jordanlewis force-pushed the fk-upgrade-version branch from f38ccda to 9048621 Compare July 30, 2019 17:06

thoszhang reviewed Jul 30, 2019

View reviewed changes

jordanlewis force-pushed the fk-upgrade-version branch 2 times, most recently from 8ac146a to e5cbe72 Compare August 1, 2019 01:29

jordanlewis commented Aug 1, 2019

View reviewed changes

thoszhang reviewed Aug 1, 2019

View reviewed changes

yuzefovich reviewed Aug 1, 2019

View reviewed changes

jordanlewis added 3 commits August 1, 2019 10:23

cluster: new version for the FK repr upgrade

979094a

Release note: None

sqlbase: introduce ProtoGetter interface for testing

1e9eae7

Release note: None

jordanlewis force-pushed the fk-upgrade-version branch from e5cbe72 to c151ba4 Compare August 1, 2019 14:29

jordanlewis commented Aug 1, 2019

View reviewed changes

jordanlewis requested a review from dt August 1, 2019 18:25

jordanlewis force-pushed the fk-upgrade-version branch from c151ba4 to 6941061 Compare August 1, 2019 18:26

dt reviewed Aug 2, 2019

View reviewed changes

jordanlewis commented Aug 2, 2019

View reviewed changes

jordanlewis force-pushed the fk-upgrade-version branch from 6941061 to 0f906e8 Compare August 2, 2019 19:35

thoszhang approved these changes Aug 2, 2019

View reviewed changes

sqlbase: {up,down}grade methods for FK migration

e366076

This commit adds two methods that upgrade and downgrade TableDescriptors to and from the new, top-level FK representation. Release note: None

jordanlewis force-pushed the fk-upgrade-version branch from 0f906e8 to e366076 Compare August 2, 2019 23:20

jordanlewis commented Aug 2, 2019

View reviewed changes

craig bot merged commit e366076 into cockroachdb:master Aug 3, 2019

jordanlewis deleted the fk-upgrade-version branch August 6, 2019 16:21

knz mentioned this pull request Nov 10, 2019

User-facing changes in 19.2 that were not picked up in release notes cockroachdb/docs#5819

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version #39173

sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version #39173

jordanlewis commented Jul 30, 2019 •

edited

Loading

cockroach-teamcity commented Jul 30, 2019

jordanlewis commented Jul 30, 2019

thoszhang left a comment

jordanlewis left a comment

thoszhang left a comment

yuzefovich left a comment

jordanlewis left a comment

dt left a comment

jordanlewis left a comment

thoszhang left a comment

jordanlewis left a comment

jordanlewis commented Aug 3, 2019

craig bot commented Aug 3, 2019

sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version #39173

sqlbase,cluster: add new FK representation, upgrade/downgrade methods, and a new cluster version #39173

Conversation

jordanlewis commented Jul 30, 2019 • edited Loading

cockroach-teamcity commented Jul 30, 2019

jordanlewis commented Jul 30, 2019

thoszhang left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

dt left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis commented Aug 3, 2019

craig bot commented Aug 3, 2019

Build succeeded

jordanlewis commented Jul 30, 2019 •

edited

Loading