opt: fixup CTE stats on placeholder queries by cucaroach · Pull Request #99433 · cockroachdb/cockroach

cucaroach · 2023-03-23T21:00:26Z

During optbuilder phase we copy the initial expressions stats into the
fake-rel but this value can change when placeholders are assigned so add
code in AssignPlaceholders to rebuild the cte if the stats change.

Fixes: #99389
Epic: none
Release note: none

blathers-crl · 2023-03-23T21:00:31Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

cockroach-teamcity · 2023-03-23T21:00:35Z

This change is

mgartner

Great find!

It's a bit unprecedented for a normalization rule to muck with statistics. Maybe that's ok, but I'm not 100% sure yet. I'm trying to think of a more idiomatic way to achieve the same goal as this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @msirek)

pkg/sql/opt/norm/with_funcs.go line 301 at r2 (raw file):

}

func (c *CustomFuncs) ApplyBindingRowCount(

nit: add comment

pkg/sql/opt/norm/with_funcs.go line 315 at r2 (raw file):

}

func (c *CustomFuncs) RowCountDifferent(

nit: add comment explaining this function.

pkg/sql/opt/norm/rules/with.opt line 68 at r2 (raw file):

)

# ApplyBindingRowCountToCTE makes sure to that stats changes are propagated on

nit: typo in "makes sure to that"

pkg/sql/opt/xform/testdata/rules/cte line 23 at r2 (raw file):


# Regression test for #99389.
assign-placeholders-opt query-args=(1) format=show-stats

Can you move this test to pkg/sql/opt/norm/testdata/rules/with and use assign-placeholder-norm instead of assign-placeholders-opt?

msirek

Nice fix!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @mgartner)

pkg/sql/opt/norm/rules/with.opt line 79 at r2 (raw file):

=>
(RecursiveCTE
    (ApplyBindingRowCount $binding $initial)

It looks like this will update the original binding expression with a new row count.
Shouldn't an expression in the memo be treated as immutable? The updated expression would have a different hash than what was computed when it was placed in the internCache.

We may want to make a new FakeRel and update its row count.

rytaft · 2023-03-24T17:05:34Z

It's a bit unprecedented for a normalization rule to muck with statistics.

@msirek was playing around with an update to optgen -- did that work?

msirek

One possible downside to using a normalization rule is use of more memory. We memoize new expressions in CopyAndReplaceDefault and then memoize again in the normalization rule.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @mgartner)

msirek

@msirek was playing around with an update to optgen -- did that work?

I haven't tested that yet, but I think it should work if we want to go that route.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach and @mgartner)

DrewKimball

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, and @msirek)

pkg/sql/opt/norm/rules/with.opt line 79 at r2 (raw file):

Previously, msirek (Mark Sirek) wrote…

It looks like this will update the original binding expression with a new row count.
Shouldn't an expression in the memo be treated as immutable? The updated expression would have a different hash than what was computed when it was placed in the internCache.

We may want to make a new FakeRel and update its row count.

+1 - we should make a new one and update the binding stored in the metadata. That would probably require adding a new method to opt.Metadata similar to AddWithBinding that would instead update an existing binding.

pkg/sql/opt/norm/rules/with.opt line 81 at r2 (raw file):

    (ApplyBindingRowCount $binding $initial)
    $initial
    $recursive

I think the recursive branch will also need to be reconstructed. The test case didn't catch it because the recursive branch is a contradiction that gets simplified, but a more interesting case should do the trick.

rytaft · 2023-03-24T17:12:34Z

I haven't tested that yet, but I think it should work if we want to go that route.

Would be interesting to know if that approach is simpler / would avoid the potential downsides of a normalization rule

DrewKimball · 2023-03-24T17:32:29Z

That would probably require adding a new method to opt.Metadata similar to AddWithBinding that would instead update an existing binding.

Actually, AddWithBinding already does what we need, so no need for a new method.

msirek

I ran the regression test in this PR against the CopyAndReplaceDefault fix and got the same results.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, and @mgartner)

