sql: panic when adding a default value that is assignment cast to the required type #93398
Assignees
Labels
branch-release-22.2
Used to mark GA and release blockers, technical advisories, and bugs for 22.2
C-bug
Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior.
C-technical-advisory
Caused a technical advisory
O-community
Originated from the community
T-sql-foundations
SQL Foundations Team (formerly SQL Schema + SQL Sessions)
X-blathers-triaged
blathers was able to find an owner
Comments
SteveLeungYL
added
the
C-bug
|
Hello, I am Blathers. I am here to help you get the issue triaged. Hoot - a bug! Though bugs are the bane of my existence, rest assured the wretched thing will get the best of care here. I have CC'd a few people who may be able to assist you:
If we have not gotten back to your issue within a few business days, you can try the following:
🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
blathers-crl
bot
added
O-community
|
@SteveLeungYL thanks for the report! It is actually possible to reproduce this crash without the CREATE TABLE t (c1 INT);
INSERT INTO t VALUES (0);
ALTER TABLE t ADD COLUMN c2 DECIMAL DEFAULT pi();
SELECT * FROM t;Note that the problem no longer reproduces
On 22.1 and 21.2 the ALTER statement results in an error If I change the ALTER statement either by adding an explicit cast to decimal (i.e. This all makes me think we have a regression with default expressions typing and the backfill process, going to put back into triage. |
|
I am curious why, on older versions, returns an error? I.e. what's wrong with converting a |
|
I think it was e4a01e0 where we stopped returning an error. This diff re-introduces the error back: It appears that the assignment cast (from a float to a decimal) is expected to happen here during the backfill process, but it doesn't occur, so we encode the value as a float, and then we attempt to read it as a decimal. |
|
@e-mbrown do you think this is related to your work on allowing default column values of different types than the column? I think we should be adding an assignment cast when performing the backfill, but it looks like we aren't. |
rafiss
changed the title
|
Well, we'll need to do the same casts when evaluating the expression as part of backfills, no? |
|
Yes. I tried to find the spot where we'd do it when I was initially debugging the ticket but failed, so I'm hoping someone more familiar with the backfill code do it. |
craig bot
pushed a commit
that referenced
this issue
Jan 18, 2023
95398: sql/catalog/schemaexpr: fix bug when new column default has different… r=ajwerner a=ajwerner … type Since 22.2 we permit default expressions to contain types which are not exactly the same as the column type; it is valid to have an expression which can be cast to the column type in an assignment context. Generally, the optimizer handles inserting the assignment cast into the execution of the relevant mutations. Unfortunately, the cast was not present for backfills. This PR detects the situation where a cast is needed and insert it directly into the plan of the backfill (or import). Epic: None Fixes: #93398 Release note (bug fix): Since 22.2, default expressions can have a type which differs from the type of the column so long as the expression's type can be cast in an assignment context. Unfortunately, this code had a bug when adding new columns. The code in the backfill logic was not sophisticated enough to know to add the cast; when such a default expression was added to a new column it would result in a panic during the backfill. This bug has now been fixed. 95414: sql: fix CLOSE ALL so it doesn't ignore the ALL flag r=jordanlewis a=rafiss informs #83061 Release note (bug fix): Fixed a bug where CLOSE ALL would not respect the "ALL" flag and would instead attempt to close a cursor with no name. 95426: gossip: don't resolve addresses while holding mutex r=erikgrinaker a=erikgrinaker This patch removes a DNS resolution call performed while holding the gossip mutex. This can lead to severe process stalls if the DNS lookup is not immediate, since we need to acquire gossip read locks in several performance critical code paths, including Raft processing. However, the DNS lookup was only done when validating a remote forwarding address, which presumably happens fairly rarely. Removing it should not cause any problems, since the address will necessarily be validated later when attempting to connect to it. Epic: none Release note (bug fix): Fixed a bug where a DNS lookup was performed during gossip remote forwarding while holding the gossip mutex. This could cause processing stalls if the DNS server was slow to respond. Co-authored-by: Andrew Werner <[email protected]> Co-authored-by: Rafi Shamim <[email protected]> Co-authored-by: Erik Grinaker <[email protected]>
ajwerner
added a commit
to ajwerner/cockroach
that referenced
this issue
Jan 18, 2023
… type Since 22.2 we permit default expressions to contain types which are not exactly the same as the column type; it is valid to have an expression which can be cast to the column type in an assignment context. Generally, the optimizer handles inserting the assignment cast into the execution of the relevant mutations. Unfortunately, the cast was not present for backfills. This PR detects the situation where a cast is needed and insert it directly into the plan of the backfill (or import). Epic: None Fixes: cockroachdb#93398 Release note (bug fix): Since 22.2, default expressions can have a type which differs from the type of the column so long as the expression's type can be cast in an assignment context. Unfortunately, this code had a bug when adding new columns. The code in the backfill logic was not sophisticated enough to know to add the cast; when such a default expression was added to a new column it would result in a panic during the backfill. This bug has now been fixed.
mgartner
added a commit
to mgartner/cockroach
that referenced
this issue
Jan 18, 2023
In cockroachdb#95398, casts were added to default expressions during backfills when the default expression's type was not equivalent to the target column's type. This commit fixes two minor bugs with the previous fix: 1. Casts were only added when types were not equivalent, i.e., when they had different families. Assignment casts must be added for non-identical types with the same family. For example, a `TEXT` default expression must be assignment cast to a `CHAR(1)` column, even though they are both in the string family. 2. Explicit casts were added instead of assignment casts, which have different behavior. For example, an assignment cast will error when casting `TEXT` to `CHAR(1)` if the string has a length greater than one. Some tests were added to show that default column backfills behave consistently with Postgres. Informs cockroachdb#93398 Epic: None Release note: None
craig bot
pushed a commit
that referenced
this issue
Jan 21, 2023
92431: roachtest: introduce new mixed-version testing API r=srosenberg a=renatolabs
**roachtest: introduce `clusterupgrade` package**
The `clusterupgrade` package is introduced in this commit as a means
towards the end goal of reusing upgrade-related functionality in
roachtests. Specifically, as we work towards a new API for
mixed-version testing in roachtests, we want to continue to support
existing upgrade tests (at least for the time being). For that reason,
it's useful to be able to reuse certain low level building blocks that
both APIs rely on. The `clusterupgrade` package includes functions to
query the binary and cluster versions on a node, to replace the binary
running on nodes, upload a certain binary, etc. Most of these
functions were moved from the `versionupgrade.go` file.
**roachtest: introduce new mixed-version testing API**
This introduces a new mixed-version API for roachtests, located under
`roachtestutil/mixedversion`. The goal of this API is to provide a
higher level framework than the existing `versionUpgradeTest` set of
functions.
Currently, all mixed-version roachtests rely on teams having to
specify, step by step, which release version nodes should start from,
how and in what order they upgrade, whether they attempt a rollback,
at what point the feature being tested is invoked, etc. All of this
makes the process of writing mixed-version roachtests time consuming
and takes the focus away from what is actually being tested in
mixed-version. In addition, it's typically not feasible to test
multiple combinations of event orderings in mixed-version tests, and
as a consequence most tests only exercise functionality after all
nodes have upgraded to the new version, potentially hiding bugs that
are only exposed if _some_ nodes are running a newer binary, while
others are running an older release.
The new API has the goal of automatically exploring different
orderings in each run. In the most common scenario, teams will only
need to specify what they want tested in a mixed-version setting, and
the framework will decide when that feature will be invoked: in some
runs, it will happen when all nodes are migrated to the new
binary (most common scenario in existing tests); in other runs, it
will happen when only some nodes have upgraded; sometimes it will run
while upgrade migrations are running in the background; etc. It's also
possible to specify multiple features to be tested in mixed-version
state, and the framework will randomly schedule them during the test,
potentially running them concurrently. The randomness is controlled by
a seed that is logged when the test is executed. By reusing the same
seed, the same mixed-version test is generated which helps with
reproducibility (but, of course, does not make the test
deterministic).
Mixed-version tests that use the `mixedversion` package can be broken
down into two stages: _planning_ and _execution_. During planning, all
mixed-version hooks will be scheduled and a test plan is
generated. All randomness is introduced during test plan
generation. The execution stage just takes care of running the plan,
and does not introduce any other randomness. When the mixed-version
test is run, the test plan is printed in a tree-like format, providing
a high level overview of what the test is doing. In addition, each
step of the test is assigned a numeric ID, which can be used to
reference the output of that step during execution. Each step logs to
a different file, and the artifacts containing the output of each
step will live in `mixed-version-test/{ID}.log`.
The new framework builds on top of the recently introduced
`clusterupgrade` package, which provides basic, low-level fiunctions
that are also used by the existing mixed-version tests.
**roachtest: rewrite acceptance/version-upgrade to use new API**
This commit migrates the `acceptace/version-upgrade` test to use the
new API in the `mixedversion` package. Two features were previously
being tested in mixed-version state in this test: backups, and basic
functionality that can be tested by running a SQL statement.
This change simplifies the code as it is no longer required to set up
the nodes, and the concept of the `versionFeatureStep` was removed as
it is not necessary and was only being used by this test.
Epic: CRDB-19321
Release note: None
94431: tsearch: support weight matches; stripped vectors r=jordanlewis a=jordanlewis
Previously, the tsearch package ignored weights when computing matches. It also computed incorrect results against stripped vectors (vectors without position information on all or some of their lexemes).
Weight matches are enabled when a tsquery has an explicit weight set on a term, a b c or d. If that's the case, the matching term in the tsvector must have a matching weight to actually be a match.
Stripped vectors are handled differently as well. If a vector is stripped, it must return false for any queries that relate to expected positions. This means that the followed by operator must keep track of whether it saw any stripped matches in either of its arms, and return false if it does.
This patch now passes the pg_regress test suite for tsvector, and test cases were added to ensure this fact.
Epic: CRDB-22357
Release note: None (no release with these issues)
95459: sql/catalog/schemaexpr: fix default expression assignment casts r=mgartner a=mgartner
In #95398, casts were added to default expressions during backfills when
the default expression's type was not equivalent to the target column's
type. This commit fixes two minor bugs with the previous fix:
1. Casts were only added when types were not equivalent, i.e., when
they had different families. Assignment casts must be added for
non-identical types with the same family. For example, a `TEXT`
default expression must be assignment cast to a `CHAR(1)` column,
even though they are both in the string family.
2. Explicit casts were added instead of assignment casts, which have
different behavior. For example, an assignment cast will error when
casting `TEXT` to `CHAR(1)` if the string has a length greater than
one.
Some tests were added to show that default column backfills behave
consistently with Postgres.
Informs #93398
Epic: None
Release note: None
Co-authored-by: Renato Costa <[email protected]>
Co-authored-by: Jordan Lewis <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
mgartner
added a commit
to mgartner/cockroach
that referenced
this issue
Jan 22, 2023
In cockroachdb#95398, casts were added to default expressions during backfills when the default expression's type was not equivalent to the target column's type. This commit fixes two minor bugs with the previous fix: 1. Casts were only added when types were not equivalent, i.e., when they had different families. Assignment casts must be added for non-identical types with the same family. For example, a `TEXT` default expression must be assignment cast to a `CHAR(1)` column, even though they are both in the string family. 2. Explicit casts were added instead of assignment casts, which have different behavior. For example, an assignment cast will error when casting `TEXT` to `CHAR(1)` if the string has a length greater than one. Some tests were added to show that default column backfills behave consistently with Postgres. Informs cockroachdb#93398 Epic: None Release note: None
exalate-issue-sync
bot
added
T-sql-foundations
rytaft
added
C-technical-advisory
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the problem
The latest version of the CockroachDB (dc93fa8) crashes when executing the following query:
To Reproduce
Here is the detail steps to reproduce the bug.
make installin the root repository folder../cockroach demo, and then paste the PoC query to the cockroach cli environment.Expected behavior
The SELECT statement should be executed correctly and it should return matched rows.
Additional data / screenshots
Here is the crashing stack trace:
Environment:
./cockroach demo)Additional context
The crashing problem may not happen in the real-world application, since it requires the specific setup:
SET testing_optimizer_disable_rule_probability = 1.0;, which is a developer option used to control the query optimization. However, a crash is still unexpected from the resulted behavior. Therefore, this problem is reported as a crashing bug.Jira issue: CRDB-22312
The text was updated successfully, but these errors were encountered: