WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
Started by Alexander Kukushkinabout 1 year ago10 messages
Hello Hacker,
When Postgres is compiled with --enable-cassert I get subj when doing the
following:
postgres=# create user test;
CREATE ROLE
postgres=# alter database postgres owner to test;
ALTER DATABASE
postgres=# reassign owned by test to postgres;
WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
REASSIGN OWNED
It was discovered on 16.6, however the master shows the same behaviour.
I suspect it to be due to aac2c9b4fde889d13f859c233c2523345e72d32b.
Regards,
--
Alexander Kukushkin
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Mon, 9 Dec 2024 at 15:27, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
postgres=# reassign owned by test to postgres;
WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
REASSIGN OWNED
Hi!
Looks like PFA should fix that.
--
Best regards,
Kirill Reshke
Attachments:
v1-0001-Lock-tuple-for-inplace-update-when-moidfying.patchapplication/octet-stream; name=v1-0001-Lock-tuple-for-inplace-update-when-moidfying.patchDownload
From f70f8948019d2f92d9e7521a27a9078d170a1767 Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Mon, 9 Dec 2024 11:49:01 +0000
Subject: [PATCH v1] Lock tuple for inplace update when moidfying
---
src/backend/commands/alter.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index a45f3bb6b83..19258ed5ac2 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -991,6 +991,8 @@ AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId)
}
}
+ LockTuple(rel, &oldtup->t_self, InplaceUpdateTupleLock);
+
/* Build a modified tuple */
nattrs = RelationGetNumberOfAttributes(rel);
values = palloc0(nattrs * sizeof(Datum));
@@ -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);
--
2.34.1
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Mon, Dec 09, 2024 at 04:50:16PM +0500, Kirill Reshke wrote:
On Mon, 9 Dec 2024 at 15:27, Alexander Kukushkin <cyberdemn@gmail.com> wrote:
postgres=# reassign owned by test to postgres;
WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
REASSIGN OWNED
Thanks for the report.
--- a/src/backend/commands/alter.c +++ b/src/backend/commands/alter.c @@ -991,6 +991,8 @@ AlterObjectOwner_internal(Oid classId, Oid objectId, Oid new_ownerId) } }+ LockTuple(rel, &oldtup->t_self, InplaceUpdateTupleLock); + /* Build a modified tuple */
This silences the warning, but it doesn't generally satisfy the locking
protocol at README.tuplock section "Locking to write inplace-updated tables".
The specific problem is that this locks the tuple after copying it from shared
buffers. If getting oldtup from a syscache, use SearchSysCacheLocked1().
Otherwise, lock before systable_endscan() and before any copying, like
AlterDatabaseOwner() does.
A fix for $SUBJECT also warrants a test in database.sql.
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Mon, 9 Dec 2024 at 23:13, Noah Misch <noah@leadboat.com> wrote:
This silences the warning, but it doesn't generally satisfy the locking
protocol at README.tuplock section "Locking to write inplace-updated tables".
The specific problem is that this locks the tuple after copying it from shared
buffers. If getting oldtup from a syscache, use SearchSysCacheLocked1().
Otherwise, lock before systable_endscan() and before any copying, like
AlterDatabaseOwner() does.
Yes, I just copied LockTuple from AlterDatabaseOwner, but it turns out
that the problem is more complex than I first believed.
PFA v2. I tried to follow your suggestion.
The get_catalog_object_by_oid_extended method, which I added to v2,
acts similarly to get_catalog_object_by_oid but locks the tuple being
searched.
Do you think this design looks good?
A fix for $SUBJECT also warrants a test in database.sql.
Sure. I added one test case to dependency.sql (for DATABASE ownership
change), do we need more (other catalog classes)?
--
Best regards,
Kirill Reshke
Attachments:
v2-0001-When-making-dependency-changes-lock-the-tuple-for.patchapplication/octet-stream; name=v2-0001-When-making-dependency-changes-lock-the-tuple-for.patchDownload
From b912d36a62c64bd55ab6e0ef300951403910b9bf Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Mon, 9 Dec 2024 11:49:01 +0000
Subject: [PATCH v2] 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 <cyberdemn@gmail.com>
Reviewed-By: Noah Misch <noah@leadboat.com>
---
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 be7e4a5dd01..25dc59c18aa 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 a45f3bb6b83..910d1b14634 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 3a70d80e320..067e1662e4d 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 74d9ff2998d..9410e6a7654 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 deptestdb1 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 deptestdb1
DROP OWNED BY regress_dep_user2, regress_dep_user0;
+-- DROP OWNED does not remove databases, so ...
+DROP DATABASE deptestdb1;
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 8d74ed7122c..343a91ff0c5 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 deptestdb1 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 deptestdb1;
DROP USER regress_dep_user2;
DROP USER regress_dep_user0;
--
2.34.1
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Tue, 10 Dec 2024 at 10:45, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v2.
Also CF entry https://commitfest.postgresql.org/51/5430/ to get CI feedback.
--
Best regards,
Kirill Reshke
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Tue, 10 Dec 2024 at 10:47, Kirill Reshke <reshkekirill@gmail.com> wrote:
On Tue, 10 Dec 2024 at 10:45, Kirill Reshke <reshkekirill@gmail.com> wrote:
PFA v2.
Also CF entry https://commitfest.postgresql.org/51/5430/ to get CI feedback.
CI fails due to bad naming in the regression test.
The change is deptestdb1 -> regressdeptestdb1
PFA v3.
--
Best regards,
Kirill Reshke
Attachments:
v3-0001-When-making-dependency-changes-lock-the-tuple-for.patchapplication/octet-stream; name=v3-0001-When-making-dependency-changes-lock-the-tuple-for.patchDownload
From 0f9b29e5e2b339c3ac18d3cadfc039fdf4ff1a1f Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
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 <cyberdemn@gmail.com>
Reviewed-By: Noah Misch <noah@leadboat.com>
---
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
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Tue, 10 Dec 2024 at 14:14, Kirill Reshke <reshkekirill@gmail.com> wrote:
CI fails due to bad naming in the regression test.
The change is deptestdb1 -> regressdeptestdb1
I'm very sorry for spam.
PFAv4
regressdeptestdb1 -> regressiondeptestdb1
--
Best regards,
Kirill Reshke
Attachments:
v4-0001-When-making-dependency-changes-lock-the-tuple-for.patchapplication/octet-stream; name=v4-0001-When-making-dependency-changes-lock-the-tuple-for.patchDownload
From e1eb9f748f5f88461bcc1c24622141e6fbd6204a Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Mon, 9 Dec 2024 11:49:01 +0000
Subject: [PATCH v4] 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 <cyberdemn@gmail.com>
Reviewed-By: Noah Misch <noah@leadboat.com>
---
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..204907ca98 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 regressiondeptestdb1 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 regressiondeptestdb1
DROP OWNED BY regress_dep_user2, regress_dep_user0;
+-- DROP OWNED does not remove databases, so ...
+DROP DATABASE regressiondeptestdb1;
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..6b596ab207 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 regressiondeptestdb1 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 regressiondeptestdb1;
DROP USER regress_dep_user2;
DROP USER regress_dep_user0;
--
2.34.1
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Wed, Dec 11, 2024 at 11:37:49AM +0500, Kirill Reshke wrote:
PFAv4
regressdeptestdb1 -> regressiondeptestdb1
The reason I said databases.sql for the test is that CREATE DATABASE is
expensive. We currently have just one successful CREATE DATABASE in the
src/test/regress suite, and we shouldn't add more that reasonably could
instead harness the existing one. Would you do it that way?
Re: WARNING: missing lock on database "postgres" (OID 5) @ TID (0,4)
On Tue, 17 Dec 2024 at 04:43, Noah Misch <noah@leadboat.com> wrote:
The reason I said databases.sql for the test is that CREATE DATABASE is
expensive. We currently have just one successful CREATE DATABASE in the
src/test/regress suite, and we shouldn't add more that reasonably could
instead harness the existing one. Would you do it that way?
Sure, my bad, I missed this part of your message. PFA v5 with
database.sql changes.
--
Best regards,
Kirill Reshke
Attachments:
v5-0001-When-making-dependency-changes-lock-the-tuple-for.patchapplication/octet-stream; name=v5-0001-When-making-dependency-changes-lock-the-tuple-for.patchDownload
From 0579e8a112c51a384b920e7bb676a7d444e7ab29 Mon Sep 17 00:00:00 2001
From: reshke kirill <reshke@double.cloud>
Date: Mon, 9 Dec 2024 11:49:01 +0000
Subject: [PATCH v5] 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 <cyberdemn@gmail.com>
Reviewed-By: Noah Misch <noah@leadboat.com>
---
src/backend/catalog/objectaddress.c | 19 ++++++++++++++++++-
src/backend/commands/alter.c | 8 +++++++-
src/include/catalog/objectaddress.h | 1 +
src/test/regress/expected/database.out | 6 ++++++
src/test/regress/sql/database.sql | 7 +++++++
5 files changed, 39 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/database.out b/src/test/regress/expected/database.out
index 454db91ec0..3b1014b0e7 100644
--- a/src/test/regress/expected/database.out
+++ b/src/test/regress/expected/database.out
@@ -12,4 +12,10 @@ WHERE datname = 'regression_utf8';
-- load catcache entry, if nothing else does
ALTER DATABASE regression_utf8 RESET TABLESPACE;
ROLLBACK;
+CREATE ROLE regress_temp_role1;
+CREATE ROLE regress_temp_role2;
+ALTER DATABASE regression_utf8 OWNER TO regress_temp_role1;
+REASSIGN OWNED BY regress_temp_role1 TO regress_temp_role2;
DROP DATABASE regression_utf8;
+DROP ROLE regress_temp_role1;
+DROP ROLE regress_temp_role2;
diff --git a/src/test/regress/sql/database.sql b/src/test/regress/sql/database.sql
index 0367c0e37a..f8d2462c7d 100644
--- a/src/test/regress/sql/database.sql
+++ b/src/test/regress/sql/database.sql
@@ -14,4 +14,11 @@ WHERE datname = 'regression_utf8';
ALTER DATABASE regression_utf8 RESET TABLESPACE;
ROLLBACK;
+CREATE ROLE regress_temp_role1;
+CREATE ROLE regress_temp_role2;
+ALTER DATABASE regression_utf8 OWNER TO regress_temp_role1;
+REASSIGN OWNED BY regress_temp_role1 TO regress_temp_role2;
+
DROP DATABASE regression_utf8;
+DROP ROLE regress_temp_role1;
+DROP ROLE regress_temp_role2;
--
2.34.1