mgartner · 2023-03-24T19:35:53Z

Is an optgen language change necessary? We already have the onConstructRelational hook which is called at the end of Construct... functions. Can we put this logic there?

mgartner · 2023-03-24T19:40:30Z

Forgot the code pointer:

cockroach/pkg/sql/opt/norm/factory.go

Lines 391 to 416 in 63d9aed

    
           // onConstructRelational is called as a final step by each factory method that 
        
           // constructs a relational expression, so that any custom manual pattern 
        
           // matching/replacement code can be run. 
        
           func (f *Factory) onConstructRelational(rel memo.RelExpr) memo.RelExpr { 
        
           	// [SimplifyZeroCardinalityGroup] 
        
           	// SimplifyZeroCardinalityGroup replaces a group with [0 - 0] cardinality 
        
           	// with an empty values expression. It is placed here because it depends on 
        
           	// the logical properties of the group in question. 
        
           	if rel.Op() != opt.ValuesOp { 
        
           		relational := rel.Relational() 
        
           		// We can do this if we only contain leakproof operators. As an example of 
        
           		// an immutable operator that should not be folded: a Limit on top of an 
        
           		// empty input has to error out if the limit turns out to be negative. 
        
           		if relational.Cardinality.IsZero() && relational.VolatilitySet.IsLeakproof() { 
        
           			if f.matchedRule == nil || f.matchedRule(opt.SimplifyZeroCardinalityGroup) { 
        
           				values := f.funcs.ConstructEmptyValues(relational.OutputCols) 
        
           				if f.appliedRule != nil { 
        
           					f.appliedRule(opt.SimplifyZeroCardinalityGroup, nil, values) 
        
           				} 
        
           				return values 
        
           			} 
        
           		} 
        
           	} 
        
           	return rel 
        
           }

cucaroach · 2023-03-24T20:39:21Z

onConstructRelational

That looks like just the thing! I'll give that a whirl.

msirek

Is an optgen language change necessary? We already have the onConstructRelational hook which is called at the end of Construct... functions. Can we put this logic there?

onConstructRelational is called after the expression is memoized, so if we alter the expression, we'll have to memoize it again (which may add another expression to the memo), unless we swap the order of the MemoizeXXX call and onConstructRelational.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @cucaroach, @DrewKimball, and @mgartner)

cucaroach · 2023-03-25T16:20:17Z

onConstructRelational is called after the expression is memoized, so if we alter the expression, we'll have to memoize it again (which may add another expression to the memo), unless we swap the order of the MemoizeXXX call and onConstructRelational.

I think if we change something we have to re memoize and intern anyway no? FakeRel interns by props pointer value and it seems unsound to just mutate it. And like Drew said we need to reconstruct the recursive branch as well.

cucaroach · 2023-03-27T14:18:19Z

Okay based on Drew's feedback I think we have to rebuild stuff (and trying to avoid rebuilding seems silly given that the model of assign-placeholders/replace is that if we change something all the ancestors are rebuilt). So I tried I cleaned it up and basically went with same model.

RFAL!

cucaroach · 2023-03-27T19:12:18Z

New approach inspired by Mark's approach, what do we think?

cucaroach · 2023-03-27T19:34:13Z

pkg/sql/opt/norm/factory.go

+			if newInitial != rcte.Initial {
+				newBinding := f.ConstructFakeRel(&memo.FakeRelPrivate{
+					Props: MakeBindingPropsForRecursiveCTE(
+						props.AnyCardinality, rcte.Binding.Relational().OutputCols, newInitial.Relational().Statistics().RowCount)})


See fixup for possible improvement to cardinality arg.

DrewKimball

I like this, AssignPlaceholders feels like the right place to solve it.

Reviewed 1 of 2 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @cucaroach, @mgartner, @msirek, and @rytaft)

-- commits line 6 at r6:
I think the commit message needs an update.

pkg/sql/opt/norm/factory.go line 363 at r6 (raw file):

