Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash caused by some form of ALTER TABLE ADD COLUMN statements. #7522

Merged
merged 3 commits into from
Mar 20, 2024
Merged

Fix crash caused by some form of ALTER TABLE ADD COLUMN statements. #7522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
94a8fd7
Fix the crash
emelsimsek Feb 19, 2024
eb275e1
Add test
emelsimsek Feb 20, 2024
4d0fb50
Expand list
emelsimsek Feb 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/backend/distributed/commands/table.c
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding a test would be nice, at least the one noted in linked issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just going to note that I did not think this deserved a test :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally try to add all crashes that we fix as a test, for two reasons:

  1. Make sure that we don't regress in the future
  2. 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.

Original file line number Diff line number Diff line change
Expand Up @@ -3053,11 +3053,15 @@
else if (constraint->contype == CONSTR_FOREIGN)
{
RangeVar *referencedTable = constraint->pktable;
char *referencedColumn = strVal(lfirst(list_head(constraint->pk_attrs)));
Oid referencedRelationId = RangeVarGetRelid(referencedTable, NoLock, false);

appendStringInfo(errHint, "FOREIGN KEY (%s) REFERENCES %s(%s)", colName,
get_rel_name(referencedRelationId), referencedColumn);
appendStringInfo(errHint, "FOREIGN KEY (%s) REFERENCES %s", colName,

Check warning on line 3058 in src/backend/distributed/commands/table.c

View check run for this annotation

Codecov / codecov/patch

src/backend/distributed/commands/table.c#L3058 

Added line #L3058 was not covered by tests
get_rel_name(referencedRelationId));

if (list_length(constraint->pk_attrs) > 0)
{
AppendColumnNameList(errHint, constraint->pk_attrs);

Check warning on line 3063 in src/backend/distributed/commands/table.c

View check run for this annotation

Codecov / codecov/patch

src/backend/distributed/commands/table.c#L3063 

Added line #L3063 was not covered by tests
}

if (constraint->fk_del_action == FKCONSTR_ACTION_SETNULL)
{
Expand Down
2 changes: 1 addition & 1 deletion src/backend/distributed/deparser/deparse_table_stmts.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ AppendAlterTableStmt(StringInfo buf, AlterTableStmt *stmt)
* AppendColumnNameList converts a list of columns into comma separated string format
* (colname_1, colname_2, .., colname_n).
*/
static void
void
AppendColumnNameList(StringInfo buf, List *columns)
{
appendStringInfoString(buf, " (");
Expand Down
2 changes: 2 additions & 0 deletions src/include/distributed/deparser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ extern void AppendGrantedByInGrant(StringInfo buf, GrantStmt *stmt);
extern void AppendGrantSharedPrefix(StringInfo buf, GrantStmt *stmt);
extern void AppendGrantSharedSuffix(StringInfo buf, GrantStmt *stmt);

extern void AppendColumnNameList(StringInfo buf, List *columns);

/* Common deparser utils */

typedef struct DefElemOptionFormat
Expand Down
9 changes: 9 additions & 0 deletions src/test/regress/expected/alter_table_add_column.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having the PR description include that this is specific the case we're fixing would be good.

ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced;
ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and CHECK constraints
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_10 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name FOREIGN KEY (test_10) REFERENCES referenced;
ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced(int_col);
ERROR: cannot execute ADD COLUMN command with PRIMARY KEY, UNIQUE, FOREIGN and CHECK constraints
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_10 data_type; ALTER TABLE referencing ADD CONSTRAINT constraint_name FOREIGN KEY (test_10) REFERENCES referenced (int_col );
-- try to add test_6 again, but with IF NOT EXISTS
ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 text;
NOTICE: column "test_6" of relation "referencing" already exists, skipping
Expand Down
4 changes: 4 additions & 0 deletions src/test/regress/sql/alter_table_add_column.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ ALTER TABLE referencing ADD COLUMN "test_\'!7" "simple_!\'custom_type";
ALTER TABLE referencing ADD COLUMN test_8 integer CHECK (test_8 > 0);
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
ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced;
ALTER TABLE referencing ADD COLUMN test_9 bool, ADD COLUMN test_10 int REFERENCES referenced(int_col);

-- try to add test_6 again, but with IF NOT EXISTS
ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 text;
ALTER TABLE referencing ADD COLUMN IF NOT EXISTS test_6 integer;
Expand Down
Loading