GetNewObjectId question
While browsing through varsup.c, I noticed this comment on GetNewObjectId:
* Hence, this routine should generally not be used directly. The only direct
* callers should be GetNewOidWithIndex() and GetNewRelFileNumber() in
* catalog/catalog.c.
But AddRoleMems in user.c appears to also call the function directly:
/* get an OID for the new row and insert it */
objectId = GetNewObjectId();
new_record[Anum_pg_auth_members_oid - 1] = objectId;
tuple = heap_form_tuple(pg_authmem_dsc,
new_record, new_record_nulls);
CatalogTupleInsert(pg_authmem_rel, tuple);
I'm not sure if that call is right, but this seems inconsistent.
Should that caller be using GetNewOidWithIndex instead? Or should the
comment be updated?
Thanks,
Maciek
Maciek Sakrejda <m.sakrejda@gmail.com> writes:
While browsing through varsup.c, I noticed this comment on GetNewObjectId:
* Hence, this routine should generally not be used directly. The only direct
* callers should be GetNewOidWithIndex() and GetNewRelFileNumber() in
* catalog/catalog.c.
But AddRoleMems in user.c appears to also call the function directly:
/* get an OID for the new row and insert it */
objectId = GetNewObjectId();
Yeah, that looks like somebody didn't read the memo.
Want to submit a patch?
regards, tom lane
On Sat, Dec 10, 2022 at 07:11:13PM -0500, Tom Lane wrote:
Yeah, that looks like somebody didn't read the memo.
Want to submit a patch?
The comment has been added in e3ce2de but the call originates from
6566133, so that's a HEAD-only issue.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Sat, Dec 10, 2022 at 07:11:13PM -0500, Tom Lane wrote:
Yeah, that looks like somebody didn't read the memo.
Want to submit a patch?
The comment has been added in e3ce2de but the call originates from
6566133, so that's a HEAD-only issue.
Ah, good that the bug hasn't made it to a released version yet.
But that comment is *way* older than e3ce2de; it's been touched
a couple of times, but it dates to 721e53785 AFAICS.
regards, tom lane
Sure. My C is pretty limited, but I think it's just the attached? I
patterned the usage on the way this is done in CreateRole. It passes
check-world here.
Attachments:
0001-Replace-GetNewObjectId-call-with-GetNewOidWithIndex.patchtext/x-patch; charset=US-ASCII; name=0001-Replace-GetNewObjectId-call-with-GetNewOidWithIndex.patchDownload
From c7cae5d3e8d179505f26851f1241436a3748f9a8 Mon Sep 17 00:00:00 2001
From: Maciek Sakrejda <m.sakrejda@gmail.com>
Date: Sat, 10 Dec 2022 22:51:02 -0800
Subject: [PATCH] Replace GetNewObjectId call with GetNewOidWithIndex
GetNewObjectId is not intended to be used directly in most situations,
since it can lead to duplicate OIDs unless that is guarded against.
---
src/backend/commands/user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 8b6543edee..0686807fa0 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1850,7 +1850,8 @@ AddRoleMems(const char *rolename, Oid roleid,
}
/* get an OID for the new row and insert it */
- objectId = GetNewObjectId();
+ objectId = GetNewOidWithIndex(pg_authmem_rel, AuthMemOidIndexId,
+ Anum_pg_auth_members_oid);
new_record[Anum_pg_auth_members_oid - 1] = objectId;
tuple = heap_form_tuple(pg_authmem_dsc,
new_record, new_record_nulls);
--
2.25.1
On Sat, Dec 10, 2022 at 11:03:38PM -0800, Maciek Sakrejda wrote:
Sure. My C is pretty limited, but I think it's just the attached? I
patterned the usage on the way this is done in CreateRole. It passes
check-world here.
Looks OK seen from here. Thanks for the patch! I don't have much
freshness and time today, but I can get back to this thread tomorrow.
--
Michael