sql: enable storage parameter syntax for primary key by chengxiong-ruan · Pull Request #75971 · cockroachdb/cockroach

chengxiong-ruan · 2022-02-03T19:02:12Z

This is a prerequisite to support WITH (bucket_count=...) syntax
for hash sharded index.

Release note (sql change): Previously, WITH (param=value) syntax
is not allowed for primary key. However, that does not align with
postgresql. Also, to support WITH (bucket_count=...) syntax for hash
sharded index, we need to enable it as well. This pr also adds
extra validation to prevent existing storage params from being
applied to primary keys. Which means, even the syntax is accepted
by the parser, none of existing storage param for inverted index
is allowed on primary key. We don't actually set those params
on top of PKs internally, but we need to tell user which param is
supprted or not. Eventually, when we add support for bucket_count
param, a param white list will be maintained for PKs.

cockroach-teamcity · 2022-02-03T19:02:19Z

This change is

chengxiong-ruan · 2022-02-03T19:12:16Z

pkg/sql/create_table.go, line 2691 at r1 (raw file):

// validateUniqueConstraintParamsForCreateTable validate storage params of
// unique constraints passed in through `CREATE TABLE` statement.
func validateUniqueConstraintParamsForCreateTable(n *tree.CreateTable) error {

I didn't put these validations into the param observer pattern because right now we don't call paramparse.SetStorageParameters() for primary index and index of unique constraints since inverted index can't be PK or Unique. We could call it anyway and do validations there, but I don't see a clean way to create hash shard index within the param observer, and I think it's kinda bad to create index within the param observer. We may do a refactor if we want to support other storage params on pk and unique constraint in the future, but right now I think this is sufficient defense and good error hints for users.

postamar

I did a superficial review and couldn't find anything obviously wrong, this looks good enough to me.

Reviewed 15 of 18 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @chengxiong-ruan)

pkg/sql/paramparse/validation.go, line 23 at r2 (raw file):

func ValidateUniqueConstraintParams(params tree.StorageParams, isPK bool) error {
	// TODO (issue 75243): add `bucket_count` as a valid param. Current dummy
	// implementation is just for a proof of concept and make golint happy.

Shouldn't bucket_count only be valid if USING HASH is present, though? Perhaps I'm wrong but the validation of the params seems like it will be more context-dependent than this function lets on. You already need an isPK bool arg to describe the context, perhaps it's worth wrapping it in a struct which describes this context.

Perhaps I'm getting ahead of myself here.

pkg/sql/paramparse/validation.go, line 30 at r2 (raw file):

		return pgerror.Newf(pgcode.InvalidParameterValue, "invalid storage param %q on Primary Key", params[0].Key)
	}
	return pgerror.Newf(pgcode.InvalidParameterValue, "invalid storage param %q on Unique Index", params[0].Key)

nit: no need to capitalize "unique index".

chengxiong-ruan

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @postamar)

pkg/sql/paramparse/validation.go, line 23 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

Shouldn't bucket_count only be valid if USING HASH is present, though? Perhaps I'm wrong but the validation of the params seems like it will be more context-dependent than this function lets on. You already need an isPK bool arg to describe the context, perhaps it's worth wrapping it in a struct which describes this context.

Perhaps I'm getting ahead of myself here.

You're definitely another man ahead of time! Yes, bucket_count should only exists if USING HASH is present. I'm having a follow up pr to actually add this validation. I planned to have this validation outside of here, but I think you brought a good point to wrap more (just one more) context in, will add it in my next pr if you are ok with that.

pkg/sql/paramparse/validation.go, line 30 at r2 (raw file):

Previously, postamar (Marius Posta) wrote…

nit: no need to capitalize "unique index".

sg, will fix this

chengxiong-ruan · 2022-02-03T22:56:03Z

pkg/sql/paramparse/validation.go, line 23 at r2 (raw file):

Previously, chengxiong-ruan (Chengxiong Ruan) wrote…

You're definitely another man ahead of time! Yes, bucket_count should only exists if USING HASH is present. I'm having a follow up pr to actually add this validation. I planned to have this validation outside of here, but I think you brought a good point to wrap more (just one more) context in, will add it in my next pr if you are ok with that.

