Fix crash caused by some form of ALTER TABLE ADD COLUMN statements. #7522
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emelsimsek
changed the title
onurctirtir
reviewed
Feb 20, 2024
There was a problem hiding this comment.
adding a test would be nice, at least the one noted in linked issue
There was a problem hiding this comment.
I was just going to note that I did not think this deserved a test :)
There was a problem hiding this comment.
We generally try to add all crashes that we fix as a test, for two reasons:
- Make sure that we don't regress in the future
- Generally these crashes happen because of using uncommon SQL features. And increasing our coverage for such uncommon features is quite useful for catching other issues too (e.g. we've caught issues introduced in new PG beta versions because our test suite tests some stuff better than Postgres).
Sometimes we don't of course, for instance if it's hard to reproduce the test reliably. But in this case I think adding the test should be very simple.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #7522 +/- ##
==========================================
- Coverage 89.69% 89.65% -0.04%
==========================================
Files 283 283
Lines 60427 60428 +1
Branches 7524 7525 +1
==========================================
- Hits 54197 54178 -19
- Misses 4078 4097 +19
- Partials 2152 2153 +1 |
onurctirtir
reviewed
Feb 20, 2024
|
|
||
| if (list_length(constraint->pk_attrs) > 0) | ||
| { | ||
| char *referencedColumn = strVal(lfirst(list_head(constraint->pk_attrs))); |
There was a problem hiding this comment.
Only considering head of the list feels a bit misleading because a pkey might cover more than one column too.
create table ref5(a int, b int, primary key(a, b));
JelteF
reviewed
Feb 21, 2024
| @@ -44,6 +44,15 @@ ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and | |||
| DETAIL: Adding a column with a constraint in one command is not supported because all constraints in Citus must have explicit names | |||
| HINT: You can issue each command separately such as ALTER TABLE referencing ADD COLUMN test_8 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name CHECK (check_expression); | |||
| ALTER TABLE referencing ADD COLUMN test_8 integer CONSTRAINT check_test_8 CHECK (test_8 > 0); | |||
| -- error out properly even if the REFERENCES does not include the column list of the referenced table | |||
There was a problem hiding this comment.
Having the PR description include that this is specific the case we're fixing would be good.
|
@JelteF @onurctirtir, is this change good to go? |
JelteF
approved these changes
Mar 19, 2024
emelsimsek
force-pushed
the
Fix-AddColumn-Crash
branch
from
e13d90c to
4d0fb50
Compare
JelteF
pushed a commit
that referenced
this pull request
Apr 16, 2024
…7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. (cherry picked from commit fdd658a)
JelteF
pushed a commit
that referenced
this pull request
Apr 16, 2024
…7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. (cherry picked from commit fdd658a)
JelteF
pushed a commit
that referenced
this pull request
Apr 16, 2024
…7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. (cherry picked from commit fdd658a)
JelteF
pushed a commit
that referenced
this pull request
Apr 16, 2024
…7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. (cherry picked from commit fdd658a)
JelteF
pushed a commit
that referenced
this pull request
Apr 17, 2024
…7522) DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs. For instance, the following statement results in a crash: ``` ALTER TABLE lt ADD COLUMN new_col1 bool, ADD COLUMN new_col2 int references rt; ``` Fixes #7520. (cherry picked from commit fdd658a)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
DESCRIPTION: Fixes a crash caused by some form of ALTER TABLE ADD COLUMN statements. When adding multiple columns, if one of the ADD COLUMN statements contains a FOREIGN constraint ommitting the referenced columns in the statement, a SEGFAULT occurs.
For instance, the following statement results in a crash:
Fixes #7520.