Skip to content

Commit 6c8d1c1

Browse files
alvherrealvherre
committed
Ensure correct lock level is used in ALTER ... RENAME
Commit 1b5d797 intended to relax the lock level used to rename indexes, but inadvertently allowed *any* relation to be renamed with a lowered lock level, as long as the command is spelled ALTER INDEX. That's undesirable for other relation types, so retry the operation with the higher lock if the relation turns out not to be an index. After this fix, ALTER INDEX <sometable> RENAME will require access exclusive lock, which it didn't before. Author: Nathan Bossart <bossartn@amazon.com> Author: Álvaro Herrera <alvherre@alvh.no-ip.org> Reported-by: Onder Kalaci <onderk@microsoft.com> Discussion: https://postgr.es/m/PH0PR21MB1328189E2821CDEC646F8178D8AE9@PH0PR21MB1328.namprd21.prod.outlook.com
1 parent 9aef401 commit 6c8d1c1

File tree

3 files changed

+127
-14
lines changed

3 files changed

+127
-14
lines changed

‎src/backend/commands/tablecmds.c

Lines changed: 48 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -3406,7 +3406,7 @@ RenameConstraint(RenameStmt *stmt)
34063406
ObjectAddress
34073407
RenameRelation(RenameStmt *stmt)
34083408
{
3409-
bool is_index = stmt->renameType == OBJECT_INDEX;
3409+
bool is_index_stmt = stmt->renameType == OBJECT_INDEX;
34103410
Oid relid;
34113411
ObjectAddress address;
34123412

@@ -3416,24 +3416,48 @@ RenameRelation(RenameStmt *stmt)
34163416
* end of transaction.
34173417
*
34183418
* Lock level used here should match RenameRelationInternal, to avoid lock
3419-
* escalation.
3419+
* escalation. However, because ALTER INDEX can be used with any relation
3420+
* type, we mustn't believe without verification.
34203421
*/
3421-
relid = RangeVarGetRelidExtended(stmt->relation,
3422-
is_index ? ShareUpdateExclusiveLock : AccessExclusiveLock,
3423-
stmt->missing_ok ? RVR_MISSING_OK : 0,
3424-
RangeVarCallbackForAlterRelation,
3425-
(void *) stmt);
3426-
3427-
if (!OidIsValid(relid))
3422+
for (;;)
34283423
{
3429-
ereport(NOTICE,
3430-
(errmsg("relation \"%s\" does not exist, skipping",
3431-
stmt->relation->relname)));
3432-
return InvalidObjectAddress;
3424+
LOCKMODE lockmode;
3425+
char relkind;
3426+
bool obj_is_index;
3427+
3428+
lockmode = is_index_stmt ? ShareUpdateExclusiveLock : AccessExclusiveLock;
3429+
3430+
relid = RangeVarGetRelidExtended(stmt->relation, lockmode,
3431+
stmt->missing_ok ? RVR_MISSING_OK : 0,
3432+
RangeVarCallbackForAlterRelation,
3433+
(void *) stmt);
3434+
3435+
if (!OidIsValid(relid))
3436+
{
3437+
ereport(NOTICE,
3438+
(errmsg("relation \"%s\" does not exist, skipping",
3439+
stmt->relation->relname)));
3440+
return InvalidObjectAddress;
3441+
}
3442+
3443+
/*
3444+
* We allow mismatched statement and object types (e.g., ALTER INDEX
3445+
* to rename a table), but we might've used the wrong lock level. If
3446+
* that happens, retry with the correct lock level. We don't bother
3447+
* if we already acquired AccessExclusiveLock with an index, however.
3448+
*/
3449+
relkind = get_rel_relkind(relid);
3450+
obj_is_index = (relkind == RELKIND_INDEX ||
3451+
relkind == RELKIND_PARTITIONED_INDEX);
3452+
if (obj_is_index || is_index_stmt == obj_is_index)
3453+
break;
3454+
3455+
UnlockRelationOid(relid, lockmode);
3456+
is_index_stmt = obj_is_index;
34333457
}
34343458

34353459
/* Do the work */
3436-
RenameRelationInternal(relid, stmt->newname, false, is_index);
3460+
RenameRelationInternal(relid, stmt->newname, false, is_index_stmt);
34373461

34383462
ObjectAddressSet(address, RelationRelationId, relid);
34393463

@@ -3481,6 +3505,16 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo
34813505
errmsg("relation \"%s\" already exists",
34823506
newrelname)));
34833507

