sql: Add default_int_size to control INT alias #32848
Conversation
|
This change is |
|
please ping me when the other PR merges and this is rebased; I'd prefer to avoid the review until the file list becomes smaller. |
There was a problem hiding this comment.
PTAL: Rebased against master.
Reviewable status:
complete! 0 of 0 LGTMs obtained
There was a problem hiding this comment.
Well done. Just a few minor comments (don't forget to update the release note accordingly).
That said I think some proper treatment of SERIAL is missing here.
Also please emphasize in the release note that this is meant for compatibility with PostgreSQL clients.
Reviewed 13 of 13 files at r1.
Reviewable status:
pkg/sql/exec_util.go, line 87 at r1 (raw file):
// TODO(bob): Remove or n-op this in v2.4: https://github.com/cockroachdb/cockroach/issues/32844 var defaultIntSize = settings.RegisterValidatedIntSetting( "sql.default_int_size",
sql.defaults.xxx_int_size for consistency with other settings
with a suitable word for xxx -- default, base, naked, whatever
pkg/sql/vars.go, line 225 at r1 (raw file):
// TODO(bob): Remove or no-op this in v2.4: https://github.com/cockroachdb/cockroach/issues/32844 `default_int_size`: { Get: func(evalCtx *extendedEvalContext) string {
Turn this session variable into one configurable using SQL integers; you can follow the model of extra_float_digits.
pkg/sql/parser/parse.go, line 107 at r1 (raw file):
// ParseOne parses a sql statement string, ensuring that it contains only a // single statement, and returns that Statement.
Please extend the comment to justify the choice of int8
pkg/sql/parser/sql.y, line 6603 at r1 (raw file):
INT { $$.val = sqllex.(*scanner).nakedIntType
You need to do something about SERIAL too.
There was a problem hiding this comment.
PTAL; comments addressed. I think this is doing the right thing w.r.t. expanding a SERIAL column to always fit the unique_rowid() function.
Reviewable status:
pkg/sql/vars.go, line 225 at r1 (raw file):
Previously, knz (kena) wrote…
Turn this session variable into one configurable using SQL integers; you can follow the model of
extra_float_digits.
Done. Extracted a common makeIntGetStringValFn() per the existing TODO.
pkg/sql/parser/parse.go, line 107 at r1 (raw file):
Previously, knz (kena) wrote…
Please extend the comment to justify the choice of int8
Done.
pkg/sql/parser/sql.y, line 6603 at r1 (raw file):
Previously, knz (kena) wrote…
You need to do something about SERIAL too.
Done. Extended the test to show that a SERIAL column is expanded to SERIAL8 to accept the result of unique_row_id().
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 87 at r1 (raw file):
Previously, knz (kena) wrote…
sql.defaults.xxx_int_sizefor consistency with other settingswith a suitable word for
xxx-- default, base, naked, whatever
Done.
There was a problem hiding this comment.
The behavior you observe here with default_int_size = 4 and SERIAL going to INT8 is OK if and only if you also include a test that demonstrates/exercises that default_int_size = 4 + experimental_serial_normalization = sql_sequence makes SERIAL go to INT4.
You may then want to add a markdown table in the commit message to enumerate exhaustively what SERIAL expands to depending on the values of default_int_size and serial_normalization.
PR LGTM after that.
Reviewed 6 of 6 files at r2.
Reviewable status:
This is a follow-up from #32831 to support #26925. The combination of `default_int_size` interacts with `experimental_serial_normalization`. Sequence types that are ultimately based on `unique_rowid()` must always be INT8 types in order to hold the returned values. Only in the case where `default_int_size=4 experimental_serial_normalization='sql_sequence'` will an INT4 column be created in response to a SERIAL type. Release note (sql change): A new session variable `default_int_size` and cluster setting `sql.defaults.default_int_size` have been added to control how the INT and SERIAL types are interpreted. The default value, 8, causes the INT and SERIAL types to be interpreted as aliases for INT8 and SERIAL8, which have been the historical defaults for CockroachDB. PostgreSQL clients that expect INT and SERIAL to be 32-bit values, can set `default_int_size` to 4, which will cause INT and SERIAL to be aliases for INT4 and SERIAL4. Please note that due to issue #32846, `SET default_int_size` does not take effect until the next statement batch is executed.
|
bors r+ |
32848: sql: Add default_int_size to control INT alias r=bobvawter a=bobvawter This is a follow-up from #32831 to support #26925. The combination of `default_int_size` interacts with `experimental_serial_normalization`. Sequence types that are ultimately based on `unique_rowid()` must always be INT8 types in order to hold the returned values. Only in the case where `default_int_size=4 experimental_serial_normalization='sql_sequence'` will an INT4 column be created in response to a SERIAL type. Release note (sql change): A new session variable `default_int_size` and cluster setting `sql.defaults.default_int_size` have been added to control how the INT and SERIAL types are interpreted. The default value, 8, causes the INT and SERIAL types to be interpreted as aliases for INT8 and SERIAL8, which have been the historical defaults for CockroachDB. PostgreSQL clients that expect INT and SERIAL to be 32-bit values, can set `default_int_size` to 4, which will cause INT and SERIAL to be aliases for INT4 and SERIAL4. Please note that due to issue #32846, `SET default_int_size` does not take effect until the next statement batch is executed. Co-authored-by: Bob Vawter <[email protected]>
Build succeeded |
This is a follow-up from #32831 to support #26925.
The combination of
default_int_sizeinteracts withexperimental_serial_normalization. Sequence types that are ultimately basedon
unique_rowid()must always be INT8 types in order to hold the returnedvalues. Only in the case where
default_int_size=4 experimental_serial_normalization='sql_sequence'will an INT4 column becreated in response to a SERIAL type.
Release note (sql change): A new session variable
default_int_sizeandcluster setting
sql.defaults.default_int_sizehave been added to control howthe INT and SERIAL types are interpreted. The default value, 8, causes the INT
and SERIAL types to be interpreted as aliases for INT8 and SERIAL8, which have
been the historical defaults for CockroachDB. PostgreSQL clients that expect
INT and SERIAL to be 32-bit values, can set
default_int_sizeto 4, which willcause INT and SERIAL to be aliases for INT4 and SERIAL4. Please note that due
to issue #32846,
SET default_int_sizedoes not take effect until the nextstatement batch is executed.