Hi,
On Sat, Nov 09, 2024 at 01:43:13PM +0100, Peter Eisentraut wrote:
> On 31.10.24 15:26, Bertrand Drouvot wrote:
> > + address = get_object_address(objtype, lfirst(cell), &relation, lockmode,
> > false);
> > + Assert(relation == NULL);
> >
> > Worth to explain why we do expect relation to be NULL here? (the comment on
> > top
> > of get_object_address() says it all, but maybe a few words here could be
> > worth
> > it).
>
> There are several other callers with this pattern.
Right. And most of those places declare a Relation prior calling
get_object_address()
_only_ for a following assertion.
> Maybe it would be better to push the assertion into get_object_address(),
> something like
>
> Assert(!relation || relp)
>
> near the end.
That looks like a good idea to me, that would make the code cleaner and easier
to understand.
> Meaning, if you pass NULL for the relp argument, then you
> don't expect a relation. This is kind of what will happen now anyway,
> except with a segfault instead of an assertion.
Yeah, I like it.
So, something like the attached (provided as .txt file to no mess up the CF bot
entry related to this thread) could be applied before?
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
>From e0766ce3403eba54f28df140dd787b049d5eaf86 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot <bertranddrouvot...@gmail.com>
Date: Mon, 11 Nov 2024 07:07:02 +0000
Subject: [PATCH v1] Add an assertion in get_object_address()
Some places declared a Relation before calling get_object_address() only to
assert that the relation is NULL after the call.
The new assertion allows passing NULL as the relation argument at those places
making the code cleaner and easier to understand.
---
src/backend/catalog/objectaddress.c | 8 +++++---
src/backend/commands/alter.c | 15 ++++-----------
2 files changed, 9 insertions(+), 14 deletions(-)
48.8% src/backend/catalog/
51.1% src/backend/commands/
diff --git a/src/backend/catalog/objectaddress.c
b/src/backend/catalog/objectaddress.c
index 85a7b7e641..e089582285 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -896,8 +896,8 @@ static void getRelationIdentity(StringInfo buffer, Oid
relid, List **object,
*
* If the object is a relation or a child object of a relation (e.g. an
* attribute or constraint), the relation is also opened and *relp receives
- * the open relcache entry pointer; otherwise, *relp is set to NULL. This
- * is a bit grotty but it makes life simpler, since the caller will
+ * the open relcache entry pointer (if *relp is not passed as a NULL argument).
+ * This is a bit grotty but it makes life simpler, since the caller will
* typically need the relcache entry too. Caller must close the relcache
* entry when done with it. The relation is locked with the specified lockmode
* if the target object is the relation itself or an attribute, but for other
@@ -1204,8 +1204,10 @@ get_object_address(ObjectType objtype, Node *object,
old_address = address;
}
+ Assert(!relation || relp);
/* Return the object address and the relation. */
- *relp = relation;
+ if (relp)
+ *relp = relation;
return address;
}
diff --git a/src/backend/commands/alter.c b/src/backend/commands/alter.c
index 4f99ebb447..a45f3bb6b8 100644
--- a/src/backend/commands/alter.c
+++ b/src/backend/commands/alter.c
@@ -421,13 +421,11 @@ ExecRenameStmt(RenameStmt *stmt)
{
ObjectAddress address;
Relation catalog;
- Relation relation;
address = get_object_address(stmt->renameType,
stmt->object,
-
&relation,
+
NULL,
AccessExclusiveLock, false);
- Assert(relation == NULL);
catalog = table_open(address.classId,
RowExclusiveLock);
AlterObjectRename_internal(catalog,
@@ -482,8 +480,7 @@ ExecAlterObjectDependsStmt(AlterObjectDependsStmt *stmt,
ObjectAddress *refAddre
table_close(rel, NoLock);
refAddr = get_object_address(OBJECT_EXTENSION, (Node *) stmt->extname,
- &rel,
AccessExclusiveLock, false);
- Assert(rel == NULL);
+ NULL,
AccessExclusiveLock, false);
if (refAddress)
*refAddress = refAddr;
@@ -563,16 +560,14 @@ ExecAlterObjectSchemaStmt(AlterObjectSchemaStmt *stmt,
case OBJECT_TSTEMPLATE:
{
Relation catalog;
- Relation relation;
Oid classId;
Oid nspOid;
address = get_object_address(stmt->objectType,
stmt->object,
-
&relation,
+
NULL,
AccessExclusiveLock,
false);
- Assert(relation == NULL);
classId = address.classId;
catalog = table_open(classId, RowExclusiveLock);
nspOid =
LookupCreationNamespace(stmt->newschema);
@@ -876,15 +871,13 @@ ExecAlterOwnerStmt(AlterOwnerStmt *stmt)
case OBJECT_TSDICTIONARY:
case OBJECT_TSCONFIGURATION:
{
- Relation relation;
ObjectAddress address;
address = get_object_address(stmt->objectType,
stmt->object,
-
&relation,
+
NULL,
AccessExclusiveLock,
false);
- Assert(relation == NULL);
AlterObjectOwner_internal(address.classId,
address.objectId,
newowner);
--
2.34.1