Previously, cucaroach (Tommy Reilly) wrote…

See fixup for possible improvement to cardinality arg.

I don't think it should be necessary to set the cardinality here, since there are already rules that should handle it (ApplyLimitToRecursiveCTEScan and TryAddLimitToRecursiveBranch).

pkg/sql/opt/norm/factory.go line 359 at r7 (raw file):

		if rcte, ok := e.(*memo.RecursiveCTEExpr); ok {
			newInitial := f.CopyAndReplaceDefault(rcte.Initial, replaceFn).(memo.RelExpr)
			if newInitial != rcte.Initial {

We could also check if the row-counts are different here.

cucaroach

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @DrewKimball, @mgartner, @msirek, and @rytaft)

-- commits line 6 at r6:

Previously, DrewKimball (Drew Kimball) wrote…

I think the commit message needs an update.

Done.

pkg/sql/opt/norm/factory.go line 363 at r6 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

I don't think it should be necessary to set the cardinality here, since there are already rules that should handle it (ApplyLimitToRecursiveCTEScan and TryAddLimitToRecursiveBranch).

👍

pkg/sql/opt/norm/factory.go line 359 at r7 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

We could also check if the row-counts are different here.

I thought about it but what if CopyAndReplaceDefault rebuilt the thing and changed something else but not the stats, doing this seemed more sane.

pkg/sql/opt/xform/testdata/rules/cte line 23 at r2 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Can you move this test to pkg/sql/opt/norm/testdata/rules/with and use assign-placeholder-norm instead of assign-placeholders-opt?

Done.

mgartner

I agree with @DrewKimball, this seems like the most natural solution. Great work!

Reviewed 1 of 2 files at r5, 1 of 1 files at r8, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @cucaroach, @DrewKimball, @msirek, and @rytaft)

-- commits line 10 at r8:
nit: Should we have a performance improvement release note here?

