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

sql: Introduce fast path delete for cascading interleaved tables #28330

Merged
merged 1 commit into from Aug 15, 2018

Conversation

Projects
None yet
5 participants
@emsal1863
Copy link
Contributor

commented Aug 7, 2018

This patch introduces fast deletes for interleaved tables with ON DELETE
CASCADE. Deleting from a table with other tables interleaved in it will
now simply call KV DelRanges instead of going through all of the
foreign key checks and cascades for the interleaved tables.

This fast path is not activated when:

  • The table or any of its interleaved tables have any secondary indices
  • The table or any of its interleaved tables are referenced by any other
    table outside of them by foreign key
  • Any of the interleaved relationships are not ON DELETE CASCADE

Release note: None

closes #27990

@emsal1863 emsal1863 requested review from jordanlewis and BramGruneir Aug 7, 2018

@emsal1863 emsal1863 requested review from cockroachdb/sql-execution-prs as code owners Aug 7, 2018

@cockroach-teamcity

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

This change is Reviewable

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 350 at r1 (raw file):

}

func canDeleteFastInterleaved(table TableDescriptor, fkTables sqlbase.TableLookupsByID) bool {

I'm not actually entirely sure how I'm going to test this as I'm not too sure how to deal with table descriptors from the tests.


pkg/sql/delete.go, line 489 at r1 (raw file):

		for i, span := range scan.spans {
			if span.EndKey.Compare(span.EndKey.PrefixEnd()) == -1 {

I feel like using span.EndKey.PrefixEnd() is sort of a hack and if there's a better way to get to the end of the interleaved keys I'd happily take it.

Using KV tracing I found that the PrefixEnd here returns something like /Table/<TableA>/<index>/<value>/NULL (I'd like to clarify what that means actually).

When you do a DelRange from the beginning of the span to this value it does delete all of the interleaved keys, because all of them will have values like /Table/<TableA>/<index>/value/#/<TableB>/..., but I don't really know if this is really the most secure way we could be doing this.

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch from 1d87a71 to bdf3e10 Aug 7, 2018

@andreimatei

This comment has been minimized.

Copy link
Member

commented Aug 7, 2018

No release note? Seems like a good win.

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch from bdf3e10 to 6630b3b Aug 7, 2018

@spencerkimball

This comment has been minimized.

Copy link
Member

commented Aug 8, 2018

It’s probably best not to use DelRange but instead issue individual deletes. This avoids pressure on the write timestamp cache if there are many deletes, which will cause txns to retry unnecessarily. If the individual deletes are in a batch should be as fast.

See the changes to rowwriter.go in #23258

@rmloveland rmloveland referenced this pull request Aug 8, 2018

Closed

Document SQL fast paths #3511

@BramGruneir
Copy link
Member

left a comment

Reviewed 1 of 2 files at r3, 4 of 4 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 350 at r1 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

I'm not actually entirely sure how I'm going to test this as I'm not too sure how to deal with table descriptors from the tests.

Unit test is the only way I think of.


pkg/sql/delete.go, line 489 at r1 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

I feel like using span.EndKey.PrefixEnd() is sort of a hack and if there's a better way to get to the end of the interleaved keys I'd happily take it.

Using KV tracing I found that the PrefixEnd here returns something like /Table/<TableA>/<index>/<value>/NULL (I'd like to clarify what that means actually).

When you do a DelRange from the beginning of the span to this value it does delete all of the interleaved keys, because all of them will have values like /Table/<TableA>/<index>/value/#/<TableB>/..., but I don't really know if this is really the most secure way we could be doing this.

I don't understand your question here. What's the concern with using prefixend?

Also, what is this compare for?


pkg/sql/delete.go, line 352 at r5 (raw file):

func canDeleteFastInterleaved(table TableDescriptor, fkTables sqlbase.TableLookupsByID) bool {

	// If there are no interleaved tables then don't take this path.

Why not? This whole check seems unneeded to me. And you don't even use the totalInterleaved value anywhere else. So if you do choose to keep it, just turn it into a bool and quickly exist if one is found.


pkg/sql/delete.go, line 358 at r5 (raw file):

	}
	if totalInterleaved == 0 {
		log.Warning(context.TODO(), "Base table has no interleaved tables")

leftover debugging code, a bunch of it


pkg/sql/delete.go, line 364 at r5 (raw file):

	// contains all table IDs that are directly or indirectly interleaved into `table`,
	// including `table` itself
	interleavedMemo := make(map[sqlbase.ID]struct{})

I don't understand the need for the memo here. You can't have circular interleaved tables.


pkg/sql/delete.go, line 366 at r5 (raw file):

	interleavedMemo := make(map[sqlbase.ID]struct{})
	interleavedQueue := make([]sqlbase.ID, 0)
	interleavedQueue = append(interleavedQueue, table.ID)

I think you can just create the queue with the single item directly.


pkg/sql/delete.go, line 369 at r5 (raw file):

	// if the base table is interleaved in another table, fail
	// TODO let this succeed if this is part of another table's fast path (???).

TODOs are fine to leave in the code, but use our standard formatting

TODO(emmanuel): consider letting this succeed if this is part of another table's fast path.

That being said, I don't understand this one.


pkg/sql/delete.go, line 396 at r5 (raw file):

			// interleavedIdxs will contain all of the table and index IDs of the indexes interleaved in this one
			interleavedIdxs := make(map[sqlbase.ID]map[sqlbase.IndexID]struct{})

Why do you need this map? Just push the table id of the interleaved table on the queue. Am I missing some other check? Also, put this at the end, so the other checks can fail earlier before you go adding to the queue.


pkg/sql/delete_test.go, line 74 at r5 (raw file):

	drop := func(tables ...string) {
		// dropping has to be done in reverse so no drop causes a foreign key violation

Or just drop with cascade I think.


pkg/sql/delete_test.go, line 82 at r5 (raw file):

	}

	// This function is to be called at the beginning of each sub-benchmark to set up the necessary tables.

This is not a benchmark.


pkg/sql/delete_test.go, line 150 at r5 (raw file):

	t.Run("canDeleteFastInterleaved, two children", testCheck(true, "parent", "child", "sibling", "grandchild"))
	t.Run("canDeleteFastInterleaved, two children", testCheck(true, "parent", "child", "grandchild"))
	//t.Run("canDeleteFastInterleaved, two children", testCheck(false, "parent", "childNoCascade")) // turning this on returns a "panic: missing table" triggered by trying to get the table descriptor for childNoCascade.

Ick. Why is that?


pkg/sql/tablewriter_delete.go, line 100 at r5 (raw file):

	ctx context.Context, scan *scanNode, autoCommit autoCommitOpt, traceKV bool,
) (rowCount int, err error) {
	log.Info(ctx, "doing fastDelete")

remove this debug line


pkg/sql/logictest/testdata/logic_test/interleaved, line 386 at r5 (raw file):

----
1

please add some tests to see if the fastpath is used for tracing, then you can test the edge cases where it can't be used and ensure it isn't.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 396 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why do you need this map? Just push the table id of the interleaved table on the queue. Am I missing some other check? Also, put this at the end, so the other checks can fail earlier before you go adding to the queue.

Below there's a check to determine if any of the foreign key references referencing the current index are different from the interleaved foreign key reference. Essentially it checks idx.ReferencedBy against idx.InterleavedBy to see if ReferencedBy contains any external references.

I don't see how it's possible to check these external references without this map, or another map of some sort (maybe I only need to store the table IDs instead of table and index ID?). I do agree on the point about pushing to the queue only at the end.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 358 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

leftover debugging code, a bunch of it

Done.


pkg/sql/delete.go, line 364 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I don't understand the need for the memo here. You can't have circular interleaved tables.

Done.


pkg/sql/delete.go, line 366 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I think you can just create the queue with the single item directly.

Done.


pkg/sql/delete.go, line 369 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

TODOs are fine to leave in the code, but use our standard formatting

TODO(emmanuel): consider letting this succeed if this is part of another table's fast path.

That being said, I don't understand this one.

Done (for the other one, removed this one; will elaborate this one later)


pkg/sql/tablewriter_delete.go, line 100 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

remove this debug line

Done.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 489 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

I don't understand your question here. What's the concern with using prefixend?

Also, what is this compare for?

Done.

also we've ironed out the details for the possible problem and unless we change our KV encoding for interleaved tables, using PrefixEnd should be fine.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete_test.go, line 74 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Or just drop with cascade I think.

Done.


pkg/sql/delete_test.go, line 82 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This is not a benchmark.

Done.


pkg/sql/delete_test.go, line 150 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Ick. Why is that?

It doesn't look like the table is getting dropped so I have a feeling this might be some sort of bug with GetTableDescriptor in testutils.

@BramGruneir
Copy link
Member

left a comment

Reviewed 1 of 1 files at r8, 1 of 1 files at r9, 1 of 1 files at r10.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 369 at r5 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

Done (for the other one, removed this one; will elaborate this one later)

ok


pkg/sql/delete.go, line 396 at r5 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

Below there's a check to determine if any of the foreign key references referencing the current index are different from the interleaved foreign key reference. Essentially it checks idx.ReferencedBy against idx.InterleavedBy to see if ReferencedBy contains any external references.

I don't see how it's possible to check these external references without this map, or another map of some sort (maybe I only need to store the table IDs instead of table and index ID?). I do agree on the point about pushing to the queue only at the end.

Why not forward check instead of back checking? Get the indexes and fks of the interleaved tables before adding them to the queue.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 350 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Unit test is the only way I think of.

Done (unit test work still going)


pkg/sql/delete.go, line 352 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why not? This whole check seems unneeded to me. And you don't even use the totalInterleaved value anywhere else. So if you do choose to keep it, just turn it into a bool and quickly exist if one is found.

Done


pkg/sql/delete.go, line 369 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

ok

Removed this as well. Perhaps it's best to add TODOs when we have more discussion about using the interleaved fast path in more cases.


pkg/sql/delete.go, line 396 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Why not forward check instead of back checking? Get the indexes and fks of the interleaved tables before adding them to the queue.

Done

We had a discussion about this earlier offline and this issue appears to be resolved

  • back checking is actually necessary to check for external references that aren't interleaved
  • easiest simplification here seems to be checking if the lengths of InterleavedBy and ReferencedBy are the same but that doesn't check to make sure all of the indexes match

pkg/sql/logictest/testdata/logic_test/interleaved, line 386 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

please add some tests to see if the fastpath is used for tracing, then you can test the edge cases where it can't be used and ensure it isn't.

Done

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch 2 times, most recently from cd3896b to 4dd8a46 Aug 13, 2018

@emsal1863

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

All CR comments have been addressed; @spencerkimball the second/third commit in this PR contains the implementation with Del instead of DelRange. Here are the performance numbers for each:

fast path:

BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved-8             2000000000             0.08 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_sibling-8     1000000000             0.23 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_grandchild-8             1000000000             0.22 ns/op

fast path with scan + del instead of delrange:

BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved-8             2000000000             0.19 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_sibling-8       100000         10123 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_grandchild-8                 2000        511899 ns/op

So there is a sizable slowdown when switching away from DelRange.

Here's the numbers from master:

BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved-8         	      20	  50800162 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_sibling-8 	       1 1557171104 ns/op
BenchmarkMultiRowFKChecks/deleteRowsFromParent_interleaved_grandchild-8         	       1 2397660316 ns/op

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch from 7457bce to ee61a0a Aug 14, 2018

@BramGruneir
Copy link
Member

left a comment

Save a branch with the point delete commit in it, and then remove the extra commit form this branch. (just post a link to the other branch).

After that, I'll give it one more pass, but I think this is just about ready to go.

Reviewed 2 of 3 files at r11, 2 of 3 files at r12, 3 of 3 files at r13.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 476 at r13 (raw file):

	}

	log.Info(ctx, "delete: fast path is available")

Leftover debugging log.


pkg/sql/delete_test.go, line 150 at r5 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

It doesn't look like the table is getting dropped so I have a feeling this might be some sort of bug with GetTableDescriptor in testutils.

See if you can recreate this in a test in a new branch. Make it as minimal as possible then we can post an issue for it as a bug.


pkg/sql/tablewriter_delete.go, line 94 at r12 (raw file):

}

func (td *tableDeleter) fastDeleteInterleaved(

This could obviously be cleaned up, but since it's being cut, I'll leave it alone.

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch 2 times, most recently from 9b0b801 to a213b4d Aug 14, 2018

@emsal1863
Copy link
Contributor Author

left a comment

Save a branch with the point delete commit in it, and then remove the extra commit form this branch. (just post a link to the other branch).

https://github.com/emsal1863/cockroach/tree/interleaved-fast-path-deletes-point

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 476 at r13 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Leftover debugging log.

Done


pkg/sql/delete_test.go, line 150 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

See if you can recreate this in a test in a new branch. Make it as minimal as possible then we can post an issue for it as a bug.

Will do

@BramGruneir
Copy link
Member

left a comment

Almost there!

Reviewed 3 of 3 files at r14.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 352 at r14 (raw file):

func canDeleteFastInterleaved(table TableDescriptor, fkTables sqlbase.TableLookupsByID) bool {

	// If there are no interleaved tables then don't take the fast path.

Please add a comment here about why we don't want the fast path if there are no interleaved tables. This would be to avoid overusing range_del.


pkg/sql/delete.go, line 364 at r14 (raw file):

	}

	// contains all table IDs that are directly or indirectly interleaved into `table`,

This comment is wrong.


pkg/sql/delete.go, line 366 at r14 (raw file):

	// contains all table IDs that are directly or indirectly interleaved into `table`,
	// including `table` itself
	interleavedQueue := []sqlbase.ID{table.ID}

This should be defined right before the for statement that uses it.


pkg/sql/delete.go, line 371 at r14 (raw file):

	for _, idx := range fkTables[table.ID].Table.AllNonDropIndexes() {
		if len(idx.Interleave.Ancestors) > 0 {
			log.Warning(context.TODO(), "Base table is interleaved in another one")

This shouldn't be a warning, but a trace statement perhaps? Or just remove it entirely.


pkg/sql/delete.go, line 395 at r14 (raw file):

			interleavedIdxs := make(map[sqlbase.ID]map[sqlbase.IndexID]struct{})
			for _, ref := range idx.InterleavedBy {

nit: extra empty line.


pkg/sql/delete.go, line 401 at r14 (raw file):

				interleavedIdxs[ref.Table][ref.Index] = struct{}{}

				// Append the next tables to inspect to the queue

this comment is in the wrong place and doesn't tell you more than what the code actual does.


pkg/sql/delete.go, line 426 at r14 (raw file):

				interleavedQueue = append(interleavedQueue, ref.Table)
			}

extra line


pkg/sql/delete_test.go, line 150 at r5 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

Will do

Then reference that new issue here. Or remove the testcase entirely, just make sure you have coverage.

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained


pkg/sql/delete.go, line 352 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Please add a comment here about why we don't want the fast path if there are no interleaved tables. This would be to avoid overusing range_del.

Done - updated comment

To be clear, DelRange is used for the regular fast path as well. The real reason for why we fail the fast path in this case is that there are several cases when, in the absence of any interleaved tables, the span modification done by the fast path will actually result in errant behaviour.


pkg/sql/delete.go, line 364 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This comment is wrong.

Done


pkg/sql/delete.go, line 366 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This should be defined right before the for statement that uses it.

Done


pkg/sql/delete.go, line 371 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This shouldn't be a warning, but a trace statement perhaps? Or just remove it entirely.

Done - removed


pkg/sql/delete.go, line 395 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

nit: extra empty line.

Done


pkg/sql/delete.go, line 401 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

this comment is in the wrong place and doesn't tell you more than what the code actual does.

Done


pkg/sql/delete.go, line 426 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

extra line

Done


pkg/sql/delete_test.go, line 150 at r5 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Then reference that new issue here. Or remove the testcase entirely, just make sure you have coverage.

Done - removed test case. Sending this issue will be its own task.

@BramGruneir
Copy link
Member

left a comment

:lgtm:

After you squash obviously. But I'm still curious about why it fails if there are no interleaved tables. Are you missing a test or check?

Reviewed 2 of 2 files at r15.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/delete.go, line 352 at r14 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

Done - updated comment

To be clear, DelRange is used for the regular fast path as well. The real reason for why we fail the fast path in this case is that there are several cases when, in the absence of any interleaved tables, the span modification done by the fast path will actually result in errant behaviour.

Can you specify what errant behaviour? What happens? What's missing? What's different?

@BramGruneir
Copy link
Member

left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/sql/delete.go, line 350 at r1 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

Done (unit test work still going)

This test work is done right?

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/sql/delete.go, line 350 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

This test work is done right?

Yes

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


pkg/sql/delete.go, line 352 at r14 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Can you specify what errant behaviour? What happens? What's missing? What's different?

There are a couple of cases where taking this fast path when there aren't any interleaved tables will cause the spans to be modified in such a way that an unintended number of rows will be deleted. Maybe some more investigation needs to be done in order to determine why this is the case, but it appears the way that PrefixEnd interacts with the EndKeys of the spans that come about when scanning tables without any other tables interleaved in them.

Here's an example of one such logic test failure that happens if this check is not here:

(statement prior to failure)
query II
SELECT * FROM [DELETE FROM a WHERE b IN (6,7) RETURNING a,b] LIMIT 0
----

testdata/logic_test/statement_source:67: SELECT * FROM a ORDER BY b
expected:
    100  2
    -2   3
    7    8
but found (query options: "") :
    100  2
    -2   3
    3    4
    -4   5
    5    6
    -6   7
    7    8

Another involving SHOW TRACE:

testdata/logic_test/show_trace:298: SELECT operation, message FROM [SHOW KV TRACE FOR SESSION]
expected:
    sql txn           Scan /Table/55/{1-2}
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 Scan to (n1,s1):1
    sql txn           DelRange /Table/55/1 - /Table/55/2
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 DelRng, 1 BeginTxn, 1 EndTxn to (n1,s1):1
    sql txn           fast path completed
    sql txn           rows affected: 2

but found (query options: "") :
    sql txn           Scan /Table/55/{1-2}
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 Scan to (n1,s1):1
    sql txn           DelRange /Table/55/1 - /Table/55/3
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 DelRng, 1 BeginTxn, 1 EndTxn to (n1,s1):1
    sql txn           fast path completed
    sql txn           rows affected: 2
@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/delete.go, line 352 at r14 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

There are a couple of cases where taking this fast path when there aren't any interleaved tables will cause the spans to be modified in such a way that an unintended number of rows will be deleted. Maybe some more investigation needs to be done in order to determine why this is the case, but it appears the way that PrefixEnd interacts with the EndKeys of the spans that come about when scanning tables without any other tables interleaved in them.

Here's an example of one such logic test failure that happens if this check is not here:

(statement prior to failure)
query II
SELECT * FROM [DELETE FROM a WHERE b IN (6,7) RETURNING a,b] LIMIT 0
----

testdata/logic_test/statement_source:67: SELECT * FROM a ORDER BY b
expected:
    100  2
    -2   3
    7    8
but found (query options: "") :
    100  2
    -2   3
    3    4
    -4   5
    5    6
    -6   7
    7    8

Another involving SHOW TRACE:

testdata/logic_test/show_trace:298: SELECT operation, message FROM [SHOW KV TRACE FOR SESSION]
expected:
    sql txn           Scan /Table/55/{1-2}
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 Scan to (n1,s1):1
    sql txn           DelRange /Table/55/1 - /Table/55/2
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 DelRng, 1 BeginTxn, 1 EndTxn to (n1,s1):1
    sql txn           fast path completed
    sql txn           rows affected: 2

but found (query options: "") :
    sql txn           Scan /Table/55/{1-2}
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 Scan to (n1,s1):1
    sql txn           DelRange /Table/55/1 - /Table/55/3
    dist sender send  querying next range at /Table/55/1
    dist sender send  r1: sending batch 1 DelRng, 1 BeginTxn, 1 EndTxn to (n1,s1):1
    sql txn           fast path completed
    sql txn           rows affected: 2

It turns out:

  • The first issue here is to do with another bug involving the RETURNING CLAUSE, which has been dealt with
  • The second failure here is still a valid concern and is still only avoided by including this interleaved check
@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/delete.go, line 352 at r14 (raw file):

Previously, emsal1863 (Emmanuel Sales) wrote…

It turns out:

  • The first issue here is to do with another bug involving the RETURNING CLAUSE, which has been dealt with
  • The second failure here is still a valid concern and is still only avoided by including this interleaved check

Done - It turns out that the SHOW TRACE error didn't really affect the number of rows deleted at all, and that discrepancy in the EndKey is a discrepancy in the Index ID rather than any row's primary key (so it's not deleting any extra rows!)

Updated the comment with the now-correct reasoning

@BramGruneir
Copy link
Member

left a comment

:lgtm:

Woo, squash and merge!

Reviewed 1 of 2 files at r16, 1 of 1 files at r17.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/delete.go, line 182 at r17 (raw file):

	// fastPathInterleaved indicates whether the delete operation can run
	// the interleaved fast path (all interleaved tables have no indexes and ON DELETE CASCADE)

nit: add a period


pkg/sql/logictest/testdata/logic_test/interleaved, line 369 at r17 (raw file):


subtest interleaved_join_on_other_columns
subtest InterleavedDeleteFastPath

You have a blank subtest here. Just delete the top one.


pkg/sql/logictest/testdata/logic_test/interleaved, line 743 at r17 (raw file):

output row: [9]
output row: [10]
rows affected: 10

Please drop the tables used in the subtest

@emsal1863
Copy link
Contributor Author

left a comment

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


pkg/sql/delete.go, line 182 at r17 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

nit: add a period

Done


pkg/sql/logictest/testdata/logic_test/interleaved, line 369 at r17 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

You have a blank subtest here. Just delete the top one.

Done - Turns out there were logic tests here that were removed (whoops) -- re-added them


pkg/sql/logictest/testdata/logic_test/interleaved, line 743 at r17 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Please drop the tables used in the subtest

Done

sql: Introduce fast path delete for cascading interleaved tables
This patch introduces fast deletes for interleaved tables with ON DELETE
CASCADE. Deleting from a table with other tables interleaved in it will
now simply call KV DelRanges instead of going through all of the
foreign key checks and cascades for the interleaved tables.

This fast path is *not* activated when:
* The table or any of its interleaved tables have any secondary indices
* The table or any of its interleaved tables are referenced by any other
table outside of them by foreign key
* Any of the interleaved relationships are not ON DELETE CASCADE

Release note: Performance is greatly improved when deleting from a table
with interleaved tables that have ON DELETE CASCADE clauses.

@emsal1863 emsal1863 force-pushed the emsal1863:interleaved-fast-path-delete branch from 6ee2586 to 70d551f Aug 15, 2018

@emsal1863

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2018

bors r+

craig bot pushed a commit that referenced this pull request Aug 15, 2018

Merge #28330 #28632 #28638 #28648
28330: sql: Introduce fast path delete for cascading interleaved tables r=emsal1863 a=emsal1863

This patch introduces fast deletes for interleaved tables with ON DELETE
CASCADE. Deleting from a table with other tables interleaved in it will
now simply call KV DelRanges instead of going through all of the
foreign key checks and cascades for the interleaved tables.

This fast path is *not* activated when:
* The table or any of its interleaved tables have any secondary indices
* The table or any of its interleaved tables are referenced by any other
table outside of them by foreign key
* Any of the interleaved relationships are not ON DELETE CASCADE

Release note: None

closes #27990 

28632: storage: require RHS catch-up during merge r=bdarnell a=benesch

Committing a merge before all replicas of the right-hand side are aware
of the merge is dangerous. Once the merge commits, the other two
replicas of the RHS can be GC'd; if a majority of the replicas are GC'd
before the down replica comes back online, it will have no peers to tell
it that the merge occurred.

To avoid this situation, use the new WaitForApplication long-poll RPC to
wait for all replicas of the RHS to catch up before committing the
merge.

This also paves the way for more efficient merges, as the RHS is
guaranteed to be complete up-to-date when the merge transaction commits.
A future patch will avoid sending a snapshot of the RHS in the merge
trigger.


28638: opt: audit TODOs and clean up easy ones r=justinj a=justinj

There's two commits here:

* the first just deletes some TODOs that weren't relevant anymore,
* the second modifies the optbuilder for groupby to only pass through ordering when there's an aggregate that needs it.

28648: imporccl: support ON DELETE CASCADE in MYSQLDUMP import r=dt a=dt

This adds support for the ON DELETE and ON UPDATE referential actions in MYSQLDUMP schema parsing.

Release note: none.

Co-authored-by: Emmanuel <absolutezero2a03@gmail.com>
Co-authored-by: Nikhil Benesch <nikhil.benesch@gmail.com>
Co-authored-by: Justin Jaffray <justin@cockroachlabs.com>
Co-authored-by: David Taylor <tinystatemachine@gmail.com>
@craig

This comment has been minimized.

Copy link

commented Aug 15, 2018

Build succeeded

@craig craig bot merged commit 70d551f into cockroachdb:master Aug 15, 2018

3 checks passed

GitHub CI (Cockroach) TeamCity build finished
Details
bors Build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@knz knz added this to Triage in (DEPRECATED) SQL Front-end, Lang & Semantics via automation Aug 16, 2018

@knz knz moved this from Triage to Finished (milestone 0731) in (DEPRECATED) SQL Front-end, Lang & Semantics Aug 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.