Thread: 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.

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

On Sun, Dec 11, 2022 at 04:20:17PM +0900, Michael Paquier wrote:
> 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.

Applied as of eae7fe4.
--
Michael