Actually, validation of bucket_count and USING_HASH coexisting should be held for normal indexes as well. So need to do it outside of this context :(

This is a prerequisite to support `WITH (bucket_count=...)` syntax for hash sharded index. Release note (sql change): Previously, `WITH (param=value)` syntax is not allowed for primary key. However, that does not align with postgresql. Also, to support `WITH (bucket_count=...)` syntax for hash sharded index, we need to enable it as well. This pr also adds extra validation to prevent existing storage params from being applied to primary keys. Which means, even the syntax is accepted by the parser, none of existing storage param for inverted index is allowed on primary key. We don't actually set those params on top of PKs internally, but we need to tell user which param is supprted or not. Eventually, when we add support for `bucket_count` param, a param white list will be maintained for PKs.

chengxiong-ruan · 2022-02-04T14:24:54Z

thanks for the review!
bors r+

craig · 2022-02-04T15:57:00Z

Build succeeded:

GitHub CI (Cockroach)

A follow up of pr cockroachdb#75971 . Will send another pr to update `SHOW CREATE` output. Release note (sql change): `bucket_count` storage parameter is added. To create hash sharded index, now we may use the new syntax: `USING HASH WITH (bucket_count=xxx)`. `bucket_count` storage param can only be use together with `USING HASH`. The old `WITH BUCKET_COUNT=xxx` syntax is still supported for backward compatibility. However, only either the old or new syntax can be used, but not both. Err is returned for clause like `USING HASH WITH BUCKET_COUNT=5 WITH (bucket_count=5).

76068: sql: support bucket_count storage parameter r=chengxiong-ruan a=chengxiong-ruan A follow up of pr #75971 . Will send another pr to update `SHOW CREATE` output. Release note (sql change): `bucket_count` storage parameter is added. To create hash sharded index, now we may use the new syntax: `USING HASH WITH (bucket_count=xxx)`. `bucket_count` storage param can only be use together with `USING HASH`. The old `WITH BUCKET_COUNT=xxx` syntax is still supported for backward compatibility. However, only either the old or new syntax can be used, but not both. Err is returned for clause like `USING HASH WITH BUCKET_COUNT=5 WITH (bucket_count=5). Co-authored-by: Chengxiong Ruan <chengxiongruan@gmail.com>

chengxiong-ruan force-pushed the enable-with-storage-param-syntax-for-pk branch 2 times, most recently from 2adb407 to 686ab41 Compare February 3, 2022 21:22

chengxiong-ruan marked this pull request as ready for review February 3, 2022 21:23

chengxiong-ruan requested review from a team and ajwerner February 3, 2022 21:23

postamar approved these changes Feb 3, 2022

View reviewed changes

chengxiong-ruan commented Feb 3, 2022

View reviewed changes

chengxiong-ruan force-pushed the enable-with-storage-param-syntax-for-pk branch from 686ab41 to 814d219 Compare February 3, 2022 22:38

chengxiong-ruan force-pushed the enable-with-storage-param-syntax-for-pk branch from 814d219 to 350354f Compare February 4, 2022 02:11

craig bot merged commit b51c9b4 into cockroachdb:master Feb 4, 2022

cockroach-teamcity mentioned this pull request Feb 4, 2022

sql: enable storage parameter syntax for primary key cockroachdb/docs#12942

Closed

chengxiong-ruan mentioned this pull request Feb 4, 2022

sql: support bucket_count storage parameter #76068

Merged

chengxiong-ruan deleted the enable-with-storage-param-syntax-for-pk branch February 4, 2022 17:54

chengxiong-ruan mentioned this pull request Feb 9, 2022

sql: the number of buckets in a hash-sharded index should be configured in a storage parameter #75243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: enable storage parameter syntax for primary key#75971

sql: enable storage parameter syntax for primary key#75971
craig[bot] merged 1 commit intocockroachdb:masterfrom
chengxiong-ruan:enable-with-storage-param-syntax-for-pk

chengxiong-ruan commented Feb 3, 2022 •

edited

Loading

Uh oh!

cockroach-teamcity commented Feb 3, 2022

Uh oh!

chengxiong-ruan commented Feb 3, 2022

Uh oh!

postamar left a comment

Uh oh!

chengxiong-ruan left a comment

Uh oh!

chengxiong-ruan commented Feb 3, 2022

Uh oh!

chengxiong-ruan commented Feb 4, 2022

Uh oh!

craig bot commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

chengxiong-ruan commented Feb 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Feb 3, 2022

Uh oh!

chengxiong-ruan commented Feb 3, 2022

Uh oh!

postamar left a comment

Choose a reason for hiding this comment

Uh oh!

chengxiong-ruan left a comment

Choose a reason for hiding this comment

Uh oh!

chengxiong-ruan commented Feb 3, 2022

Uh oh!

chengxiong-ruan commented Feb 4, 2022

Uh oh!

craig bot commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chengxiong-ruan commented Feb 3, 2022 •

edited

Loading