sql: add drop type to the prepared statement generator #61394

tharun208 · 2021-03-03T08:15:17Z

Release justification: fixes for high-priority or high-severity bugs in existing functionality

Previously, prepare statement doesn't have support for drop type
in the optimizer, so it panics during execution as no flags are generated.

This patch fixes this by adding dropType to the optimizer.

Resolves #61226

Release note: none

Signed-off-by: Tharun rajendrantharun@live.com

blathers-crl · 2021-03-03T08:15:21Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

@rafiss (author of sql: PREPARE q AS DROP TYPE typ causes an internal error #61226)

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

cockroach-teamcity · 2021-03-03T08:15:26Z

This change is

tharun208 · 2021-03-03T13:02:47Z

@rafiss, can you help by re-triggering the ci ?. thanks in advance.

rafiss

thanks for this fix!

this needs a test. can you add one in pkg/sql/logictest/testdata/logic_test/drop_type

the change looks fine, but since it's optimizer-related and we're in stability period, @RaduBerinde can you please sign off?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @tharun208)

pkg/sql/plan_opt.go, line 64 at r1 (raw file):

		*tree.CreateStats,
		*tree.Deallocate, *tree.Discard, *tree.DropDatabase, *tree.DropIndex,
		*tree.DropTable, *tree.DropView, *tree.DropSequence,

could you add DropType here, closer to the other drop statements?

tharun208 · 2021-03-03T16:17:53Z

pkg/sql/logictest/testdata/logic_test/drop_type

@rafiss, I think the test should need to be a part of prepare statement

blathers-crl · 2021-03-03T16:41:29Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

RaduBerinde

Can you also add DROP TYPE to TestStatementReuses ? This tests that all those statements work with PREPARE and EXPLAIN.

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

tharun208 · 2021-03-03T20:18:43Z

Can you also add DROP TYPE to TestStatementReuses ? This tests that all those statements work with PREPARE and EXPLAIN.

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

@RaduBerinde done with the changes. To pass the TestStatementReuses , I need to add Explain to the plan_opt. But , now existing test for select explain is failing

RaduBerinde

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

pkg/sql/plan_opt.go, line 65 at r2 (raw file):

		*tree.Deallocate, *tree.Discard, *tree.DropDatabase, *tree.DropIndex,
		*tree.DropTable, *tree.DropView, *tree.DropSequence, *tree.DropType,
		*tree.Execute, *tree.Explain,

Adding Explain here is not correct, Explain has result columns and is supported by the optimizer.

I assume you added it because some PREPARE EXPLAIN case was failing?

tharun208 · 2021-03-03T21:18:57Z

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

pkg/sql/plan_opt.go, line 65 at r2 (raw file):
		*tree.Deallocate, *tree.Discard, *tree.DropDatabase, *tree.DropIndex,
		*tree.DropTable, *tree.DropView, *tree.DropSequence, *tree.DropType,
		*tree.Execute, *tree.Explain,
Adding Explain here is not correct, Explain has result columns and is supported by the optimizer.

I assume you added it because some PREPARE EXPLAIN case was failing?

yes, just now checked EXPLAIN will return the result columns and yes, I added *tree.Explain because, when I tried adding DROP TYPE to TestStatementReuses , PREPARE p AS EXPLAIN of my test DROP TYPE type_name failed

RaduBerinde · 2021-03-03T21:22:23Z

pkg/sql/plan_opt.go, line 65 at r2 (raw file):

Previously, tharun208 (Tharun Rajendran) wrote…

yes, just now checked EXPLAIN will return the result columns and yes, I added *tree.Explain because, when I tried adding DROP TYPE to TestStatementReuses , PREPARE p AS EXPLAIN of my test DROP TYPE type_name failed

What was the failure?

tharun208 · 2021-03-03T21:26:20Z

pkg/sql/plan_opt.go, line 65 at r2 (raw file):

Previously, tharun208 (Tharun Rajendran) wrote…
What was the failure?

The same stack trace error we got for DROP TYPE on prepared statement , no Annotations found.

RaduBerinde · 2021-03-03T21:33:18Z

