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.

Sign up for GitHub

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

Jump to bottom

sql: add support for executing postqueries #38281

Merged

craig merged 1 commit into cockroachdb:master from yuzefovich:fk

Jun 24, 2019

Member

yuzefovich commented Jun 18, 2019

This commit introduces infrastructure for running "deferred
subqueries" that are to be executed after the execution of the main
query tree which is needed for (among other things) for foreign key
checks.

Release note: None

yuzefovich requested review from jordanlewis, RaduBerinde and a team

June 18, 2019 21:38

Member

cockroach-teamcity commented Jun 18, 2019

This change is

yuzefovich commented

View reviewed changes

Member Author

yuzefovich left a comment

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

pkg/sql/distsql_running.go, line 925 at r1 (raw file):

		return deferredSubqueryRecv.commErr
	}
	return deferredSubqueryRowReceiver.Err()

Not sure if this is needed (whether this should be return nil): I'm a little confused by the comment above DistSQLReceiver.commErr.

pkg/sql/plan.go, line 330 at r1 (raw file):

// deferredSubquery is a query tree that is executed after the main one. It can
// only return an error (for example, foreign key violation).
type deferredSubquery struct {

Not sure what the best name for this is. Suggestions are welcome.

RaduBerinde approved these changes

View reviewed changes

Member

RaduBerinde left a comment

Thanks, it will be exciting to wire this up to my prototype!

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

jordanlewis reviewed

View reviewed changes

Member

jordanlewis left a comment

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @jordanlewis, @RaduBerinde, and @yuzefovich)

pkg/sql/distsql_running.go, line 909 at r1 (raw file):

	deferredSubqueryPlanCtx.stmtType = tree.Rows
	// Don't close the top-level plan from deferred subqueries - someone else
	// will handle that.

I think in this case we actually do need to close all the top level plans. The comment about this in the other code path is because subqueries are also going to be closed via the main planNode tree. In our cases, we have the only reference to the postqueries.

pkg/sql/distsql_running.go, line 939 at r1 (raw file):

