sql: add a default AOST option to CREATE STATISTICS

[Uzair5162]

Using CREATE STATISTICS without an AOST option results in a regular scan which could contend with concurrent transactions. This commit adds a default AOST option of -1us to the CREATE STATISTICS command to avoid this.

Fixes: #72719

Release note (sql change): using the CREATE STATISTICS command without the AS OF SYSTEM TIME option could contend with concurrent transactions and cost performance. Running CREATE STATISTICS without specifying AS OF SYSTEM TIME now uses a default of -1us.

[cockroach-teamcity]

This change is 

[michae2]

Nice PR! It looks great!

This is a straightforward change, and the motivation makes sense to me. But I wonder if anyone has written a script that is calling CREATE STATISTICS manually which will be broken by this new requirement (as our tests were)? Maybe it would be better to add a default AOST if one isn't provided?

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

---

pkg/sql/create_stats.go line 234 at r1 (raw file):

	if n.Options.AsOf.Expr == nil {
		return nil, pgerror.New(pgcode.Syntax,

Instead of returning an error, what would you think of adding a default AOST of -1μs, like we do for ANALYZE?

Here's where that happens:

cockroach/pkg/sql/opt/optbuilder/builder.go

Lines 429 to 444 in d146ecf

	case *tree.CreateStats:
	return b.buildCreateStatistics(stmt, inScope)
	case *tree.Analyze:
	// ANALYZE is syntactic sugar for CREATE STATISTICS. We add AS OF SYSTEM
	// TIME '-0.001ms' to trigger use of inconsistent scans. This prevents
	// GC TTL errors during ANALYZE. See the sql.stats.max_timestamp_age
	// setting.
	return b.buildCreateStatistics(&tree.CreateStats{
	Table: stmt.Table,
	Options: tree.CreateStatsOptions{
	AsOf: tree.AsOfClause{
	Expr: tree.NewStrVal("-0.001ms"),
	},
	},
	}, inScope)

[mgartner]

Maybe it would be better to add a default AOST if one isn't provided?

I like that idea. Is there any reasonable use case for running CREATE STATISTICS without AOST? If not, I think we should do this. If there is some need, maybe we need some setting to allow it for certain cases.

[michae2]

Is there any reasonable use case for running CREATE STATISTICS without AOST?

Not that I can think of that wouldn't also work for -1μs.

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

[Uzair5162]

Thanks for the suggestion! I've changed it to apply a default AOST instead.

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

[DrewKimball]

This is great work!

Reviewed 4 of 4 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Uzair5162)

---

-- commits line 5 at r2:
[nit] Implements a default AOST of -> This commit adds a default AOST option of

---

pkg/sql/opt/optbuilder/testdata/misc_statements line 186 at r3 (raw file):

----
create-statistics
 └── CREATE STATISTICS foo FROM ab WITH OPTIONS AS OF SYSTEM TIME '-1us'

A few other tests might be a good idea here:

1. Make sure that a user-specified AOST doesn't get over-ruled.
2. Make sure this works even if a non-AOST option is specified (like THROTTLING 0.3)

[Uzair5162]

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

---

-- commits line 5 at r2:

Previously, DrewKimball (Drew Kimball) wrote…

[nit] Implements a default AOST of -> This commit adds a default AOST option of

Done.

---

pkg/sql/opt/optbuilder/testdata/misc_statements line 186 at r3 (raw file):

Previously, DrewKimball (Drew Kimball) wrote…

A few other tests might be a good idea here:

1. Make sure that a user-specified AOST doesn't get over-ruled.
2. Make sure this works even if a non-AOST option is specified (like THROTTLING 0.3)

Done.

[DrewKimball]

Nice!

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Uzair5162)

[michae2]

Nice work!

Reviewed 1 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @DrewKimball and @Uzair5162)

---

pkg/sql/opt/optbuilder/misc_statements.go line 152 at r4 (raw file):

	outScope = inScope.push()

	if n.Options.AsOf.Expr == nil {

nit: it might be nice to add a short comment here explaining why the AOST is helpful. (Totally ok to copy the comment from ANALYZE.)

---

pkg/sql/opt/optbuilder/testdata/misc_statements line 186 at r3 (raw file):

Previously, Uzair5162 (Uzair Ahmad) wrote…

Done.

I'm curious, does AS OF SYSTEM TIME '0s' work? That might be a good testcase as well.

[Uzair5162]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @DrewKimball and @michae2)

---

pkg/sql/opt/optbuilder/testdata/misc_statements line 186 at r3 (raw file):

Previously, michae2 (Michael Erickson) wrote…

I'm curious, does AS OF SYSTEM TIME '0s' work? That might be a good testcase as well.

We require the AOST interval to be >= 1µs:

cockroach/pkg/sql/sem/asof/as_of.go

Line 266 in d146ecf

convErr = errors.Errorf("interval value %v too small, absolute value must be >= %v", d, time.Microsecond)

[Uzair5162]

bors r+

[craig]

Build succeeded:

• local_roachtest_fips
• linux_amd64_fips_build
• docker_image_amd64
• unit_tests
• linux_amd64_build
• macos_amd64_build
• acceptance
• windows_build
• examples_orms
• linux_arm64_build
• local_roachtest
• lint
• macos_arm64_build
• check_generated_code

[mgartner]

Great work @Uzair5162!