diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c index 3ce6c09b44d..17461a91890 100644 --- a/src/backend/catalog/aclchk.c +++ b/src/backend/catalog/aclchk.c @@ -4103,9 +4103,14 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid) if (superuser_arg(roleid)) return true; + /* For large objects, the catalog to consult is pg_largeobject_metadata */ + if (classid == LargeObjectRelationId) + classid = LargeObjectMetadataRelationId; + cacheid = get_object_catcache_oid(classid); if (cacheid != -1) { + /* we can get the object's tuple from the syscache */ HeapTuple tuple; tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid)); @@ -4122,7 +4127,6 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid) else { /* for catalogs without an appropriate syscache */ - Relation rel; ScanKeyData entry[1]; SysScanDesc scan; @@ -4442,9 +4446,9 @@ recordExtObjInitPriv(Oid objoid, Oid classoid) ReleaseSysCache(tuple); } - /* pg_largeobject_metadata */ - else if (classoid == LargeObjectMetadataRelationId) + else if (classoid == LargeObjectRelationId) { + /* For large objects, we must consult pg_largeobject_metadata */ Datum aclDatum; bool isNull; HeapTuple tuple; diff --git a/src/backend/catalog/pg_shdepend.c b/src/backend/catalog/pg_shdepend.c index 8f09c632f96..f9580fc8c4e 100644 --- a/src/backend/catalog/pg_shdepend.c +++ b/src/backend/catalog/pg_shdepend.c @@ -1614,20 +1614,9 @@ shdepReassignOwned(List *roleids, Oid newrole) case DatabaseRelationId: case TSConfigRelationId: case TSDictionaryRelationId: - { - Oid classId = sdepForm->classid; - Relation catalog; - - if (classId == LargeObjectRelationId) - classId = LargeObjectMetadataRelationId; - - catalog = table_open(classId, RowExclusiveLock); - - AlterObjectOwner_internal(catalog, sdepForm->objid, - newrole); - - table_close(catalog, NoLock); - } + AlterObjectOwner_internal(sdepForm->classid, + sdepForm->objid, + newrole); break; default: diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c index ff8d003876f..e9a344d03e3 100644 --- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -918,9 +918,7 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt) case OBJECT_TSDICTIONARY: case OBJECT_TSCONFIGURATION: { - Relation catalog; Relation relation; - Oid classId; ObjectAddress address; address = get_object_address(stmt->objectType, @@ -929,20 +927,9 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt) AccessExclusiveLock, false); Assert(relation == NULL); - classId = address.classId; - /* - * XXX - get_object_address returns Oid of pg_largeobject - * catalog for OBJECT_LARGEOBJECT because of historical - * reasons. Fix up it here. - */ - if (classId == LargeObjectRelationId) - classId = LargeObjectMetadataRelationId; - - catalog = table_open(classId, RowExclusiveLock); - - AlterObjectOwner_internal(catalog, address.objectId, newowner); - table_close(catalog, RowExclusiveLock); + AlterObjectOwner_internal(address.classId, address.objectId, + newowner); return address; } @@ -960,25 +947,32 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt) * cases (won't work for tables, nor other cases where we need to do more than * change the ownership column of a single catalog entry). * - * rel: catalog relation containing object (RowExclusiveLock'd by caller) + * classId: OID of catalog containing object * objectId: OID of object to change the ownership of * new_ownerId: OID of new object owner + * + * This will work on large objects, but we have to beware of the fact that + * classId isn't the OID of the catalog to modify in that case. */ void -AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId) +AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId) { - Oid classId = RelationGetRelid(rel); - AttrNumber Anum_oid = get_object_attnum_oid(classId); - AttrNumber Anum_owner = get_object_attnum_owner(classId); - AttrNumber Anum_namespace = get_object_attnum_namespace(classId); - AttrNumber Anum_acl = get_object_attnum_acl(classId); - AttrNumber Anum_name = get_object_attnum_name(classId); + /* For large objects, the catalog to modify is pg_largeobject_metadata */ + Oid catalogId = (classId == LargeObjectRelationId) ? LargeObjectMetadataRelationId : classId; + AttrNumber Anum_oid = get_object_attnum_oid(catalogId); + AttrNumber Anum_owner = get_object_attnum_owner(catalogId); + AttrNumber Anum_namespace = get_object_attnum_namespace(catalogId); + AttrNumber Anum_acl = get_object_attnum_acl(catalogId); + AttrNumber Anum_name = get_object_attnum_name(catalogId); + Relation rel; HeapTuple oldtup; Datum datum; bool isnull; Oid old_ownerId; Oid namespaceId = InvalidOid; + rel = table_open(catalogId, RowExclusiveLock); + oldtup = get_catalog_object_by_oid(rel, Anum_oid, objectId); if (oldtup == NULL) elog(ERROR, "cache lookup failed for object %u of catalog \"%s\"", @@ -1026,7 +1020,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId) snprintf(namebuf, sizeof(namebuf), "%u", objectId); objname = namebuf; } - aclcheck_error(ACLCHECK_NOT_OWNER, get_object_type(classId, objectId), + aclcheck_error(ACLCHECK_NOT_OWNER, + get_object_type(catalogId, objectId), objname); } /* Must be able to become new owner */ @@ -1079,8 +1074,6 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId) CatalogTupleUpdate(rel, &newtup->t_self, newtup); /* Update owner dependency reference */ - if (classId == LargeObjectMetadataRelationId) - classId = LargeObjectRelationId; changeDependencyOnOwner(classId, objectId, new_ownerId); /* Release memory */ @@ -1089,5 +1082,8 @@ AlterObjectOwner_internal(Relation rel, Oid objectId, Oid new_ownerId) pfree(replaces); } + /* Note the post-alter hook gets classId not catalogId */ InvokeObjectPostAlterHook(classId, objectId, 0); + + table_close(rel, RowExclusiveLock); } diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c index d189044a4fc..230c6575320 100644 --- a/src/backend/libpq/be-fsstubs.c +++ b/src/backend/libpq/be-fsstubs.c @@ -43,7 +43,7 @@ #include #include "access/xact.h" -#include "catalog/pg_largeobject_metadata.h" +#include "catalog/pg_largeobject.h" #include "libpq/be-fsstubs.h" #include "libpq/libpq-fs.h" #include "miscadmin.h" @@ -323,7 +323,7 @@ be_lo_unlink(PG_FUNCTION_ARGS) * relevant FDs. */ if (!lo_compat_privileges && - !object_ownercheck(LargeObjectMetadataRelationId, lobjId, GetUserId())) + !object_ownercheck(LargeObjectRelationId, lobjId, GetUserId())) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), errmsg("must be owner of large object %u", lobjId))); diff --git a/src/backend/storage/large_object/inv_api.c b/src/backend/storage/large_object/inv_api.c index cc9c335a92d..a35f1dfeb9f 100644 --- a/src/backend/storage/large_object/inv_api.c +++ b/src/backend/storage/large_object/inv_api.c @@ -221,11 +221,10 @@ inv_create(Oid lobjId) /* * dependency on the owner of largeobject * - * The reason why we use LargeObjectRelationId instead of - * LargeObjectMetadataRelationId here is to provide backward compatibility - * to the applications which utilize a knowledge about internal layout of - * system catalogs. OID of pg_largeobject_metadata and loid of - * pg_largeobject are same value, so there are no actual differences here. + * Note that LO dependencies are recorded using classId + * LargeObjectRelationId for backwards-compatibility reasons. Using + * LargeObjectMetadataRelationId instead would simplify matters for the + * backend, but it'd complicate pg_dump and possibly break other clients. */ recordDependencyOnOwner(LargeObjectRelationId, lobjId_new, GetUserId()); diff --git a/src/include/commands/alter.h b/src/include/commands/alter.h index ae79968fadd..5ddeaba8f0d 100644 --- a/src/include/commands/alter.h +++ b/src/include/commands/alter.h @@ -17,7 +17,6 @@ #include "catalog/dependency.h" #include "catalog/objectaddress.h" #include "nodes/parsenodes.h" -#include "utils/relcache.h" extern ObjectAddress ExecRenameStmt(RenameStmt *stmt); @@ -29,7 +28,7 @@ extern Oid AlterObjectNamespace_oid(Oid classId, Oid objid, Oid nspOid, ObjectAddresses *objsMoved); extern ObjectAddress ExecAlterOwnerStmt(AlterOwnerStmt *stmt); -extern void AlterObjectOwner_internal(Relation rel, Oid objectId, +extern void AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId); #endif /* ALTER_H */ diff --git a/src/test/regress/expected/largeobject.out b/src/test/regress/expected/largeobject.out index bdcede6728e..4921dd79aee 100644 --- a/src/test/regress/expected/largeobject.out +++ b/src/test/regress/expected/largeobject.out @@ -6,7 +6,7 @@ \getenv abs_builddir PG_ABS_BUILDDIR -- ensure consistent test output regardless of the default bytea format SET bytea_output TO escape; --- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT +-- Test ALTER LARGE OBJECT OWNER CREATE ROLE regress_lo_user; SELECT lo_create(42); lo_create @@ -15,8 +15,11 @@ SELECT lo_create(42); (1 row) ALTER LARGE OBJECT 42 OWNER TO regress_lo_user; +-- Test GRANT, COMMENT as non-superuser +SET SESSION AUTHORIZATION regress_lo_user; GRANT SELECT ON LARGE OBJECT 42 TO public; COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer'; +RESET SESSION AUTHORIZATION; -- Test psql's \lo_list et al (we assume no other LOs exist yet) \lo_list Large objects diff --git a/src/test/regress/expected/largeobject_1.out b/src/test/regress/expected/largeobject_1.out index d700910c359..7172ddb39bb 100644 --- a/src/test/regress/expected/largeobject_1.out +++ b/src/test/regress/expected/largeobject_1.out @@ -6,7 +6,7 @@ \getenv abs_builddir PG_ABS_BUILDDIR -- ensure consistent test output regardless of the default bytea format SET bytea_output TO escape; --- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT +-- Test ALTER LARGE OBJECT OWNER CREATE ROLE regress_lo_user; SELECT lo_create(42); lo_create @@ -15,8 +15,11 @@ SELECT lo_create(42); (1 row) ALTER LARGE OBJECT 42 OWNER TO regress_lo_user; +-- Test GRANT, COMMENT as non-superuser +SET SESSION AUTHORIZATION regress_lo_user; GRANT SELECT ON LARGE OBJECT 42 TO public; COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer'; +RESET SESSION AUTHORIZATION; -- Test psql's \lo_list et al (we assume no other LOs exist yet) \lo_list Large objects diff --git a/src/test/regress/sql/largeobject.sql b/src/test/regress/sql/largeobject.sql index 800e4fcc6a2..a4aee02e3a4 100644 --- a/src/test/regress/sql/largeobject.sql +++ b/src/test/regress/sql/largeobject.sql @@ -9,13 +9,19 @@ -- ensure consistent test output regardless of the default bytea format SET bytea_output TO escape; --- Test ALTER LARGE OBJECT OWNER, GRANT, COMMENT +-- Test ALTER LARGE OBJECT OWNER CREATE ROLE regress_lo_user; SELECT lo_create(42); ALTER LARGE OBJECT 42 OWNER TO regress_lo_user; + +-- Test GRANT, COMMENT as non-superuser +SET SESSION AUTHORIZATION regress_lo_user; + GRANT SELECT ON LARGE OBJECT 42 TO public; COMMENT ON LARGE OBJECT 42 IS 'the ultimate answer'; +RESET SESSION AUTHORIZATION; + -- Test psql's \lo_list et al (we assume no other LOs exist yet) \lo_list \lo_list+