One-off with syscache ID in get_catalog_object_by_oid_extended()

Started by Michael Paquierabout 2 months ago3 messageshackers
Jump to latest
#1Michael Paquier
michael@paquier.xyz

Hi all,

While reviewing the syscache code, I have bumped into the following
funny bit in objectaddress.c:
if (oidCacheId > 0)
{
if (locktup)
tuple = SearchSysCacheLockedCopy1(oidCacheId,
ObjectIdGetDatum(objectId));
else
tuple = SearchSysCacheCopy1(oidCacheId,
ObjectIdGetDatum(objectId));
if (!HeapTupleIsValid(tuple)) /* should not happen */
return NULL;
}

This is wrong, because SysCacheIdentifier starts at 0. This has no
consequence currently, because the first value in the enum is
AGGFNOID, something that we don't rely on for object type lookups.
Even if this code had the idea to use an ID of 0, the logic would just
get back to a systable lookup, that would still work, that's just less
efficient.

Simple patch attached, planned for a backpatch quickly as I am playing
with a different patch that reworks a bit this code.

Regards,
--
Michael

Attachments:

cacheid-oneoff.patchtext/plain; charset=us-asciiDownload+1-1
#2Michael Paquier
michael@paquier.xyz
In reply to: Michael Paquier (#1)
Re: One-off with syscache ID in get_catalog_object_by_oid_extended()

On Wed, Feb 18, 2026 at 07:30:21AM +0900, Michael Paquier wrote:

Simple patch attached, planned for a backpatch quickly as I am playing
with a different patch that reworks a bit this code.

Err, actually, with next week's release planned next week in mind,
I'll fix that only on HEAD.
--
Michael

#3Kirill Reshke
reshkekirill@gmail.com
In reply to: Michael Paquier (#1)
Re: One-off with syscache ID in get_catalog_object_by_oid_extended()

On Wed, 18 Feb 2026 at 03:30, Michael Paquier <michael@paquier.xyz> wrote:

Hi all,

While reviewing the syscache code, I have bumped into the following
funny bit in objectaddress.c:
if (oidCacheId > 0)
{
if (locktup)
tuple = SearchSysCacheLockedCopy1(oidCacheId,
ObjectIdGetDatum(objectId));
else
tuple = SearchSysCacheCopy1(oidCacheId,
ObjectIdGetDatum(objectId));
if (!HeapTupleIsValid(tuple)) /* should not happen */
return NULL;
}

This is wrong, because SysCacheIdentifier starts at 0. This has no
consequence currently, because the first value in the enum is
AGGFNOID, something that we don't rely on for object type lookups.
Even if this code had the idea to use an ID of 0, the logic would just
get back to a systable lookup, that would still work, that's just less
efficient.

As I can see, this if-statement was introduced in 994c36e, at which
point it was already wrong (AGGFNOID was present and equal to 0 back
then).

I agree with analysis, SearchSysCache function does this check:
```
if (cacheId < 0 || cacheId >= SysCacheSize ||
!PointerIsValid(SysCache[cacheId]))
elog(ERROR, "invalid cache ID: %d", cacheId);
```

Simple patch attached, planned for a backpatch quickly as I am playing
with a different patch that reworks a bit this code.

LGTM

Regards,
--
Michael

--
Best regards,
Kirill Reshke