sql: add notice for interleaved tables #56874

postamar · 2020-11-18T23:28:14Z

This patch adds a deprecation notice when the user attempts to create an
interleaved table by executing a CREATE TABLE ... INTERLEAVE IN PARENT
statement.

Fixes #56579.

Release note (sql change): Added deprecation notice to CREATE TABLE ...
INTERLEAVE IN PARENT statements.

cockroach-teamcity · 2020-11-18T23:30:22Z

This change is

postamar · 2020-11-18T23:31:59Z

Did I do this right? At this point I'm wondering if it's possible to test against notices. I saw it show up when I ran cockroach demo and I'm not sure whether that's good enough:

[email protected]:26257/movr> CREATE TABLE public.barf ( id UUID NOT NULL, city VARCHAR NOT NULL, PRIMARY KEY (city, id) ) INTERLEAVE IN PARENT users (city, id);
NOTICE: interleaved tables are deprecated
CREATE TABLE

Time: 44ms total (execution 20ms / network 24ms)

ajwerner · 2020-11-18T23:49:48Z

I'd write a logic test. You can express NOTICE expectations in them with the noticetrace option. See

cockroach/pkg/sql/logictest/testdata/logic_test/notice

Line 2 in b03a7f8

query T noticetrace

. Probably this belongs in the interleaved logic test file.

jordanlewis

Nice!

You should add tests to a "logic test".

Logic tests are data driven tests that live in files in pkg/sql/logictest/testdata/logic_test. Grep that directory for noticetrace, and you'll see a bunch of tests that assert notices.

You can run an individual logic test file like this:

make testbaselogic FILES=yourfile

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @rafiss)

pkg/sql/create_table.go, line 225 at r1 (raw file):

		params.p.BufferClientNotice(
			params.ctx,
			pgnotice.Newf("interleaved tables are deprecated"),

I think we should say:

interleaved tables will be deprecated in 21.1, and removed in 21.2

And, in a hint (see errors.WithHintf), say:

see https://github.com/cockroachdb/cockroach/issues/52009 for more details.

It's possible that we want to bikeshed this wording a bit (cc @vy-ton)

jordanlewis

Oops, sorry for the double feedback from myself and Andrew, saying the same thing in both messages.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @rafiss)

jordanlewis

There are a few more places we need to add this notice to, as well.

ALTER PRIMARY KEY, which can make a table interleaved
CREATE INDEX, which can make an index interleaved

Example:

alter table a alter primary key using columns (a, b) interleave in parent(a)
create index foo on b(a) interleave in parent a(a)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @postamar and @rafiss)

postamar

(edit: sorry still getting familiar with Reviewable)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @rafiss, and @vy-ton)

pkg/sql/create_table.go, line 225 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think we should say:
interleaved tables will be deprecated in 21.1, and removed in 21.2
And, in a hint (see errors.WithHintf), say:
see https://github.com/cockroachdb/cockroach/issues/52009 for more details.
It's possible that we want to bikeshed this wording a bit (cc @vy-ton)

I took the liberty of rewording it to "interleaved tables and indexes will..." after discussion with @ajwerner

vy-ton

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rafiss)

pkg/sql/create_table.go, line 225 at r1 (raw file):

will be deprecated in 21.1

Are we backporting the notice to 20.2? If so, I would prefer interleaved tables and indexes are deprecated in 20.2 and will be removed in 21.2

postamar · 2020-11-20T00:28:42Z

Looks like I forgot some test cases in pkg/cli, that was careless of me. For some reason I was unable to run those locally with make test PKG=./pkg/cli, I keep getting seemingly unrelated version errors, is this expected?

ajwerner · 2020-11-20T00:30:47Z

What are the errors? One trick is to use the builder docker container: ./build/builder.sh make test .... That thing is handy.

postamar