func (r *deferredSubqueryRowResultWriter) AddRow(ctx context.Context, row tree.Datums) error {
	// TODO(yuzefovich): should we panic here since this should never be called?

How about returning an internal error?

pkg/sql/distsql_running.go, line 944 at r1 (raw file):

func (r *deferredSubqueryRowResultWriter) IncrementRowsAffected(n int) {
	// TODO(yuzefovich): should we panic here since this should never be called?

I think this one could be called one day once we implement cascade.

pkg/sql/plan.go, line 302 at r1 (raw file):

	// deferredSubqueryPlans contains all the plans for subqueries that are to be
	// executed after the main query (for example, foreign keys checks).

nit: s/keys/key/

pkg/sql/plan.go, line 330 at r1 (raw file):

Previously, yuzefovich wrote…

Not sure what the best name for this is. Suggestions are welcome.

What about postquery?

yuzefovich force-pushed the fk branch from 95ed012 to cc8f605 Compare

June 19, 2019 16:55

yuzefovich changed the title ~~sql: add support for executing deferred subquries~~ sql: add support for executing postqueries

yuzefovich commented

View reviewed changes

Member Author

yuzefovich left a comment

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

pkg/sql/distsql_running.go, line 909 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think in this case we actually do need to close all the top level plans. The comment about this in the other code path is because subqueries are also going to be closed via the main planNode tree. In our cases, we have the only reference to the postqueries.

Oh, thanks, makes sense.

pkg/sql/distsql_running.go, line 939 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

How about returning an internal error?

Done. I forget that now we're using internal errors instead of panics in order to not crash the nodes.

pkg/sql/distsql_running.go, line 944 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

I think this one could be called one day once we implement cascade.

Ok, left a TODO.

pkg/sql/plan.go, line 302 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

nit: s/keys/key/

Done.

pkg/sql/plan.go, line 330 at r1 (raw file):

Previously, jordanlewis (Jordan Lewis) wrote…

What about postquery?

I like it - it has a nice symmetry with "subqueries."

Member

RaduBerinde commented Jun 20, 2019

pkg/sql/plan.go, line 330 at r1 (raw file):

Previously, yuzefovich wrote…

I like it - it has a nice symmetry with "subqueries."

👍

Member

RaduBerinde commented Jun 20, 2019

We should also add some kind of way of showing these queries in EXPLAIN. Doesn't need to be in this PR though.

Member

RaduBerinde commented Jun 21, 2019

Would be good to rebase this when you can, it's hard to work on top of it and on top of other recent changes I merged.

yuzefovich force-pushed the fk branch from cc8f605 to 5baf0ad Compare

June 21, 2019 02:37

Member Author

yuzefovich commented Jun 21, 2019

Rebased.

@jordanlewis do you want to take another look?

Member Author

yuzefovich commented Jun 21, 2019

We should also add some kind of way of showing these queries in EXPLAIN. Doesn't need to be in this PR though.

Let's do it in a separate PR. I'll open an issue.

yuzefovich mentioned this pull request

sql: display postqueries in EXPLAIN #38340

Closed

Member

RaduBerinde commented Jun 21, 2019

Are we cleaning up everything after the postqueries? I built something on top of f03b5bb91b and got this:

panic: txn: unexpected 10240 leftover bytes [recovered]
	panic: panic while executing 1 statements: INSERT INTO _ VALUES (_, _); caused by txn: unexpected 10240 leftover bytes

goroutine 762 [running]:
github.com/cockroachdb/cockroach/pkg/sql.(*connExecutor).closeWrapper(0xc0015fcd80, 0x32c3800, 0xc001592240, 0x27e4880, 0xc000359410)
	/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:729 +0x330
github.com/cockroachdb/cockroach/pkg/sql.(*Server).ServeConn.func1(0xc0015fcd80, 0x32c3800, 0xc001592240)
	/go/src/github.com/cockroachdb/cockroach/pkg/sql/conn_executor.go:442 +0x61
panic(0x27e4880, 0xc000359410)
	/usr/local/go/src/runtime/panic.go:522 +0x1b5
github.com/cockroachdb/cockroach/pkg/util/log.ReportOrPanic(0x32c3800, 0xc000771e80, 0xc000bc2000, 0xc001431c80, 0x24, 0x0, 0x0, 0x0)
	/go/src/github.com/cockroachdb/cockroach/pkg/util/log/crash_reporting.go:549 +0x226
github.com/cockroachdb/cockroach/pkg/util/mon.(*BytesMonitor).doStop(0xc0013b45a0, 0x32c3800, 0xc000771e80, 0xc001099301)
	/go/src/github.com/cockroachdb/cockroach/pkg/util/mon/bytes_usage.go:394 +0x291
github.com/cockroachdb/cockroach/pkg/util/mon.(*BytesMonitor).Stop(...)
	/go/src/github.com/cockroachdb/cockroach/pkg/util/mon/bytes_usage.go:381
github.com/cockroachdb/cockroach/pkg/sql.(*txnState).finishSQLTxn(0xc0015fce10)
	/go/src/github.com/cockroachdb/cockroach/pkg/sql/txn_state.go:232 +0x5f
github.com/cockroachdb/cockroach/pkg/sql.cleanupAndFinish(0x32c38c0, 0xc00165e0c0, 0x3269840, 0xc00022f420, 0x2a92000, 0xc0015fce10, 0x32696e0, 0xc0003593d0, 0x295cbc0, 0xc0003593e0, ...)


          sql: add support for executing postqueries

c0b1cc3

This commit introduces infrastructure for running "deferred
subqueries" that are to be executed after the execution of the main
query tree which is needed for (among other things) for foreign key
checks.

Release note: None

yuzefovich force-pushed the fk branch from 5baf0ad to c0b1cc3 Compare

June 21, 2019 22:49

yuzefovich commented

View reviewed changes

Member Author

yuzefovich left a comment

I fixed the closing issue, but now running logic tests on Radu's WIP gives:

expected success, but found
                (XXUUU) TransactionStatusError: client already committed or rolled back the transaction. Trying to execute: Scan [/Table/53/1/1,/Table/53/1/1/#) (REASON_UNKNOWN)

My guess is that kv fetcher is somehow closed when we're executing the postquery after we executed the main query (this error appears when we're initializing a table reader for the postquery), but I didn't get much further than that.

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

pkg/sql/distsql_running.go, line 909 at r1 (raw file):

Previously, yuzefovich wrote…

Oh, thanks, makes sense.

I reverted to ignoreClose = true and added a manual closure. The reason is that planner.curPlan, as is, is always the main query, so if we keep ignoreClose = false, then the main query tree is closed multiple times whereas the postqueries remain open.

Member

RaduBerinde commented Jun 22, 2019

I looked around a bit. It may be due to the tablewriter using tb.txn.CommitInBatch (in tableWriterBase.finalize). I think maybe we want to not call enableAutoCommit in execFactory.ConstructPlan if we have postqueries. Will give that a shot.

Member

RaduBerinde commented Jun 24, 2019

Yeah, that worked. LGTM, thanks!

jordanlewis approved these changes

View reviewed changes

Member

jordanlewis left a comment

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

pkg/sql/distsql_running.go, line 909 at r1 (raw file):

Previously, yuzefovich wrote…

I reverted to ignoreClose = true and added a manual closure. The reason is that planner.curPlan, as is, is always the main query, so if we keep ignoreClose = false, then the main query tree is closed multiple times whereas the postqueries remain open.

Ah I see. Okay, sorry about that.

Member Author

yuzefovich commented Jun 24, 2019

TFTRs!

bors r+

craig bot pushed a commit that referenced this pull request


          Merge #38281

225a320

38281: sql: add support for executing postqueries r=yuzefovich a=yuzefovich

This commit introduces infrastructure for running "deferred
subqueries" that are to be executed after the execution of the main
query tree which is needed for (among other things) for foreign key
checks.

Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

Contributor

craig bot commented Jun 24, 2019

Build succeeded

GitHub CI (Cockroach)

craig bot merged commit c0b1cc3 into cockroachdb:master

yuzefovich deleted the fk branch

June 25, 2019 02:22

knz mentioned this pull request

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

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet