I was about to push this when it occurred to me that it seems a bit pointless to release AEL in order to retry with the lighter lock; once we have AEL, let's just keep it and proceed. So how about the attached?
I'm now thinking that this is to back-patch all the way to 12. -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
>From 04334bc719efe69f63c8a590b9c682a22c6fb413 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 19 Oct 2021 17:02:17 -0300 Subject: [PATCH v3] Ensure correct lock level is used in ALTER ... RENAME Commit 1b5d797cd4f7 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. Author: Nathan Bossart <bossa...@amazon.com> Reported-by: Onder Kalaci <ond...@microsoft.com> Discussion: https://postgr.es/m/ph0pr21mb1328189e2821cdec646f8178d8...@ph0pr21mb1328.namprd21.prod.outlook.com --- src/backend/commands/tablecmds.c | 60 +++++++++++++++++------ src/test/regress/expected/alter_table.out | 39 +++++++++++++++ src/test/regress/sql/alter_table.sql | 25 ++++++++++ 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 487852a14e..f02399f699 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -3735,7 +3735,7 @@ RenameConstraint(RenameStmt *stmt) ObjectAddress RenameRelation(RenameStmt *stmt) { - bool is_index = stmt->renameType == OBJECT_INDEX; + bool is_index_stmt = stmt->renameType == OBJECT_INDEX; Oid relid; ObjectAddress address; @@ -3745,24 +3745,45 @@ RenameRelation(RenameStmt *stmt) * end of transaction. * * Lock level used here should match RenameRelationInternal, to avoid lock - * escalation. + * escalation. However, because ALTER INDEX can be used with any relation + * type, we mustn't believe without verification. */ - relid = RangeVarGetRelidExtended(stmt->relation, - is_index ? ShareUpdateExclusiveLock : AccessExclusiveLock, - stmt->missing_ok ? RVR_MISSING_OK : 0, - RangeVarCallbackForAlterRelation, - (void *) stmt); - - if (!OidIsValid(relid)) + for (;;) { - ereport(NOTICE, - (errmsg("relation \"%s\" does not exist, skipping", - stmt->relation->relname))); - return InvalidObjectAddress; + LOCKMODE lockmode; + bool obj_is_index; + + lockmode = is_index_stmt ? ShareUpdateExclusiveLock : AccessExclusiveLock; + + relid = RangeVarGetRelidExtended(stmt->relation, lockmode, + stmt->missing_ok ? RVR_MISSING_OK : 0, + RangeVarCallbackForAlterRelation, + (void *) stmt); + + if (!OidIsValid(relid)) + { + ereport(NOTICE, + (errmsg("relation \"%s\" does not exist, skipping", + stmt->relation->relname))); + return InvalidObjectAddress; + } + + /* + * We allow mismatched statement and object types (e.g., ALTER INDEX + * to rename a table), but we might've used the wrong lock level. If + * that happens, retry with the correct lock level. We don't bother + * if we already acquired AccessExclusiveLock with an index, however. + */ + obj_is_index = (get_rel_relkind(relid) == RELKIND_INDEX); + if (obj_is_index || is_index_stmt == obj_is_index) + break; + + UnlockRelationOid(relid, lockmode); + is_index_stmt = obj_is_index; } /* Do the work */ - RenameRelationInternal(relid, stmt->newname, false, is_index); + RenameRelationInternal(relid, stmt->newname, false, is_index_stmt); ObjectAddressSet(address, RelationRelationId, relid); @@ -3810,6 +3831,17 @@ RenameRelationInternal(Oid myrelid, const char *newrelname, bool is_internal, bo errmsg("relation \"%s\" already exists", newrelname))); + /* + * RenameRelation is careful not to believe the caller's idea of the + * relation kind being handled. We don't have to worry about this (at + * least not until ALTER TABLE .. ADD CONSTRAINT ... USING INDEX is + * implemented for partitioned tables), but let's not be totally oblivious + * to it. We can process an index as not-an-index, but not the other way + * around. + */ + Assert(!is_index || + is_index == (targetrelation->rd_rel->relkind == RELKIND_INDEX)); + /* * Update pg_class tuple with new relname. (Scribbling on reltup is OK * because it's a copy...) diff --git a/src/test/regress/expected/alter_table.out b/src/test/regress/expected/alter_table.out index fa54bef89a..a3a18b23bf 100644 --- a/src/test/regress/expected/alter_table.out +++ b/src/test/regress/expected/alter_table.out @@ -232,6 +232,45 @@ SET ROLE regress_alter_table_user1; ALTER INDEX onek_unique1 RENAME TO fail; -- permission denied ERROR: must be owner of index onek_unique1 RESET ROLE; +-- rename statements with mismatching statement and object types +CREATE TABLE alter_idx_rename_test (a INT); +CREATE INDEX alter_idx_rename_test_idx ON alter_idx_rename_test (a); +BEGIN; +ALTER INDEX alter_idx_rename_test RENAME TO alter_idx_rename_test_2; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%'; + relation | mode +-------------------------+--------------------- + alter_idx_rename_test_2 | AccessExclusiveLock +(1 row) + +COMMIT; +BEGIN; +ALTER INDEX alter_idx_rename_test_idx RENAME TO alter_idx_rename_test_idx_2; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%' +ORDER BY relation::regclass::text; + relation | mode +-----------------------------+-------------------------- + alter_idx_rename_test_idx_2 | ShareUpdateExclusiveLock +(1 row) + +COMMIT; +BEGIN; +ALTER TABLE alter_idx_rename_test_idx_2 RENAME TO alter_idx_rename_test_idx_3; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%' +ORDER BY relation::regclass::text; + relation | mode +-----------------------------+--------------------- + alter_idx_rename_test_idx_3 | AccessExclusiveLock +(1 row) + +COMMIT; +DROP TABLE alter_idx_rename_test_2; -- renaming views CREATE VIEW attmp_view (unique1) AS SELECT unique1 FROM tenk1; ALTER TABLE attmp_view RENAME TO attmp_view_new; diff --git a/src/test/regress/sql/alter_table.sql b/src/test/regress/sql/alter_table.sql index 0c61604456..d9b9e4b02b 100644 --- a/src/test/regress/sql/alter_table.sql +++ b/src/test/regress/sql/alter_table.sql @@ -227,6 +227,31 @@ SET ROLE regress_alter_table_user1; ALTER INDEX onek_unique1 RENAME TO fail; -- permission denied RESET ROLE; +-- rename statements with mismatching statement and object types +CREATE TABLE alter_idx_rename_test (a INT); +CREATE INDEX alter_idx_rename_test_idx ON alter_idx_rename_test (a); +BEGIN; +ALTER INDEX alter_idx_rename_test RENAME TO alter_idx_rename_test_2; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%'; +COMMIT; +BEGIN; +ALTER INDEX alter_idx_rename_test_idx RENAME TO alter_idx_rename_test_idx_2; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%' +ORDER BY relation::regclass::text; +COMMIT; +BEGIN; +ALTER TABLE alter_idx_rename_test_idx_2 RENAME TO alter_idx_rename_test_idx_3; +SELECT relation::regclass, mode FROM pg_locks +WHERE pid = pg_backend_pid() AND locktype = 'relation' + AND relation::regclass::text LIKE 'alter\_idx%' +ORDER BY relation::regclass::text; +COMMIT; +DROP TABLE alter_idx_rename_test_2; + -- renaming views CREATE VIEW attmp_view (unique1) AS SELECT unique1 FROM tenk1; ALTER TABLE attmp_view RENAME TO attmp_view_new; -- 2.30.2