They were errors which I wasn't able to reproduce after trying the docker build, the conclusion I'm taking from this is I need to better understand the build process, pronto. In any case I was able to run the tests locally today.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @rafiss)

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r2 (raw file):

			errors.WithHint(
				pgnotice.Newf("interleaved tables and indexes are deprecated in 20.2 and will be removed in 21.2"),
				"See https://github.com/cockroachdb/cockroach/issues/52009 for more details.",

Can you please use the facility unimplemented.MakeURL()
(you may want to move that function from package "unimplemented" to something with a more generic name)

As we want to have proper telemetry on URL usage

pkg/sql/create_index.go, line 400 at r2 (raw file):

			errors.WithHint(
				pgnotice.Newf("interleaved tables and indexes are deprecated in 20.2 and will be removed in 21.2"),
				"See https://github.com/cockroachdb/cockroach/issues/52009 for more details.",

ditto

pkg/sql/create_table.go, line 227 at r2 (raw file):

			errors.WithHint(
				pgnotice.Newf("interleaved tables and indexes are deprecated in 20.2 and will be removed in 21.2"),
				"See https://github.com/cockroachdb/cockroach/issues/52009 for more details.",

ditto

postamar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r2 (raw file):

Previously, knz (kena) wrote…

Can you please use the facility unimplemented.MakeURL()
(you may want to move that function from package "unimplemented" to something with a more generic name)

As we want to have proper telemetry on URL usage

I looked into this and found out about errors.WithIssueLink which seems the way to go. Anyway I noticed that MakeURL puts the version prefix in the url string and my understanding is this will break CI tests when building a real version: assertions on See: https://go.crdb.dev/issue-v/52009/dev (in pkg/sql) or See: https://go.crdb.dev/issue-v/52009/v999.0 (in pkg/cli) will fail.
Perhaps for the SQL logic tests I could expand statement to assert on notice instead of just ok or error, to do assertions on notices in a similar way to statement error which seems to cope well in this case (see https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/temp_table#L4 ) but I'm not sure how to do it right for CLI tests. I'm looking into it.

knz

Reviewed 5 of 5 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

I looked into this and found out about errors.WithIssueLink which seems the way to go. Anyway I noticed that MakeURL puts the version prefix in the url string and my understanding is this will break CI tests when building a real version: assertions on See: https://go.crdb.dev/issue-v/52009/dev (in pkg/sql) or See: https://go.crdb.dev/issue-v/52009/v999.0 (in pkg/cli) will fail.
Perhaps for the SQL logic tests I could expand statement to assert on notice instead of just ok or error, to do assertions on notices in a similar way to statement error which seems to cope well in this case (see https://github.com/cockroachdb/cockroach/blob/master/pkg/sql/logictest/testdata/logic_test/temp_table#L4 ) but I'm not sure how to do it right for CLI tests. I'm looking into it.

So yes you found that our automated logic tests are not able to deal with this properly. This is a limitation in the test framework, not an indication that you should do the implementation differently (my idiom for this is "don't let the tail wag the dog")

These special URLs generated by MakeURL achieve multiple things:

they tell our product team when people click the links.
they tell our product team which version the user is encountering the link in.
they enable us to redirect the URLs to something else than github if/when we migrate from github issues to something else.

Overall we're moving away from raw github URLs. That's what I'm pointing out here.

postamar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @knz, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r2 (raw file):

Previously, knz (kena) wrote…

So yes you found that our automated logic tests are not able to deal with this properly. This is a limitation in the test framework, not an indication that you should do the implementation differently (my idiom for this is "don't let the tail wag the dog")

These special URLs generated by MakeURL achieve multiple things:

they tell our product team when people click the links.

they tell our product team which version the user is encountering the link in.

they enable us to redirect the URLs to something else than github if/when we migrate from github issues to something else.

Overall we're moving away from raw github URLs. That's what I'm pointing out here.

Thanks for confirming and for explaining.

I published a new revision which extends the test framework in a way I'm hoping you'll all find reasonable and not too out of scope. This does place a higher burden on the reviewers though. FWIW I cursorily checked the new feature's correctness by falsifying the expected notice patterns and making sure the assertions correspondingly failed.

I found a workaround for the pkg/cli tests, checking for the notice there is redundant anyway.

This patch adds a deprecation notice on execution of a statement containing an INTERLEAVE IN PARENT clause. These are: - CREATE TABLE ... INTERLEAVE IN PARENT ... - ALTER TABLE ... ALTER PRIMARY KEY ... INTERLEAVE IN PARENT ... - CREATE INDEX ... INTERLEAVE IN PARENT ... Previously, we weren't able to test notices in sql logic tests by asserting against regexps, only against plain text using `query T noticetrace`. This made us unable to test deprecation notices featuring issue links, which include build version info. This commit addresses that by adding support for `statement notice <regexp>` in the logic test framework. Fixes cockroachdb#56579. Release note (sql change): Added deprecation notice to statements containing INTERLEAVE IN PARENT clause.

knz

very nice 💯

Reviewed 18 of 18 files at r3.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

vy-ton

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r3 (raw file):

			ctx,
			errors.WithIssueLink(
				pgnotice.Newf("interleaved tables and indexes are deprecated in 20.2 and will be removed in 21.2"),

@jordanlewis @lucy-zhang Are we ok with this deprecation being backported to 20.2?

thoszhang

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r3 (raw file):

Previously, vy-ton (Vy Ton) wrote…

@jordanlewis @lucy-zhang Are we ok with this deprecation being backported to 20.2?

We're documenting the feature as deprecated in 20.2, right? I think that's fine.

vy-ton

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @postamar, and @rafiss)

pkg/sql/alter_primary_key.go, line 38 at r3 (raw file):

Previously, lucy-zhang (Lucy Zhang) wrote…

We're documenting the feature as deprecated in 20.2, right? I think that's fine.

Yes I filed https://github.com/cockroachdb/docs/issues/8891to be done after this notice is added

jordanlewis

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @ajwerner, @jordanlewis, @postamar, and @rafiss)

postamar · 2020-11-23T20:12:11Z

bors r+

craig · 2020-11-23T22:01:21Z

Build succeeded:

GitHub CI (Cockroach)

postamar requested a review from rafiss November 18, 2020 23:28

jordanlewis requested changes Nov 18, 2020

View reviewed changes

jordanlewis reviewed Nov 18, 2020

View reviewed changes

jordanlewis reviewed Nov 19, 2020

View reviewed changes

postamar force-pushed the 56579 branch from b71d70a to 0643fab Compare November 19, 2020 22:25

postamar requested a review from a team as a code owner November 19, 2020 22:25

postamar commented Nov 19, 2020

View reviewed changes

vy-ton reviewed Nov 19, 2020

View reviewed changes

postamar force-pushed the 56579 branch from 0643fab to d5e6076 Compare November 20, 2020 14:02

postamar commented Nov 20, 2020

View reviewed changes

postamar force-pushed the 56579 branch from d5e6076 to 93c6957 Compare November 20, 2020 14:10

knz reviewed Nov 20, 2020

View reviewed changes

postamar commented Nov 20, 2020

View reviewed changes

knz reviewed Nov 20, 2020

View reviewed changes

postamar force-pushed the 56579 branch from 93c6957 to 0ca2647 Compare November 20, 2020 22:08

postamar commented Nov 20, 2020

View reviewed changes

postamar force-pushed the 56579 branch from 0ca2647 to 93a32ae Compare November 23, 2020 14:17

knz approved these changes Nov 23, 2020

View reviewed changes

vy-ton reviewed Nov 23, 2020

View reviewed changes

thoszhang reviewed Nov 23, 2020

View reviewed changes

vy-ton approved these changes Nov 23, 2020

View reviewed changes

postamar requested a review from jordanlewis November 23, 2020 19:02

jordanlewis approved these changes Nov 23, 2020

View reviewed changes

craig bot merged commit c27ae46 into cockroachdb:master Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add notice for interleaved tables #56874

sql: add notice for interleaved tables #56874

postamar commented Nov 18, 2020

cockroach-teamcity commented Nov 18, 2020

postamar commented Nov 18, 2020

ajwerner commented Nov 18, 2020

jordanlewis left a comment

jordanlewis left a comment

jordanlewis left a comment

postamar left a comment •

edited

Loading

vy-ton left a comment

postamar commented Nov 20, 2020 •

edited

Loading

ajwerner commented Nov 20, 2020

postamar left a comment

knz left a comment

postamar left a comment

knz left a comment

postamar left a comment

knz left a comment

vy-ton left a comment

thoszhang left a comment

vy-ton left a comment

jordanlewis left a comment

postamar commented Nov 23, 2020

craig bot commented Nov 23, 2020

sql: add notice for interleaved tables #56874

sql: add notice for interleaved tables #56874

Conversation

postamar commented Nov 18, 2020

cockroach-teamcity commented Nov 18, 2020

postamar commented Nov 18, 2020

ajwerner commented Nov 18, 2020

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

postamar left a comment • edited Loading

Choose a reason for hiding this comment

vy-ton left a comment

Choose a reason for hiding this comment

postamar commented Nov 20, 2020 • edited Loading

ajwerner commented Nov 20, 2020

postamar left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

postamar left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

vy-ton left a comment

Choose a reason for hiding this comment

thoszhang left a comment

Choose a reason for hiding this comment

vy-ton left a comment

Choose a reason for hiding this comment

jordanlewis left a comment

Choose a reason for hiding this comment

postamar commented Nov 23, 2020

craig bot commented Nov 23, 2020

postamar left a comment •

edited

Loading

postamar commented Nov 20, 2020 •

edited

Loading