From 0f9b29e5e2b339c3ac18d3cadfc039fdf4ff1a1f Mon Sep 17 00:00:00 2001 From: reshke kirill Date: Mon, 9 Dec 2024 11:49:01 +0000 Subject: [PATCH v3] When making dependency changes, lock the tuple for in-place updates. Also provide regression test for reassigning ownership of non-locals objects (e.g. DATABASE). Reported-By: Alexander Kukushkin Reviewed-By: Noah Misch --- src/backend/catalog/objectaddress.c | 19 ++++++++++++++++++- src/backend/commands/alter.c | 8 +++++++- src/include/catalog/objectaddress.h | 1 + src/test/regress/expected/dependency.out | 4 ++++ src/test/regress/sql/dependency.sql | 5 +++++ 5 files changed, 35 insertions(+), 2 deletions(-) diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c index be7e4a5dd0..25dc59c18a 100644 --- a/src/backend/catalog/objectaddress.c +++ b/src/backend/catalog/objectaddress.c @@ -2784,6 +2784,16 @@ get_object_property_data(Oid class_id) */ HeapTuple get_catalog_object_by_oid(Relation catalog, AttrNumber oidcol, Oid objectId) +{ + return get_catalog_object_by_oid_extended(catalog, oidcol, objectId, false); +} + +/* +* Workhorse for get_catalog_object_by_oid. +* Accepts an extra locktup parameter that indicates whether a tuple lock is required. +*/ +HeapTuple +get_catalog_object_by_oid_extended(Relation catalog, AttrNumber oidcol, Oid objectId, bool locktup) { HeapTuple tuple; Oid classId = RelationGetRelid(catalog); @@ -2791,7 +2801,10 @@ get_catalog_object_by_oid(Relation catalog, AttrNumber oidcol, Oid objectId) if (oidCacheId > 0) { - tuple = SearchSysCacheCopy1(oidCacheId, ObjectIdGetDatum(objectId)); + if (locktup) + tuple = SearchSysCacheLockedCopy1(oidCacheId, ObjectIdGetDatum(objectId)); + else + tuple = SearchSysCacheCopy1(oidCacheId, ObjectIdGetDatum(objectId)); if (!HeapTupleIsValid(tuple)) /* should not happen */ return NULL; } @@ -2816,6 +2829,10 @@ get_catalog_object_by_oid(Relation catalog, AttrNumber oidcol, Oid objectId) systable_endscan(scan); return NULL; } + + if (locktup) + LockTuple(catalog, &tuple->t_self, InplaceUpdateTupleLock); + tuple = heap_copytuple(tuple); systable_endscan(scan); diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index a45f3bb6b8..910d1b1463 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -59,6 +59,7 @@ #include "miscadmin.h" #include "replication/logicalworker.h" #include "rewrite/rewriteDefine.h" +#include "storage/lmgr.h" #include "utils/acl.h" #include "utils/builtins.h" #include "utils/lsyscache.h" @@ -924,7 +925,8 @@ AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId) rel = table_open(catalogId, RowExclusiveLock); - oldtup = get_catalog_object_by_oid(rel, Anum_oid, objectId); + /* Search tuple and lock it. */ + oldtup = get_catalog_object_by_oid_extended(rel, Anum_oid, objectId, true); if (oldtup == NULL) elog(ERROR, "cache lookup failed for object %u of catalog \"%s\"", objectId, RelationGetRelationName(rel)); @@ -1024,6 +1026,8 @@ AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId) /* Perform actual update */ CatalogTupleUpdate(rel, &newtup->t_self, newtup); + UnlockTuple(rel, &oldtup->t_self, InplaceUpdateTupleLock); + /* Update owner dependency reference */ changeDependencyOnOwner(classId, objectId, new_ownerId); @@ -1032,6 +1036,8 @@ AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId) pfree(nulls); pfree(replaces); } + else + UnlockTuple(rel, &oldtup->t_self, InplaceUpdateTupleLock); /* Note the post-alter hook gets classId not catalogId */ InvokeObjectPostAlterHook(classId, objectId, 0); diff --git a/src/include/catalog/objectaddress.h b/src/include/catalog/objectaddress.h index 3a70d80e32..067e1662e4 100644 --- a/src/include/catalog/objectaddress.h +++ b/src/include/catalog/objectaddress.h @@ -69,6 +69,7 @@ extern bool get_object_namensp_unique(Oid class_id); extern HeapTuple get_catalog_object_by_oid(Relation catalog, AttrNumber oidcol, Oid objectId); +extern HeapTuple get_catalog_object_by_oid_extended(Relation catalog, AttrNumber oidcol, Oid objectId, bool locktup); extern char *getObjectDescription(const ObjectAddress *object, bool missing_ok); diff --git a/src/test/regress/expected/dependency.out b/src/test/regress/expected/dependency.out index 74d9ff2998..c1d9ef50c2 100644 --- a/src/test/regress/expected/dependency.out +++ b/src/test/regress/expected/dependency.out @@ -114,6 +114,7 @@ FROM pg_type JOIN pg_class c ON typrelid = c.oid WHERE typname = 'deptest_t'; (1 row) RESET SESSION AUTHORIZATION; +CREATE DATABASE regressdeptestdb1 OWNER regress_dep_user1; REASSIGN OWNED BY regress_dep_user1 TO regress_dep_user2; \dt deptest List of relations @@ -148,6 +149,9 @@ owner of type deptest_range owner of table deptest2 owner of sequence ss1 owner of type deptest_t +owner of database regressdeptestdb1 DROP OWNED BY regress_dep_user2, regress_dep_user0; +-- DROP OWNED does not remove databases, so ... +DROP DATABASE regressdeptestdb1; DROP USER regress_dep_user2; DROP USER regress_dep_user0; diff --git a/src/test/regress/sql/dependency.sql b/src/test/regress/sql/dependency.sql index 8d74ed7122..06318abbe8 100644 --- a/src/test/regress/sql/dependency.sql +++ b/src/test/regress/sql/dependency.sql @@ -99,6 +99,9 @@ SELECT typowner = relowner FROM pg_type JOIN pg_class c ON typrelid = c.oid WHERE typname = 'deptest_t'; RESET SESSION AUTHORIZATION; + +CREATE DATABASE regressdeptestdb1 OWNER regress_dep_user1; + REASSIGN OWNED BY regress_dep_user1 TO regress_dep_user2; \dt deptest @@ -112,5 +115,7 @@ DROP USER regress_dep_user1; DROP USER regress_dep_user2; DROP OWNED BY regress_dep_user2, regress_dep_user0; +-- DROP OWNED does not remove databases, so ... +DROP DATABASE regressdeptestdb1; DROP USER regress_dep_user2; DROP USER regress_dep_user0; -- 2.34.1