3508+
/*
3509+
* RenameRelation is careful not to believe the caller's idea of the
3510+
* relation kind being handled. We don't have to worry about this, but
3511+
* let's not be totally oblivious to it. We can process an index as
3512+
* not-an-index, but not the other way around.
3513+
*/
3514+
Assert(!is_index ||
3515+
is_index == (targetrelation->rd_rel->relkind == RELKIND_INDEX ||
3516+
targetrelation->rd_rel->relkind == RELKIND_PARTITIONED_INDEX));
3517+
34843518
/*
34853519
* Update pg_class tuple with new relname. (Scribbling on reltup is OK
34863520
* because it's a copy...)

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

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -232,6 +232,54 @@ SET ROLE regress_alter_table_user1;
232232
ALTER INDEX onek_unique1 RENAME TO fail; -- permission denied
233233
ERROR: must be owner of index onek_unique1
234234
RESET ROLE;
235+
-- rename statements with mismatching statement and object types
236+
CREATE TABLE alter_idx_rename_test (a INT);
237+
CREATE INDEX alter_idx_rename_test_idx ON alter_idx_rename_test (a);
238+
CREATE TABLE alter_idx_rename_test_parted (a INT) PARTITION BY LIST (a);
239+
CREATE INDEX alter_idx_rename_test_parted_idx ON alter_idx_rename_test_parted (a);
240+
BEGIN;
241+
ALTER INDEX alter_idx_rename_test RENAME TO alter_idx_rename_test_2;
242+
ALTER INDEX alter_idx_rename_test_parted RENAME TO alter_idx_rename_test_parted_2;
243+
SELECT relation::regclass, mode FROM pg_locks
244+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
245+
AND relation::regclass::text LIKE 'alter\_idx%'
246+
ORDER BY relation::regclass::text;
247+
relation | mode
248+
--------------------------------+---------------------
249+
alter_idx_rename_test_2 | AccessExclusiveLock
250+
alter_idx_rename_test_parted_2 | AccessExclusiveLock
251+
(2 rows)
252+
253+
COMMIT;
254+
BEGIN;
255+
ALTER INDEX alter_idx_rename_test_idx RENAME TO alter_idx_rename_test_idx_2;
256+
ALTER INDEX alter_idx_rename_test_parted_idx RENAME TO alter_idx_rename_test_parted_idx_2;
257+
SELECT relation::regclass, mode FROM pg_locks
258+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
259+
AND relation::regclass::text LIKE 'alter\_idx%'
260+
ORDER BY relation::regclass::text;
261+
relation | mode
262+
------------------------------------+--------------------------
263+
alter_idx_rename_test_idx_2 | ShareUpdateExclusiveLock
264+
alter_idx_rename_test_parted_idx_2 | ShareUpdateExclusiveLock
265+
(2 rows)
266+
267+
COMMIT;
268+
BEGIN;
269+
ALTER TABLE alter_idx_rename_test_idx_2 RENAME TO alter_idx_rename_test_idx_3;
270+
ALTER TABLE alter_idx_rename_test_parted_idx_2 RENAME TO alter_idx_rename_test_parted_idx_3;
271+
SELECT relation::regclass, mode FROM pg_locks
272+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
273+
AND relation::regclass::text LIKE 'alter\_idx%'
274+
ORDER BY relation::regclass::text;
275+
relation | mode
276+
------------------------------------+---------------------
277+
alter_idx_rename_test_idx_3 | AccessExclusiveLock
278+
alter_idx_rename_test_parted_idx_3 | AccessExclusiveLock
279+
(2 rows)
280+
281+
COMMIT;
282+
DROP TABLE alter_idx_rename_test_2;
235283
-- renaming views
236284
CREATE VIEW attmp_view (unique1) AS SELECT unique1 FROM tenk1;
237285
ALTER TABLE attmp_view RENAME TO attmp_view_new;

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

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,37 @@ SET ROLE regress_alter_table_user1;
227227
ALTER INDEX onek_unique1 RENAME TO fail; -- permission denied
228228
RESET ROLE;
229229

230+
-- rename statements with mismatching statement and object types
231+
CREATE TABLE alter_idx_rename_test (a INT);
232+
CREATE INDEX alter_idx_rename_test_idx ON alter_idx_rename_test (a);
233+
CREATE TABLE alter_idx_rename_test_parted (a INT) PARTITION BY LIST (a);
234+
CREATE INDEX alter_idx_rename_test_parted_idx ON alter_idx_rename_test_parted (a);
235+
BEGIN;
236+
ALTER INDEX alter_idx_rename_test RENAME TO alter_idx_rename_test_2;
237+
ALTER INDEX alter_idx_rename_test_parted RENAME TO alter_idx_rename_test_parted_2;
238+
SELECT relation::regclass, mode FROM pg_locks
239+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
240+
AND relation::regclass::text LIKE 'alter\_idx%'
241+
ORDER BY relation::regclass::text;
242+
COMMIT;
243+
BEGIN;
244+
ALTER INDEX alter_idx_rename_test_idx RENAME TO alter_idx_rename_test_idx_2;
245+
ALTER INDEX alter_idx_rename_test_parted_idx RENAME TO alter_idx_rename_test_parted_idx_2;
246+
SELECT relation::regclass, mode FROM pg_locks
247+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
248+
AND relation::regclass::text LIKE 'alter\_idx%'
249+
ORDER BY relation::regclass::text;
250+
COMMIT;
251+
BEGIN;
252+
ALTER TABLE alter_idx_rename_test_idx_2 RENAME TO alter_idx_rename_test_idx_3;
253+
ALTER TABLE alter_idx_rename_test_parted_idx_2 RENAME TO alter_idx_rename_test_parted_idx_3;
254+
SELECT relation::regclass, mode FROM pg_locks
255+
WHERE pid = pg_backend_pid() AND locktype = 'relation'
256+
AND relation::regclass::text LIKE 'alter\_idx%'
257+
ORDER BY relation::regclass::text;
258+
COMMIT;
259+
DROP TABLE alter_idx_rename_test_2;
260+
230261
-- renaming views
231262
CREATE VIEW attmp_view (unique1) AS SELECT unique1 FROM tenk1;
232263
ALTER TABLE attmp_view RENAME TO attmp_view_new;

0 commit comments

Comments
 (0)