ObjectIdGetDatum() missing from SearchSysCache*() callers
Hi all,
While scanning the code, I have noticed that a couple of code paths
that do syscache lookups are passing down directly Oids rather than
Datums. I think that we'd better be consistent here, even if there is
no actual bug.
I have noticed 11 callers of SearchSysCache*() that pass down
an Oid instead of a Datum.
Thoughts or comments?
--
Michael
Attachments:
syscache-datums.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 67b743e251..eb2b8d84c3 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1330,7 +1330,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indcoloptions = (int2vector *) DatumGetPointer(colOptionDatum);
/* Fetch options of index if any */
- classTuple = SearchSysCache1(RELOID, oldIndexId);
+ classTuple = SearchSysCache1(RELOID, ObjectIdGetDatum(oldIndexId));
if (!HeapTupleIsValid(classTuple))
elog(ERROR, "cache lookup failed for relation %u", oldIndexId);
optionDatum = SysCacheGetAttr(RELOID, classTuple,
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4dc029f91f..727f151750 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10147,7 +10147,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel)
Oid deleteTriggerOid,
updateTriggerOid;
- tuple = SearchSysCache1(CONSTROID, constrOid);
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constrOid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for constraint %u", constrOid);
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
@@ -10353,7 +10353,7 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
Oid insertTriggerOid,
updateTriggerOid;
- tuple = SearchSysCache1(CONSTROID, parentConstrOid);
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parentConstrOid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for constraint %u",
parentConstrOid);
@@ -13723,7 +13723,7 @@ ATExecAlterColumnGenericOptions(Relation rel,
/* First, determine FDW validator associated to the foreign table. */
ftrel = table_open(ForeignTableRelationId, AccessShareLock);
- tuple = SearchSysCache1(FOREIGNTABLEREL, rel->rd_id);
+ tuple = SearchSysCache1(FOREIGNTABLEREL, ObjectIdGetDatum(rel->rd_id));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -16186,7 +16186,8 @@ ATExecGenericOptions(Relation rel, List *options)
ftrel = table_open(ForeignTableRelationId, RowExclusiveLock);
- tuple = SearchSysCacheCopy1(FOREIGNTABLEREL, rel->rd_id);
+ tuple = SearchSysCacheCopy1(FOREIGNTABLEREL,
+ ObjectIdGetDatum(rel->rd_id));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 6b42d4fc34..ce77a055e5 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1935,7 +1935,7 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
HeapTuple mrtup;
Form_pg_authid mrform;
- mrtup = SearchSysCache1(AUTHOID, memberid);
+ mrtup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(memberid));
if (!HeapTupleIsValid(mrtup))
elog(ERROR, "cache lookup failed for role %u", memberid);
mrform = (Form_pg_authid) GETSTRUCT(mrtup);
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7c5d9110fb..5436cc302d 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -4313,7 +4313,7 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
Datum datum;
PartitionBoundSpec *bspec;
- tuple = SearchSysCache1(RELOID, inhrelid);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(inhrelid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", inhrelid);
diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 7a2b5e57ff..65f3d5a5e6 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -183,7 +183,7 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached)
PartitionBoundSpec *boundspec = NULL;
/* Try fetching the tuple from the catcache, for speed. */
- tuple = SearchSysCache1(RELOID, inhrelid);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(inhrelid));
if (HeapTupleIsValid(tuple))
{
Datum datum;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 883e09393a..27eabb80ab 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5334,13 +5334,13 @@ get_rolespec_tuple(const RoleSpec *role)
case ROLESPEC_CURRENT_ROLE:
case ROLESPEC_CURRENT_USER:
- tuple = SearchSysCache1(AUTHOID, GetUserId());
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId()));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for role %u", GetUserId());
break;
case ROLESPEC_SESSION_USER:
- tuple = SearchSysCache1(AUTHOID, GetSessionUserId());
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetSessionUserId()));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for role %u", GetSessionUserId());
break;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index d3a973d86b..fcb2f45f62 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3283,7 +3283,7 @@ print_function_arguments(StringInfo buf, HeapTuple proctup,
HeapTuple aggtup;
Form_pg_aggregate agg;
- aggtup = SearchSysCache1(AGGFNOID, proc->oid);
+ aggtup = SearchSysCache1(AGGFNOID, ObjectIdGetDatum(proc->oid));
if (!HeapTupleIsValid(aggtup))
elog(ERROR, "cache lookup failed for aggregate %u",
proc->oid);
Hi,
I have noticed 11 callers of SearchSysCache*() that pass down
an Oid instead of a Datum.
Good catch.
I think that we'd better be consistent here, even if there is
no actual bug.
+1
--
Best regards,
Aleksander Alekseev
Hi
Regards,
Zhang Mingli
On Jul 17, 2023 at 19:10 +0800, Michael Paquier <michael@paquier.xyz>, wrote:
Hi all,
While scanning the code, I have noticed that a couple of code paths
that do syscache lookups are passing down directly Oids rather than
Datums. I think that we'd better be consistent here, even if there is
no actual bug.I have noticed 11 callers of SearchSysCache*() that pass down
an Oid instead of a Datum.Thoughts or comments?
--
Michael
LGTM, and there are two functions missed, in sequence_options
pgstuple = SearchSysCache1(SEQRELID, relid);
Shall we fix that too?
Hi,
Regards,
Zhang Mingli
On Jul 17, 2023 at 21:09 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
sequence_options
And inside pg_sequence_parameters:
pgstuple = SearchSysCache1(SEQRELID, relid);
Hi,
And inside pg_sequence_parameters:
pgstuple = SearchSysCache1(SEQRELID, relid);
Found another one in partcache.c:
```
/* Get pg_class.relpartbound */
tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
```
I can't be 100% sure but it looks like that's all of them. PFA the
updated patch v2.
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Pass-Datum-to-SearchSysCache-instead-of-Oid.patchapplication/octet-stream; name=v2-0001-Pass-Datum-to-SearchSysCache-instead-of-Oid.patchDownload
From 0cd1377c2dc1f3b95356ee4f08fcf387c6d91130 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Mon, 17 Jul 2023 15:33:44 +0300
Subject: [PATCH v2] Pass Datum to SearchSysCache*() instead of Oid
This is no actual bug but we'd better be consistent here.
Michael Paquier, reviewed by Zhang Mingli, Aleksander Alekseev
Discussion: https://postgr.es/m/ZLUhqsqQN1MOaxdw%40paquier.xyz
---
src/backend/catalog/index.c | 2 +-
src/backend/commands/sequence.c | 4 ++--
src/backend/commands/tablecmds.c | 9 +++++----
src/backend/commands/user.c | 2 +-
src/backend/partitioning/partbounds.c | 2 +-
src/backend/partitioning/partdesc.c | 2 +-
src/backend/utils/adt/acl.c | 4 ++--
src/backend/utils/adt/ruleutils.c | 2 +-
src/backend/utils/cache/partcache.c | 3 ++-
9 files changed, 16 insertions(+), 14 deletions(-)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 67b743e251..eb2b8d84c3 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1330,7 +1330,7 @@ index_concurrently_create_copy(Relation heapRelation, Oid oldIndexId,
indcoloptions = (int2vector *) DatumGetPointer(colOptionDatum);
/* Fetch options of index if any */
- classTuple = SearchSysCache1(RELOID, oldIndexId);
+ classTuple = SearchSysCache1(RELOID, ObjectIdGetDatum(oldIndexId));
if (!HeapTupleIsValid(classTuple))
elog(ERROR, "cache lookup failed for relation %u", oldIndexId);
optionDatum = SysCacheGetAttr(RELOID, classTuple,
diff --git a/src/backend/commands/sequence.c b/src/backend/commands/sequence.c
index ef01449678..fc4f77e787 100644
--- a/src/backend/commands/sequence.c
+++ b/src/backend/commands/sequence.c
@@ -1718,7 +1718,7 @@ sequence_options(Oid relid)
Form_pg_sequence pgsform;
List *options = NIL;
- pgstuple = SearchSysCache1(SEQRELID, relid);
+ pgstuple = SearchSysCache1(SEQRELID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(pgstuple))
elog(ERROR, "cache lookup failed for sequence %u", relid);
pgsform = (Form_pg_sequence) GETSTRUCT(pgstuple);
@@ -1766,7 +1766,7 @@ pg_sequence_parameters(PG_FUNCTION_ARGS)
memset(isnull, 0, sizeof(isnull));
- pgstuple = SearchSysCache1(SEQRELID, relid);
+ pgstuple = SearchSysCache1(SEQRELID, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(pgstuple))
elog(ERROR, "cache lookup failed for sequence %u", relid);
pgsform = (Form_pg_sequence) GETSTRUCT(pgstuple);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 4dc029f91f..727f151750 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -10147,7 +10147,7 @@ CloneFkReferenced(Relation parentRel, Relation partitionRel)
Oid deleteTriggerOid,
updateTriggerOid;
- tuple = SearchSysCache1(CONSTROID, constrOid);
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(constrOid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for constraint %u", constrOid);
constrForm = (Form_pg_constraint) GETSTRUCT(tuple);
@@ -10353,7 +10353,7 @@ CloneFkReferencing(List **wqueue, Relation parentRel, Relation partRel)
Oid insertTriggerOid,
updateTriggerOid;
- tuple = SearchSysCache1(CONSTROID, parentConstrOid);
+ tuple = SearchSysCache1(CONSTROID, ObjectIdGetDatum(parentConstrOid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for constraint %u",
parentConstrOid);
@@ -13723,7 +13723,7 @@ ATExecAlterColumnGenericOptions(Relation rel,
/* First, determine FDW validator associated to the foreign table. */
ftrel = table_open(ForeignTableRelationId, AccessShareLock);
- tuple = SearchSysCache1(FOREIGNTABLEREL, rel->rd_id);
+ tuple = SearchSysCache1(FOREIGNTABLEREL, ObjectIdGetDatum(rel->rd_id));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
@@ -16186,7 +16186,8 @@ ATExecGenericOptions(Relation rel, List *options)
ftrel = table_open(ForeignTableRelationId, RowExclusiveLock);
- tuple = SearchSysCacheCopy1(FOREIGNTABLEREL, rel->rd_id);
+ tuple = SearchSysCacheCopy1(FOREIGNTABLEREL,
+ ObjectIdGetDatum(rel->rd_id));
if (!HeapTupleIsValid(tuple))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_OBJECT),
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index 6b42d4fc34..ce77a055e5 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -1935,7 +1935,7 @@ AddRoleMems(Oid currentUserId, const char *rolename, Oid roleid,
HeapTuple mrtup;
Form_pg_authid mrform;
- mrtup = SearchSysCache1(AUTHOID, memberid);
+ mrtup = SearchSysCache1(AUTHOID, ObjectIdGetDatum(memberid));
if (!HeapTupleIsValid(mrtup))
elog(ERROR, "cache lookup failed for role %u", memberid);
mrform = (Form_pg_authid) GETSTRUCT(mrtup);
diff --git a/src/backend/partitioning/partbounds.c b/src/backend/partitioning/partbounds.c
index 7c5d9110fb..5436cc302d 100644
--- a/src/backend/partitioning/partbounds.c
+++ b/src/backend/partitioning/partbounds.c
@@ -4313,7 +4313,7 @@ get_qual_for_range(Relation parent, PartitionBoundSpec *spec,
Datum datum;
PartitionBoundSpec *bspec;
- tuple = SearchSysCache1(RELOID, inhrelid);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(inhrelid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u", inhrelid);
diff --git a/src/backend/partitioning/partdesc.c b/src/backend/partitioning/partdesc.c
index 7a2b5e57ff..65f3d5a5e6 100644
--- a/src/backend/partitioning/partdesc.c
+++ b/src/backend/partitioning/partdesc.c
@@ -183,7 +183,7 @@ RelationBuildPartitionDesc(Relation rel, bool omit_detached)
PartitionBoundSpec *boundspec = NULL;
/* Try fetching the tuple from the catcache, for speed. */
- tuple = SearchSysCache1(RELOID, inhrelid);
+ tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(inhrelid));
if (HeapTupleIsValid(tuple))
{
Datum datum;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 883e09393a..27eabb80ab 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -5334,13 +5334,13 @@ get_rolespec_tuple(const RoleSpec *role)
case ROLESPEC_CURRENT_ROLE:
case ROLESPEC_CURRENT_USER:
- tuple = SearchSysCache1(AUTHOID, GetUserId());
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetUserId()));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for role %u", GetUserId());
break;
case ROLESPEC_SESSION_USER:
- tuple = SearchSysCache1(AUTHOID, GetSessionUserId());
+ tuple = SearchSysCache1(AUTHOID, ObjectIdGetDatum(GetSessionUserId()));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for role %u", GetSessionUserId());
break;
diff --git a/src/backend/utils/adt/ruleutils.c b/src/backend/utils/adt/ruleutils.c
index d3a973d86b..fcb2f45f62 100644
--- a/src/backend/utils/adt/ruleutils.c
+++ b/src/backend/utils/adt/ruleutils.c
@@ -3283,7 +3283,7 @@ print_function_arguments(StringInfo buf, HeapTuple proctup,
HeapTuple aggtup;
Form_pg_aggregate agg;
- aggtup = SearchSysCache1(AGGFNOID, proc->oid);
+ aggtup = SearchSysCache1(AGGFNOID, ObjectIdGetDatum(proc->oid));
if (!HeapTupleIsValid(aggtup))
elog(ERROR, "cache lookup failed for aggregate %u",
proc->oid);
diff --git a/src/backend/utils/cache/partcache.c b/src/backend/utils/cache/partcache.c
index 5f3516ad0c..c6c0d5a00f 100644
--- a/src/backend/utils/cache/partcache.c
+++ b/src/backend/utils/cache/partcache.c
@@ -365,7 +365,8 @@ generate_partition_qual(Relation rel)
parent = relation_open(parentrelid, AccessShareLock);
/* Get pg_class.relpartbound */
- tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
+ tuple = SearchSysCache1(RELOID,
+ ObjectIdGetDatum(RelationGetRelid(rel)));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for relation %u",
RelationGetRelid(rel));
--
2.41.0
Hi,
And inside pg_sequence_parameters:
pgstuple = SearchSysCache1(SEQRELID, relid);Found another one in partcache.c:
```
/* Get pg_class.relpartbound */
tuple = SearchSysCache1(RELOID, RelationGetRelid(rel));
```I can't be 100% sure but it looks like that's all of them. PFA the
updated patch v2.
Added a CF entry, just in case:
https://commitfest.postgresql.org/44/4448/
--
Best regards,
Aleksander Alekseev
On Mon, Jul 17, 2023 at 05:33:42PM +0300, Aleksander Alekseev wrote:
I can't be 100% sure but it looks like that's all of them. PFA the
updated patch v2.
Thanks. Yes, this stuff is easy to miss. I was just grepping for a
few patterns and missed these two.
--
Michael
On Tue, Jul 18, 2023 at 07:27:02AM +0900, Michael Paquier wrote:
On Mon, Jul 17, 2023 at 05:33:42PM +0300, Aleksander Alekseev wrote:
I can't be 100% sure but it looks like that's all of them. PFA the
updated patch v2.Thanks. Yes, this stuff is easy to miss. I was just grepping for a
few patterns and missed these two.
Spotted a few more of these things after a second lookup.
One for subscriptions:
src/backend/commands/alter.c:
if (SearchSysCacheExists2(SUBSCRIPTIONNAME, MyDatabaseId,
And two for transforms:
src/backend/utils/cache/lsyscache.c:
tup = SearchSysCache2(TRFTYPELANG, typid, langid);
src/backend/utils/cache/lsyscache.c:
tup = SearchSysCache2(TRFTYPELANG, typid, langid);
And applied the whole. Thanks for looking and spot more of these
inconsistencies!
--
Michael