pkg/sql/opt/norm/factory.go line 359 at r8 (raw file):

		}
		if rcte, ok := e.(*memo.RecursiveCTEExpr); ok {
			newInitial := f.CopyAndReplaceDefault(rcte.Initial, replaceFn).(memo.RelExpr)

nit: A comment explaining what we're doing here would be good, since it's not obvious.

During optbuilder phase we copy the initial expressions stats into the fake-rel but this value can change when placeholders are assigned so add code in AssignPlaceholders to rebuild the cte if the stats change. Fixes: cockroachdb#99389 Epic: none Release note (sql change): Prepared statements using placeholders in recursive CTEs sometimes did not re-optimize correctly after plugging in the parameters leading to poor plan choices, this has been fixed.

cucaroach · 2023-03-27T21:12:33Z

TFTRs! Nice teamwork!
bors r+

craig · 2023-03-27T23:44:07Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-28T02:18:43Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-28T04:26:24Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-28T10:37:21Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-03-28T13:47:53Z

Build failed:

Bazel Essential CI (Cockroach)

cucaroach · 2023-03-28T14:28:52Z

bors retry

craig · 2023-03-28T16:36:29Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-03-28T16:36:40Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 2014cf2 to blathers/backport-release-22.1-99433: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

error creating merge commit from 2014cf2 to blathers/backport-release-22.2-99433: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

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

cucaroach mentioned this pull request Mar 23, 2023

[DNM]: norm: recalculate fakeRel stats in CopyAndReplaceDefault #99431

Closed

cucaroach force-pushed the placeholder-cte-stats-fix branch 2 times, most recently from ca5da91 to 9c807dc Compare March 24, 2023 11:53

cucaroach changed the title ~~wip: try to fixup binding stats on normalize~~ opt: fixup CTE stats on placeholder queries Mar 24, 2023

cucaroach marked this pull request as ready for review March 24, 2023 11:55

cucaroach requested a review from a team as a code owner March 24, 2023 11:55

cucaroach requested a review from msirek March 24, 2023 11:55

cucaroach added backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only backport-22.2.x labels Mar 24, 2023

mgartner self-requested a review March 24, 2023 16:49

mgartner reviewed Mar 24, 2023

View reviewed changes

msirek reviewed Mar 24, 2023

View reviewed changes

DrewKimball reviewed Mar 24, 2023

View reviewed changes

msirek reviewed Mar 24, 2023

View reviewed changes

cucaroach force-pushed the placeholder-cte-stats-fix branch from 9c807dc to 4ecd597 Compare March 27, 2023 08:05

cucaroach force-pushed the placeholder-cte-stats-fix branch from 4ecd597 to b092bd6 Compare March 27, 2023 14:19

cucaroach force-pushed the placeholder-cte-stats-fix branch from 60d0ff2 to 9d3d8b3 Compare March 27, 2023 19:11

cucaroach commented Mar 27, 2023

View reviewed changes

DrewKimball approved these changes Mar 27, 2023

View reviewed changes

cucaroach force-pushed the placeholder-cte-stats-fix branch from 41518e5 to 314d1bf Compare March 27, 2023 20:53

cucaroach commented Mar 27, 2023

View reviewed changes

mgartner approved these changes Mar 27, 2023

View reviewed changes

cucaroach force-pushed the placeholder-cte-stats-fix branch from 314d1bf to 2014cf2 Compare March 27, 2023 21:09

msirek approved these changes Mar 27, 2023

View reviewed changes

craig bot merged commit 0745cd4 into cockroachdb:master Mar 28, 2023

blathers-crl bot mentioned this pull request Mar 28, 2023

release-23.1: opt: fixup CTE stats on placeholder queries #99823

Merged

cockroach-teamcity mentioned this pull request Mar 29, 2023

PR #99433 - opt: fixup CTE stats on placeholder queries cockroachdb/docs#16627

Closed

This was referenced Mar 31, 2023

release-22.2: opt: fixup CTE stats on placeholder queries #100327

Merged

release-22.1: opt: fixup CTE stats on placeholder queries #100455

Closed

cucaroach removed the backport-22.1.x label Apr 5, 2023

Conversation

cucaroach commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Mar 23, 2023

Uh oh!

cockroach-teamcity commented Mar 23, 2023

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

msirek left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft commented Mar 24, 2023

Uh oh!

msirek left a comment

Choose a reason for hiding this comment

Uh oh!

msirek left a comment

Choose a reason for hiding this comment

Uh oh!

DrewKimball left a comment

Choose a reason for hiding this comment

Uh oh!

rytaft commented Mar 24, 2023

Uh oh!

DrewKimball commented Mar 24, 2023

Uh oh!

msirek left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner commented Mar 24, 2023

Uh oh!

mgartner commented Mar 24, 2023

Uh oh!

cucaroach commented Mar 24, 2023

Uh oh!

msirek left a comment

Choose a reason for hiding this comment

Uh oh!

cucaroach commented Mar 25, 2023

Uh oh!

cucaroach commented Mar 27, 2023

Uh oh!

cucaroach commented Mar 27, 2023

Uh oh!

cucaroach Mar 27, 2023

Choose a reason for hiding this comment

Uh oh!

DrewKimball left a comment

Choose a reason for hiding this comment

Uh oh!

cucaroach left a comment

Choose a reason for hiding this comment

Uh oh!

mgartner left a comment

Choose a reason for hiding this comment

Uh oh!

cucaroach commented Mar 27, 2023

Uh oh!

craig bot commented Mar 27, 2023

Uh oh!

craig bot commented Mar 28, 2023

Uh oh!

craig bot commented Mar 28, 2023

Uh oh!

craig bot commented Mar 28, 2023

Uh oh!

craig bot commented Mar 28, 2023

Uh oh!

cucaroach commented Mar 28, 2023

Uh oh!

craig bot commented Mar 28, 2023

Uh oh!

blathers-crl bot commented Mar 28, 2023

Uh oh!

Reviewers

Assignees

Labels

cucaroach commented Mar 23, 2023 •

edited

Loading