I think the right fix here is to move the part that uses .Ann() in startExec instead of DropType, similar to how the drop_table code does it.

tharun208 · 2021-03-03T21:51:58Z

I think the right fix here is to move the part that uses .Ann() in startExec instead of DropType, similar to how the drop_table code does it.

yes, I am able to achieve it, without adding dropType to the optimizer.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @tharun208)

pkg/sql/drop_type.go, line 33 at r3 (raw file):

type typeToDrop struct {
	desc   *typedesc.Mutable
	fqName string

i believe fqName should be removed now

pkg/sql/drop_type.go, line 111 at r3 (raw file):

		// Record these descriptors for deletion.
		node.td[typeDesc.ID] = typeToDrop{
			desc: typeDesc,

instead of removing fqName here, does it work with:

		fqName, err := getTypeNameFromTypeDescriptor(
			oneAtATimeSchemaResolver{ctx, p},
			typeDesc,
		)
		if err != nil {
			return nil, err
		}
		node.td[typeDesc.ID] = typeToDrop{
			desc:   typeDesc,
			fqName: fqName.FQString(),
		}

pkg/sql/drop_type.go, line 151 at r3 (raw file):

func (n *dropTypeNode) startExec(params runParams) error {
	for _, toDrop := range n.td {
		typ, fqName := toDrop.desc, toDrop.fqName

and now this can be changed to

fqName := tree.AsStringWithFQNames(n.n, params.Ann())

pkg/sql/drop_type.go, line 152 at r3 (raw file):

	for _, toDrop := range n.td {
		typ, fqName := toDrop.desc, toDrop.fqName
		if err := params.p.dropTypeImpl(params.ctx, typ, tree.AsStringWithFQNames(n.n, params.Ann()), true /* queueJob */); err != nil {

use fqName as the 3rd argument here

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @tharun208)

pkg/sql/drop_type.go, line 33 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

i believe fqName should be removed now

sorry, i didn't mean to publish this comment.

pkg/sql/drop_type.go, line 151 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

and now this can be changed to
fqName := tree.AsStringWithFQNames(n.n, params.Ann())

i didn't mean to publish this comment

RaduBerinde

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @tharun208)

pkg/sql/drop_type.go, line 112 at r3 (raw file):

		node.td[typeDesc.ID] = typeToDrop{
			desc:   typeDesc,
			fqName: tree.AsStringWithFQNames(name, p.Ann()),

I don't get it, now fqName is not set in half the cases, but we are using it in startExec.

Why don't we remove the field altogether and just do getTypeNameFromTypeDescriptor() in all cases instartExec and use FQString()

RaduBerinde · 2021-03-03T22:32:29Z

pkg/sql/drop_type.go, line 152 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

use fqName as the 3rd argument here

Noete that the third argument is the entire statement, not just the type name. I would move it out of the loop and assign to an aptly named variable, it's silly to reformat for each type.

tharun208 · 2021-03-04T10:30:06Z

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @tharun208)

pkg/sql/drop_type.go, line 112 at r3 (raw file):
		node.td[typeDesc.ID] = typeToDrop{
			desc:   typeDesc,
			fqName: tree.AsStringWithFQNames(name, p.Ann()),
I don't get it, now fqName is not set in half the cases, but we are using it in startExec.

Why don't we remove the field altogether and just do getTypeNameFromTypeDescriptor() in all cases instartExec and use FQString()

this will change the TypeName in the event log. Current changes will not alter the event log` and all necessary cases got passed.

RaduBerinde

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

pkg/sql/drop_type.go, line 112 at r3 (raw file):

Previously, tharun208 (Tharun Rajendran) wrote…

this will change the TypeName in the event log. Current changes will not alter the event log` and all necessary cases got passed.

I don't understand.. what I'm suggesting should be equivalent to the latest revision, we would just call getTypeNameFromTypeDescriptor from startExec instead. Here's a diff (it passes the sql package tests and the drop_type logictest):

https://gist.github.com/RaduBerinde/f78a2b2c17f22cc429734b366f53edc5

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde and @tharun208)

pkg/sql/drop_type.go, line 152 at r3 (raw file):

Previously, RaduBerinde wrote…

Noete that the third argument is the entire statement, not just the type name. I would move it out of the loop and assign to an aptly named variable, it's silly to reformat for each type.

oh good call. hmm well also this parameter is the job description, which is used in the db console as a display name. so perhaps it would be better for them to have different names anyway.

maybe change to

params.p.dropTypeImpl(params.ctx, typ, "dropping type " + fqName, true /* queueJob */)

RaduBerinde · 2021-03-04T17:24:15Z

pkg/sql/drop_type.go, line 152 at r3 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

oh good call. hmm well also this parameter is the job description, which is used in the db console as a display name. so perhaps it would be better for them to have different names anyway.

maybe change to
params.p.dropTypeImpl(params.ctx, typ, "dropping type " + fqName, true /* queueJob */)

Yeah that's probably better

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde, @rafiss, and @tharun208)

pkg/sql/drop_type.go, line 135 at r4 (raw file):

	statementStr := tree.AsStringWithFQNames(n.n, params.Ann())
	for _, typeDesc := range n.toDrop {
		err := params.p.dropTypeImpl(params.ctx, typeDesc, "dropping type "+statementStr, true /* queueJob */)

please change "dropping type "+statementStr to dropping type "+typeFQName

and remove the statementStr variable entirely

tharun208 · 2021-03-04T19:52:08Z

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @RaduBerinde, @rafiss, and @tharun208)

pkg/sql/drop_type.go, line 135 at r4 (raw file):
	statementStr := tree.AsStringWithFQNames(n.n, params.Ann())
	for _, typeDesc := range n.toDrop {
		err := params.p.dropTypeImpl(params.ctx, typeDesc, "dropping type "+statementStr, true /* queueJob */)
please change "dropping type "+statementStr to dropping type "+typeFQName

and remove the statementStr variable entirely

so, this dropping type "+typeFQName will be changed to dropping type "+typeFQName.FQString(). since both are of different types.

Release justification: fixes for high-priority or high-severity bugs in existing functionality Previously, prepare statement doesnt have support for drop type in optimizer, so it panic during execution as no flags are generated. This patch fixes this by adding dropType to the optimizer. Resolves cockroachdb#61226 Release note: none Signed-off-by: Tharun <rajendrantharun@live.com>

rafiss

thank you! this was a nice cleanup

rafiss · 2021-03-04T21:26:51Z

bors r+

RaduBerinde

Thanks!

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

craig · 2021-03-04T22:22:23Z

Build succeeded:

GitHub CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Mar 3, 2021

blathers-crl bot requested a review from rafiss March 3, 2021 08:15

rafiss requested changes Mar 3, 2021

View reviewed changes

rafiss requested a review from RaduBerinde March 3, 2021 16:10

tharun208 force-pushed the sql/fix_prepare_drop_type branch from 1c0ec3b to b8394e7 Compare March 3, 2021 16:41

tharun208 force-pushed the sql/fix_prepare_drop_type branch 2 times, most recently from fb9f20f to bd61249 Compare March 3, 2021 18:18

tharun208 requested a review from rafiss March 3, 2021 18:19

tharun208 force-pushed the sql/fix_prepare_drop_type branch 2 times, most recently from e87836a to 726ca2b Compare March 3, 2021 19:12

RaduBerinde approved these changes Mar 3, 2021

View reviewed changes

tharun208 force-pushed the sql/fix_prepare_drop_type branch from 726ca2b to 8a3cdce Compare March 3, 2021 20:17

RaduBerinde reviewed Mar 3, 2021

View reviewed changes

tharun208 force-pushed the sql/fix_prepare_drop_type branch from 8a3cdce to aa022d0 Compare March 3, 2021 21:51

tharun208 force-pushed the sql/fix_prepare_drop_type branch from aa022d0 to fa553b7 Compare March 3, 2021 22:06

rafiss requested changes Mar 3, 2021

View reviewed changes

rafiss reviewed Mar 3, 2021

View reviewed changes

tharun208 requested a review from RaduBerinde March 3, 2021 22:19

RaduBerinde reviewed Mar 3, 2021

View reviewed changes

tharun208 force-pushed the sql/fix_prepare_drop_type branch from fa553b7 to 3432a33 Compare March 4, 2021 05:59

tharun208 marked this pull request as draft March 4, 2021 06:35

tharun208 marked this pull request as ready for review March 4, 2021 07:46

tharun208 force-pushed the sql/fix_prepare_drop_type branch from 3432a33 to daff007 Compare March 4, 2021 09:36

tharun208 requested review from rafiss and RaduBerinde March 4, 2021 10:26

tharun208 force-pushed the sql/fix_prepare_drop_type branch from daff007 to 82230d8 Compare March 4, 2021 10:31

RaduBerinde reviewed Mar 4, 2021

View reviewed changes

rafiss reviewed Mar 4, 2021

View reviewed changes

tharun208 force-pushed the sql/fix_prepare_drop_type branch from 82230d8 to ad4fbad Compare March 4, 2021 19:20

tharun208 requested review from rafiss and RaduBerinde March 4, 2021 19:20

rafiss requested changes Mar 4, 2021

View reviewed changes

tharun208 force-pushed the sql/fix_prepare_drop_type branch from ad4fbad to 556ea20 Compare March 4, 2021 20:03

tharun208 requested a review from rafiss March 4, 2021 20:03

rafiss approved these changes Mar 4, 2021

View reviewed changes

RaduBerinde reviewed Mar 4, 2021

View reviewed changes

craig bot merged commit 69ed26b into cockroachdb:master Mar 4, 2021

tharun208 deleted the sql/fix_prepare_drop_type branch March 4, 2021 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add drop type to the prepared statement generator #61394

sql: add drop type to the prepared statement generator #61394

tharun208 commented Mar 3, 2021 •

edited

blathers-crl bot commented Mar 3, 2021

cockroach-teamcity commented Mar 3, 2021

tharun208 commented Mar 3, 2021

rafiss left a comment

tharun208 commented Mar 3, 2021

blathers-crl bot commented Mar 3, 2021

RaduBerinde left a comment

tharun208 commented Mar 3, 2021 •

edited

RaduBerinde left a comment

tharun208 commented Mar 3, 2021

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 3, 2021

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 3, 2021

rafiss left a comment

rafiss left a comment

RaduBerinde left a comment

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 4, 2021

RaduBerinde left a comment

rafiss left a comment

RaduBerinde commented Mar 4, 2021

rafiss left a comment

tharun208 commented Mar 4, 2021

rafiss left a comment

rafiss commented Mar 4, 2021

RaduBerinde left a comment

craig bot commented Mar 4, 2021

sql: add drop type to the prepared statement generator #61394

sql: add drop type to the prepared statement generator #61394

Conversation

tharun208 commented Mar 3, 2021 • edited

blathers-crl bot commented Mar 3, 2021

cockroach-teamcity commented Mar 3, 2021

tharun208 commented Mar 3, 2021

rafiss left a comment

Choose a reason for hiding this comment

tharun208 commented Mar 3, 2021

blathers-crl bot commented Mar 3, 2021

RaduBerinde left a comment

Choose a reason for hiding this comment

tharun208 commented Mar 3, 2021 • edited

RaduBerinde left a comment

Choose a reason for hiding this comment

tharun208 commented Mar 3, 2021

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 3, 2021

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 3, 2021

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

RaduBerinde left a comment

Choose a reason for hiding this comment

RaduBerinde commented Mar 3, 2021

tharun208 commented Mar 4, 2021

RaduBerinde left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

RaduBerinde commented Mar 4, 2021

rafiss left a comment

Choose a reason for hiding this comment

tharun208 commented Mar 4, 2021

rafiss left a comment

Choose a reason for hiding this comment

rafiss commented Mar 4, 2021

RaduBerinde left a comment

Choose a reason for hiding this comment

craig bot commented Mar 4, 2021

tharun208 commented Mar 3, 2021 •

edited

tharun208 commented Mar 3, 2021 •

edited