Skip to content

Commit 6d4395b

Browse files
tglsfdctglsfdc
committed
Obtain required table lock during cross-table updates, redux.
Commits 8319e5c et al missed the fact that ATPostAlterTypeCleanup contains three calls to ATPostAlterTypeParse, and the other two also need protection against passing a relid that we don't yet have lock on. Add similar logic to those code paths, and add some test cases demonstrating the need for it. In v18 and master, the test cases demonstrate that there's a behavioral discrepancy between stored generated columns and virtual generated columns: we disallow changing the expression of a stored column if it's used in any composite-type columns, but not that of a virtual column. Since the expression isn't actually relevant to either sort of composite-type usage, this prohibition seems unnecessary; but changing it is a matter for separate discussion. For now we are just documenting the existing behavior. Reported-by: jian he <jian.universality@gmail.com> Author: jian he <jian.universality@gmail.com> Reviewed-by: Tom Lane <tgl@sss.pgh.pa.us> Discussion: CACJufxGKJtGNRRSXfwMW9SqVOPEMdP17BJ7DsBf=tNsv9pWU9g@mail.gmail.com Backpatch-through: 13
1 parent ba9201b commit 6d4395b

File tree

5 files changed

+87
-0
lines changed

5 files changed

+87
-0
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13926,6 +13926,14 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
1392613926
Oid relid;
1392713927

1392813928
relid = IndexGetRelation(oldId, false);
13929+
13930+
/*
13931+
* As above, make sure we have lock on the index's table if it's not
13932+
* the same table.
13933+
*/
13934+
if (relid != tab->relid)
13935+
LockRelationOid(relid, AccessExclusiveLock);
13936+
1392913937
ATPostAlterTypeParse(oldId, relid, InvalidOid,
1393013938
(char *) lfirst(def_item),
1393113939
wqueue, lockmode, tab->rewrite);
@@ -13942,6 +13950,20 @@ ATPostAlterTypeCleanup(List **wqueue, AlteredTableInfo *tab, LOCKMODE lockmode)
1394213950
Oid relid;
1394313951

1394413952
relid = StatisticsGetRelation(oldId, false);
13953+
13954+
/*
13955+
* As above, make sure we have lock on the statistics object's table
13956+
* if it's not the same table. However, we take
13957+
* ShareUpdateExclusiveLock here, aligning with the lock level used in
13958+
* CreateStatistics and RemoveStatisticsById.
13959+
*
13960+
* CAUTION: this should be done after all cases that grab
13961+
* AccessExclusiveLock, else we risk causing deadlock due to needing
13962+
* to promote our table lock.
13963+
*/
13964+
if (relid != tab->relid)
13965+
LockRelationOid(relid, ShareUpdateExclusiveLock);
13966+
1394513967
ATPostAlterTypeParse(oldId, relid, InvalidOid,
1394613968
(char *) lfirst(def_item),
1394713969
wqueue, lockmode, tab->rewrite);

‎src/test/regress/expected/alter_table.out

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4620,6 +4620,14 @@ create table attbl(a int);
46204620
create table atref(b attbl check ((b).a is not null));
46214621
alter table attbl alter column a type numeric; -- someday this should work
46224622
ERROR: cannot alter table "attbl" because column "atref.b" uses its row type
4623+
alter table atref drop constraint atref_b_check;
4624+
create statistics atref_stat on ((b).a is not null) from atref;
4625+
alter table attbl alter column a type numeric; -- someday this should work
4626+
ERROR: cannot alter table "attbl" because column "atref.b" uses its row type
4627+
drop statistics atref_stat;
4628+
create index atref_idx on atref (((b).a));
4629+
alter table attbl alter column a type numeric; -- someday this should work
4630+
ERROR: cannot alter table "attbl" because column "atref.b" uses its row type
46234631
drop table attbl, atref;
46244632
/* End test case for bug #18970 */
46254633
-- Test that ALTER TABLE rewrite preserves a clustered index

‎src/test/regress/expected/generated.out

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1190,6 +1190,30 @@ Inherits: gtest30
11901190

