sql/schemachanger: implement ALTER PRIMARY KEY in the declarative schema changer #83932
Labels
A-schema-changer-impl
Related to the implementation of the new schema changer
A-schema-changes
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Comments
fqazi
added
the
C-enhancement
fqazi
changed the title
postamar
added
A-schema-changes
A-schema-changer-impl
This was referenced Aug 9, 2022
craig bot
pushed a commit
that referenced
this issue
Aug 9, 2022
84514: sql/schemachanger: implemented ALTER PRIMARY KEY for vanilla case r=postamar a=Xiang-Gu
The PR implements `ALTER PRIMARY KEY` under the declarative schema
changer framework that handles the simplest, "vanilla" case like
```
CREATE TABLE t (i INT PRIMARY KEY, j INT NOT NULL)
ALTER TABLE t ALTER PRIMARY KEY USING COLUMNS (j)
```
This is the first of a series PRs where followup PRs will expand its
capabilities to be able to handle more complex cases, including
1. Allow the requested new primary key to be hash-sharded;
2. Consider the case where altering primary key requires us to
modify existing secondary indexes(see the legacy schema change
about in what cases we should rewrite)
3. Consider the case where the old primary index is on the implicitly
created `rowid` column, in which case we also need to drop that
column;
5. Consider partitioning and locality (I'm not sure what they are,
and why they play a role when `ALTER PRIMARY KEY` but I've seen
them in the old schema changer, so I assume we ought to do
something about them too here).
6. Support `ALTER PRIMARY KEY` with concurrent schema change
statements. E.g.
```ALTER TABLE t ADD COLUMN k INT NOT NULL DEFAULT 30, ALTER PRIMARY KEY USING COLUMNS (j);```
related: #83932
Release note: None
84718: sql: populate query-level stats earlier & add contention to telemetry log r=THardy98 a=THardy98
Addresses: #71328
This change adds contention time (measured in nanoseconds) to the
`SampledQuery` telemetry log.
To accomodate this change, we needed to collect query-level statistics
earlier. Previously, query-level statistics were fetched when we called
`Finish` under the `instrumentationHelper`, however this occurred after
we had already emitted our query execution logs. Now, we collect
query-level stats in `dispatchToExecutionEngine` after we've executed
the query.
As a tradeoff to collecting query-level stats earlier, we need to fetch
the trace twice:
- once when populating query-level stats (trace is required to compute
query-level stats) at `populateQueryLevelStats` in
`dispatchToExecutionEngine` after query execution
- once during the instrumentation helper's `Finish` (as we do currently)
This allows us to collect query-level stats earlier without omitting any
tracing events we record currently. This approach is safer, with the
additional overhead of fetching the trace twice only occuring at the
tracing sampling rate of 1-2%, which is fairly conservative. The concern
with only fetching the trace at query-level stats population was the
ommission of a number of events that occur in
`commitSQLTransactionInternal` (or any execution paths that don't lead
to `dispatchToExecutionEngine`).
Release note (sql change): Add `ContentionTime` field to the
`SampledQuery` telemetry log. Query-level statistics are collected
earlier to facilitate the adding of contention time to query execution
logs. The earlier collection of query-level statistics requires the
additional overhead of fetching the query's trace twice (instead of
previously once).
85280: sql, server: add new system privileges for observability r=Santamaura a=Santamaura
This patch introduces 2 new system privileges
VIEWDEBUG and VIEWCLUSTERMETADATA. VIEWDEBUG
will now be used to gate taking traces and viewing
debug endpoints. VIEWCLUSTERMETADATA will now be
used to gate the node and range reports.
Resolves #83844, #83856, #83857, #83858, #83861
Release note (sql change): add VIEWDEBUG and
VIEWCLUSTERMETADATA system privileges.
85458: changefeedccl: add retries to sinkless changefeeds r=jayshrivastava a=jayshrivastava
This change updates core/sinkless changefeeds to run in a retry loop, allowing for changefeed restarts in case of transient errors or declarative schema changes.
See commit notes for more details.
Fixes #85008
85819: kv: use max timestamp during below-Raft scan to gossip liveness r=nvanbenschoten a=nvanbenschoten
Related to https://github.com/cockroachlabs/support/issues/1573.
This commit switches `MaybeGossipNodeLivenessRaftMuLocked` to evaluate its scan
at the maximum timestamp instead of at the local node's HLC time. This ensures
that we gossip the most recent liveness record, regardless of what timestamp it
is written at.
85822: colbuilder: fall back to row-by-row processor wrapping for many renders r=yuzefovich a=yuzefovich
**colbuilder: add a microbenchmark for running many render expressions**
This commit adds a microbenchmark of queries with many render
expressions. It'll be used in the following commit to tune when we fall
back to wrapping a row-by-row processor to handle those renders.
Release note: None
**colbuilder: fall back to row-by-row processor wrapping for many renders**
This commit introduces a mechanism to handle render expressions by
wrapping a row-by-row processor into the vectorized flow when
1. the estimated number of rows to go through the renders is relatively
small
2. the number of renders is relatively high.
The idea is that the vectorized projection operators have higher
overhead when many of them are planned AND there is not enough data to
amortize the overhead, so to improve the performance in those cases
we'll use the row-by-row noop processor. Both of the thresholds are
controlled by cluster settings and the defaults were chosen based on
a representative microbenchmark.
It's worth pointing out that we only have the estimated row count for
the scan operators, so the change has limited applicability.
```
RenderPlanning/rows=1/renders=1-24 407µs ± 2% 408µs ± 2% ~ (p=0.684 n=10+10)
RenderPlanning/rows=1/renders=8-24 516µs ± 1% 537µs ± 1% +4.05% (p=0.000 n=10+10)
RenderPlanning/rows=1/renders=32-24 832µs ± 1% 811µs ± 1% -2.59% (p=0.000 n=10+10)
RenderPlanning/rows=1/renders=64-24 1.22ms ± 0% 1.14ms ± 1% -6.62% (p=0.000 n=9+10)
RenderPlanning/rows=1/renders=128-24 2.02ms ± 0% 1.80ms ± 1% -11.18% (p=0.000 n=8+9)
RenderPlanning/rows=1/renders=512-24 7.75ms ± 1% 5.75ms ± 1% -25.77% (p=0.000 n=10+9)
RenderPlanning/rows=1/renders=4096-24 160ms ± 1% 62ms ± 1% -61.51% (p=0.000 n=10+9)
RenderPlanning/rows=4/renders=1-24 438µs ± 2% 438µs ± 1% ~ (p=0.853 n=10+10)
RenderPlanning/rows=4/renders=8-24 603µs ± 1% 633µs ± 2% +5.06% (p=0.000 n=10+10)
RenderPlanning/rows=4/renders=32-24 1.08ms ± 1% 1.08ms ± 1% ~ (p=0.105 n=10+10)
RenderPlanning/rows=4/renders=64-24 1.72ms ± 0% 1.62ms ± 0% -5.83% (p=0.000 n=9+9)
RenderPlanning/rows=4/renders=128-24 3.01ms ± 1% 2.75ms ± 1% -8.78% (p=0.000 n=10+10)
RenderPlanning/rows=4/renders=512-24 11.6ms ± 1% 9.6ms ± 2% -17.58% (p=0.000 n=10+10)
RenderPlanning/rows=4/renders=4096-24 192ms ± 2% 91ms ± 2% -52.58% (p=0.000 n=10+10)
RenderPlanning/rows=16/renders=1-24 494µs ± 1% 499µs ± 1% +1.03% (p=0.006 n=10+8)
RenderPlanning/rows=16/renders=8-24 855µs ± 1% 901µs ± 1% +5.37% (p=0.000 n=10+10)
RenderPlanning/rows=16/renders=32-24 2.03ms ± 1% 2.04ms ± 1% ~ (p=0.190 n=10+10)
RenderPlanning/rows=16/renders=64-24 3.58ms ± 1% 3.42ms ± 1% -4.56% (p=0.000 n=10+9)
RenderPlanning/rows=16/renders=128-24 6.74ms ± 1% 6.31ms ± 1% -6.37% (p=0.000 n=10+10)
RenderPlanning/rows=16/renders=512-24 26.9ms ± 1% 24.7ms ± 1% -8.24% (p=0.000 n=9+10)
RenderPlanning/rows=16/renders=4096-24 329ms ± 2% 218ms ± 2% -33.66% (p=0.000 n=10+10)
RenderPlanning/rows=64/renders=1-24 666µs ± 1% 659µs ± 2% -1.07% (p=0.007 n=10+10)
RenderPlanning/rows=64/renders=8-24 1.79ms ± 1% 1.84ms ± 1% +3.01% (p=0.000 n=10+10)
RenderPlanning/rows=64/renders=32-24 5.53ms ± 1% 5.79ms ± 2% +4.74% (p=0.000 n=10+10)
RenderPlanning/rows=64/renders=64-24 10.8ms ± 1% 11.0ms ± 1% +1.87% (p=0.000 n=10+9)
RenderPlanning/rows=64/renders=128-24 21.2ms ± 1% 21.7ms ± 1% +2.71% (p=0.000 n=10+10)
RenderPlanning/rows=64/renders=512-24 83.6ms ± 0% 84.9ms ± 0% +1.47% (p=0.000 n=10+7)
RenderPlanning/rows=64/renders=4096-24 824ms ± 1% 751ms ± 2% -8.88% (p=0.000 n=10+10)
RenderPlanning/rows=128/renders=1-24 853µs ± 1% 851µs ± 1% ~ (p=0.481 n=10+10)
RenderPlanning/rows=128/renders=8-24 2.98ms ± 1% 3.11ms ± 1% +4.32% (p=0.000 n=10+10)
RenderPlanning/rows=128/renders=32-24 10.4ms ± 1% 10.9ms ± 1% +5.44% (p=0.000 n=10+10)
RenderPlanning/rows=128/renders=64-24 20.1ms ± 1% 21.3ms ± 1% +5.99% (p=0.000 n=10+10)
RenderPlanning/rows=128/renders=128-24 39.7ms ± 1% 42.1ms ± 2% +5.98% (p=0.000 n=10+10)
RenderPlanning/rows=128/renders=512-24 160ms ± 1% 168ms ± 2% +5.13% (p=0.000 n=9+10)
RenderPlanning/rows=128/renders=4096-24 1.44s ± 1% 1.48s ± 2% +3.15% (p=0.000 n=9+10)
RenderPlanning/rows=256/renders=1-24 1.22ms ± 1% 1.21ms ± 1% -1.01% (p=0.000 n=10+10)
RenderPlanning/rows=256/renders=8-24 5.22ms ± 0% 5.19ms ± 1% -0.54% (p=0.011 n=8+9)
RenderPlanning/rows=256/renders=32-24 19.9ms ± 1% 20.0ms ± 1% ~ (p=0.182 n=9+10)
RenderPlanning/rows=256/renders=64-24 39.0ms ± 0% 38.9ms ± 0% -0.33% (p=0.023 n=10+10)
RenderPlanning/rows=256/renders=128-24 76.8ms ± 1% 76.7ms ± 1% ~ (p=0.739 n=10+10)
RenderPlanning/rows=256/renders=512-24 316ms ± 1% 319ms ± 1% +1.15% (p=0.001 n=9+10)
RenderPlanning/rows=256/renders=4096-24 2.75s ± 1% 2.73s ± 1% -0.64% (p=0.002 n=8+9)
```
Fixes: #85632.
Release note: None
Co-authored-by: Xiang Gu <[email protected]>
Co-authored-by: Thomas Hardy <[email protected]>
Co-authored-by: Santamaura <[email protected]>
Co-authored-by: Jayant Shrivastava <[email protected]>
Co-authored-by: Nathan VanBenschoten <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Aug 22, 2022
This commit fixes bugs related to constraints and comments in the declarative schema changer. This commit also rewrites how index targets are manipulated inside the builder, but otherwise does not change its output. This commit is a spin-off from work performed on adding secondary index support in the ALTER PRIMARY KEY implementation. That work is not going to be included in the 22.2 release, unlike this. Relates to cockroachdb#83932. Release justification: bug fixes with otherwise no functional changes Release note: None
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Aug 24, 2022
This commit fixes bugs related to constraints and comments in the declarative schema changer. This commit also rewrites how index targets are manipulated inside the builder, but otherwise does not change its output. This commit is a spin-off from work performed on adding secondary index support in the ALTER PRIMARY KEY implementation. That work is not going to be included in the 22.2 release, unlike this. Relates to cockroachdb#83932. Release justification: bug fixes with otherwise no functional changes Release note: None
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Aug 26, 2022
This commit fixes bugs related to constraints and comments in the declarative schema changer. This commit also rewrites how index targets are manipulated inside the builder, but otherwise does not change its output. This commit is a spin-off from work performed on adding secondary index support in the ALTER PRIMARY KEY implementation. That work is not going to be included in the 22.2 release, unlike this. Relates to cockroachdb#83932. Release justification: bug fixes with otherwise no functional changes Release note: None
craig bot
pushed a commit
that referenced
this issue
Aug 27, 2022
82435: sql: add json{,b}_to_record{,set}; recordtype srfs r=jordanlewis a=jordanlewis
Closes #70037
This commit adds support for the family of four json{,b}_to_record{,set}
functions, which deconstruct JSON input (either an object or an array)
and return a row or set of rows with well-typed elements from the JSON,
governed by the *table alias* that the function is invoked with.
For example, the query below deconstructs the input JSON, returning each
of the keys "requested" by the table alias definition. Missing keys are
replaced with `NULL`.
```
SELECT * FROM json_to_record('{"a": "b", "c": true}') AS t(a TEXT, z INT)
----
b NULL
```
The logic that governs the type casting from JSON to SQL mimics the
logic in the similar `json_populate_record` family of functions, and
should be identical to Postgres's such logic. It's permissible to use
the virtual table type to deconstruct a sub-object into a record field
within the top level JSON, like this:
```
CREATE TABLE mytable (a INT, b TEXT)
SELECT * FROM json_to_record('{"foo": {"a": 3, "b": "bar"}}') AS t(foo mytable)
----
(3,bar)
```
Functions like these ones that return record types in PostgreSQL must be
aliased to a named and typed tuple (like `AS t(a INT, b INT)`) to be
usable. As a result, this commit:
1. adds parser support for this form of alias, with types
2. pushes the alias information for an aliased expression down the
optimizer's call stack so that the aliased expression can access the
alias information. Previously, the aliased expression was blind to
any aliases that were applied to it.
3. teaches the generator function factories to get the alias information
plumbed in as well.
Release note (sql change): add the json{,b}_to_record{,set} builtin
functions, which transform JSON into structured SQL records.
Release justification: low risk, high reward change to existing functionality
86571: scbuild, rules: rewrite index handling in scbuild, fix bugs r=postamar a=postamar
This commit fixes bugs related to constraints and comments in the
declarative schema changer. This commit also rewrites how index targets
are manipulated inside the builder, but otherwise does not change its
output.
This commit is a spin-off from work performed on adding secondary index
support in the ALTER PRIMARY KEY implementation. That work is not going
to be included in the 22.2 release, unlike this.
Relates to #83932.
Release justification: bug fixes with otherwise no functional changes
Release note: None
86830: backupccl, restoreccl: include system.privileges in full cluster restore r=stevendanna a=RichardJCai
Release justification: Minor enhancement to not yet released feature
Release note: None
Fixes #84762
Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: richardjcai <[email protected]>
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Sep 19, 2022
Previously, we had limited support for ALTER PRIMARY KEY statements in the declarative schema changer. One of the limitations was that secondary indexes were not supported. This commit removes this limitation. Relates to cockroachdb#83932. Release note (sql change): declarative schema changer support for ALTER PRIMARY KEY statements now extends to tables which have secondary indexes. Release justification:
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Sep 22, 2022
Previously, we had limited support for ALTER PRIMARY KEY statements in the declarative schema changer. One of the limitations was that secondary indexes were not supported. This commit removes this limitation. Relates to cockroachdb#83932. Release note (sql change): declarative schema changer support for ALTER PRIMARY KEY statements now extends to tables which have secondary indexes.
postamar
pushed a commit
to postamar/cockroach
that referenced
this issue
Oct 3, 2022
Previously, we had limited support for ALTER PRIMARY KEY statements in the declarative schema changer. One of the limitations was that secondary indexes were not supported. This commit removes this limitation. Relates to cockroachdb#83932. Release note (sql change): declarative schema changer support for ALTER PRIMARY KEY statements now extends to tables which have secondary indexes.
craig bot
pushed a commit
that referenced
this issue
Oct 4, 2022
86176: scbuildstmt: add secondary index support for ALTER PK r=postamar a=postamar
scbuildstmt: add secondary index support for ALTER PK
Previously, we had limited support for ALTER PRIMARY KEY statements in
the declarative schema changer. One of the limitations was that
secondary indexes were not supported. This commit removes this limitation.
Relates to #83932.
Release note (sql change): declarative schema changer support for ALTER
PRIMARY KEY statements now extends to tables which have secondary
indexes.
89201: release: add MacOS ARM64 platform r=rickystewart a=rail
Previously, MacOS ARM64 support was implemented, but not as an officially released platform.
This adds MacOS ARM64 as a build platfomr and published as an unsigned binary. The signing step will be implemented separately.
Release note (build change): Added suport for MacOS ARM64.
89210: roachtest: skip roachtests needing old releases on ARM64 r=renatolabs a=healthy-pod
This code change skips some roachtests on ARM64 because
we do not have enough ARM64 releases to run them.
Release note: None
89300: ci: fix reference to nonexistent variable in `compose` nightly r=rail a=rickystewart
Closes #89190.
Release note: None
Co-authored-by: Marius Posta <[email protected]>
Co-authored-by: Rail Aliiev <[email protected]>
Co-authored-by: healthy-pod <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
|
Update: We've made good progress toward fully supporting ALTER PRIMARY KEY in v23.1. It is by default turned on, and it will only fall back to legacy schema changer in a few cases. I'll leave this issue open until we've lifted all fallback limitations. |
exalate-issue-sync
bot
added
T-sql-foundations
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
We should be trivially be able to support ALTER PRIMARY KEY inside the declarative schema changer, since we already support swapping primary keys because of ADD COLUMN. As a part of this work, we should be able to support cleaning up rowid columns at the same time: #44923, which will depend on DROP COLUMN support.
Jira issue: CRDB-17360
Epic CRDB-31460
The text was updated successfully, but these errors were encountered: