sql: use a spool operator to fix the correctness of CTE mutations by knz · Pull Request #23824 · cockroachdb/cockroach

knz · 2018-03-14T13:08:34Z

Fixes #20732. This is the very strict minimum amount of code needed to fix that correctness issue, without requiring the other changes in #23373. cc @jordanlewis. I'll want to cherry-pick this.

Prior to this patch, if a mutation statement was used in a CTE or as a
data source with [ ... ] it would run the risk to not complete (and
not commit its results properly) if the surrounding query did not
consume its results fully (which can happen e.g. with LIMIT).

This patch fixes that by introducing a spool operator that does the
right thing. The result rows, if any, are accumulated in memory.

Release note (bug fix): fixed a bug where INSERT/DELETE/UPDATE/UPSERT
may lose updates if run using WITH or the [ ... ] syntax.

cockroach-teamcity · 2018-03-14T13:08:39Z

This change is

knz · 2018-03-14T19:12:31Z

Friendly request to look at this later today, so that we can cover any iteration on the code tomorrow and I can issue the cherry-pick PR. I'm not yet 100% sure I'll have time after that on Friday and in the weekend to both iterate and prepare the cherry-pick.

RaduBerinde · 2018-03-14T20:30:44Z

This is cool and the code came out very clean.

Review status: 0 of 15 files reviewed at latest revision, 6 unresolved discussions, all commit checks successful.

pkg/sql/opt_limits.go, line 138 at r2 (raw file):

			n.hardLimit = numRows
		}
		p.setUnlimited(n.source)

I would add a comment here explaining that it's important that we process all the rows in the spool's source.

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

var _ planNodeFastPath = &upsertNode{}

// planNodeRequireSpool is implemented by nodes whose parent must

[nit] I would say "serves as a marker for nodes .." since it doesn't actually do anything

pkg/sql/spool.go, line 50 at r1 (raw file):

		params.EvalContext().Mon.MakeBoundAccount(),
		sqlbase.ColTypeInfoFromResCols(planColumns(s.source)),
		0,

/* rowCapacity */

pkg/sql/spool.go, line 74 at r1 (raw file):

// FastPathResults implements the planNodeFastPath interface.
func (s *spoolNode) FastPathResults() (int, bool) {
	if f, ok := s.source.(planNodeFastPath); ok {

this could use some commentary. (The fast path is safe to use without spooling?)

pkg/sql/spool.go, line 56 at r2 (raw file):

	)

	// Accumulate all the rows. This also guarantees execution of the

[nit] comment needs updating (we're not accumulating all rows necessarily)

pkg/sql/logictest/testdata/logic_test/spool, line 152 at r1 (raw file):

·            size  1 column, 1 row

# Check that no spool is inserted for a top-level INSERT, but

s/inserted/used

Comments from Reviewable

knz · 2018-03-14T20:36:22Z

Thanks! Radu I will address your comments tomorrow morning. Jordan that gives you a little time if you want to chime in too.

jordanlewis · 2018-03-14T23:18:29Z

very elegant solution

To be clear, does this also fix #22304?

Reviewed 15 of 15 files at r1, 4 of 4 files at r2.
Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/sql/logictest/testdata/logic_test/spool, line 12 at r1 (raw file):

----
limit                  ·     ·
 └── spool             ·     ·

beautiful!

pkg/sql/logictest/testdata/logic_test/spool, line 164 at r1 (raw file):

                │           into  t(x)
                └── values  ·     ·
·                           size  1 column, 1 row

Unless I'm missing something, this is missing an actual execution test. We definitely need one of those, both to exercise the code and as a regression test.

Comments from Reviewable

Prior to this patch, if a mutation statement was used in a CTE or as a data source with [ ... ] it would run the risk to not complete (and not commit its results properly) if the surrounding query did not consume its results fully (which can happen e.g. with LIMIT). This patch fixes that by introducing a spool operator that does the right thing. The result rows, if any, are accumulated in memory. Release note (bug fix): fixed a bug where INSERT/DELETE/UPDATE/UPSERT may lose updates if run using WITH or the [ ... ] syntax.

This patch prevents the accumulation of rows in memory beyond the number required by the current LIMIT clause. Release note: None

knz · 2018-03-15T13:27:17Z

does this also fix #22304?

No it doesn't. #23373 does this.

Review status: 10 of 16 files reviewed at latest revision, 8 unresolved discussions.

pkg/sql/opt_limits.go, line 138 at r2 (raw file):