11911191
ALTER TABLE gtest30_1 ALTER COLUMN b DROP EXPRESSION; -- error
11921192
ERROR: cannot drop generation expression from inherited column
1193+
-- composite type dependencies
1194+
CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text);
1195+
CREATE TABLE gtest31_2 (x int, y gtest31_1);
1196+
ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails
1197+
ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type
1198+
-- bug #18970: these cases are unsupported, but make sure they fail cleanly
1199+
ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL);
1200+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello1');
1201+
ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type
1202+
ALTER TABLE gtest31_2 DROP CONSTRAINT cc;
1203+
CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2;
1204+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello2');
1205+
ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type
1206+
DROP STATISTICS gtest31_2_stat;
1207+
CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b));
1208+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello3');
1209+
ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type
1210+
DROP TABLE gtest31_1, gtest31_2;
1211+
-- Check it for a partitioned table, too
1212+
CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text) PARTITION BY LIST (a);
1213+
CREATE TABLE gtest31_2 (x int, y gtest31_1);
1214+
ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails
1215+
ERROR: cannot alter table "gtest31_1" because column "gtest31_2.y" uses its row type
1216+
DROP TABLE gtest31_1, gtest31_2;
11931217
-- triggers
11941218
CREATE TABLE gtest26 (
11951219
a int PRIMARY KEY,

‎src/test/regress/sql/alter_table.sql

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3032,6 +3032,14 @@ drop table attbl, atref;
30323032
create table attbl(a int);
30333033
create table atref(b attbl check ((b).a is not null));
30343034
alter table attbl alter column a type numeric; -- someday this should work
3035+
alter table atref drop constraint atref_b_check;
3036+
3037+
create statistics atref_stat on ((b).a is not null) from atref;
3038+
alter table attbl alter column a type numeric; -- someday this should work
3039+
drop statistics atref_stat;
3040+
3041+
create index atref_idx on atref (((b).a));
3042+
alter table attbl alter column a type numeric; -- someday this should work
30353043
drop table attbl, atref;
30363044

30373045
/* End test case for bug #18970 */

‎src/test/regress/sql/generated.sql

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -550,6 +550,31 @@ ALTER TABLE ONLY gtest30 ALTER COLUMN b DROP EXPRESSION; -- error
550550
\d gtest30_1
551551
ALTER TABLE gtest30_1 ALTER COLUMN b DROP EXPRESSION; -- error
552552

553+
-- composite type dependencies
554+
CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text);
555+
CREATE TABLE gtest31_2 (x int, y gtest31_1);
556+
ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails
557+
558+
-- bug #18970: these cases are unsupported, but make sure they fail cleanly
559+
ALTER TABLE gtest31_2 ADD CONSTRAINT cc CHECK ((y).b IS NOT NULL);
560+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello1');
561+
ALTER TABLE gtest31_2 DROP CONSTRAINT cc;
562+
563+
CREATE STATISTICS gtest31_2_stat ON ((y).b is not null) FROM gtest31_2;
564+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello2');
565+
DROP STATISTICS gtest31_2_stat;
566+
567+
CREATE INDEX gtest31_2_y_idx ON gtest31_2(((y).b));
568+
ALTER TABLE gtest31_1 ALTER COLUMN b SET EXPRESSION AS ('hello3');
569+
570+
DROP TABLE gtest31_1, gtest31_2;
571+
572+
-- Check it for a partitioned table, too
573+
CREATE TABLE gtest31_1 (a int, b text GENERATED ALWAYS AS ('hello') STORED, c text) PARTITION BY LIST (a);
574+
CREATE TABLE gtest31_2 (x int, y gtest31_1);
575+
ALTER TABLE gtest31_1 ALTER COLUMN b TYPE varchar; -- fails
576+
DROP TABLE gtest31_1, gtest31_2;
577+
553578
-- triggers
554579
CREATE TABLE gtest26 (
555580
a int PRIMARY KEY,

0 commit comments

Comments
 (0)