On 20.12.24 12:47, Peter Eisentraut wrote:
In aclchk.c, there are a few error messages that use ereport() but it
seems like they should be internal error messages. Moreover, they are
using get_object_class_descr(), which is only meant for internal errors.
(For example, it does not have translation support.) I dug through
this and it seems like these errors are indeed not or no longer user-
facing, so we can downgrade them to internal. See commit messages in
the attached patches for further explanations.
There was a mistake in the second patch, I had missed some other callers
that I have fixed up here. Amended patch set attached.
From b9a2945b367bc32f1a1dda421a72e176fecacdda Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri, 20 Dec 2024 10:10:19 +0100
Subject: [PATCH v2 1/2] Downgrade errors in object_ownercheck() to internal
The "does not exist" errors in object_ownership() were written as
ereport(), suggesting that they are user-facing. But no code path
except one can reach this function without first checking that the
object exists. If this were actually a user-facing error message,
then there would be some problems: get_object_class_descr() is meant
to be for internal errors only and does not support translation.
The one case that can reach this without first checking the object
existence is from be_lo_unlink(). (This makes some sense since large
objects are referred to by their OID directly.) In this one case, we
can add a line of code to check the object existence explicitly,
consistent with other LO code.
For the rest, downgrade the error messages to elog()s. The new
message wordings are the same as in DropObjectById().
---
src/backend/catalog/aclchk.c | 10 ++++------
src/backend/libpq/be-fsstubs.c | 5 +++++
2 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index aaf96a965e4..840122dca44 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -4082,9 +4082,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = SearchSysCache1(cacheid, ObjectIdGetDatum(objectid));
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not
exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
tuple,
@@ -4113,9 +4112,8 @@ object_ownercheck(Oid classid, Oid objectid, Oid roleid)
tuple = systable_getnext(scan);
if (!HeapTupleIsValid(tuple))
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not
exist", get_object_class_descr(classid), objectid)));
+ elog(ERROR, "could not find tuple for %s %u",
+ get_object_class_descr(classid), objectid);
ownerId = DatumGetObjectId(heap_getattr(tuple,
get_object_attnum_owner(classid),
diff --git a/src/backend/libpq/be-fsstubs.c b/src/backend/libpq/be-fsstubs.c
index 27d317dfdc0..66bec869cbc 100644
--- a/src/backend/libpq/be-fsstubs.c
+++ b/src/backend/libpq/be-fsstubs.c
@@ -317,6 +317,11 @@ be_lo_unlink(PG_FUNCTION_ARGS)
PreventCommandIfReadOnly("lo_unlink()");
+ if (!LargeObjectExists(lobjId))
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("large object %u does not exist",
lobjId)));
+
/*
* Must be owner of the large object. It would be cleaner to check this
* in inv_drop(), but we want to throw the error before not after
closing
base-commit: 8ac0021b6f10928a46b7f3d1b25bc21c0ac7f8c5
--
2.47.1
From b68de2c26fac1d11c52dcfa391992454b65d7f52 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Fri, 20 Dec 2024 14:45:28 +0100
Subject: [PATCH v2 2/2] Downgrade error in object_aclmask_ext() to internal
The "does not exist" error in object_aclmask_ext() was written as
ereport(), suggesting that it is user-facing. This is problematic:
get_object_class_descr() is meant to be for internal errors only and
does not support translation.
For the has_xxx_privilege functions, the error has not been
user-facing since commit 403ac226ddd. The remaining users are
pg_database_size() and pg_tablespace_size(). The call stack here is
pretty deep and this dependency is not obvious. Here we can put in an
explicit existence check with a bespoke error message early in the
function.
Then we can downgrade the error in object_aclmask_ext() to a normal
"cache lookup failed" internal error.
---
src/backend/catalog/aclchk.c | 10 ++--------
src/backend/utils/adt/dbsize.c | 10 ++++++++++
2 files changed, 12 insertions(+), 8 deletions(-)
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 840122dca44..25b15027447 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -3004,10 +3004,6 @@ pg_aclmask(ObjectType objtype, Oid object_oid,
AttrNumber attnum, Oid roleid,
* Exported routines for examining a user's privileges for various objects
*
* See aclmask() for a description of the common API for these functions.
- *
- * Note: we give lookup failure the full ereport treatment because the
- * has_xxx_privilege() family of functions allow users to pass any random
- * OID to these functions.
* ****************************************************************
*/
@@ -3074,10 +3070,8 @@ object_aclmask_ext(Oid classid, Oid objectid, Oid roleid,
return 0;
}
else
- ereport(ERROR,
- (errcode(ERRCODE_UNDEFINED_OBJECT),
- errmsg("%s with OID %u does not exist",
-
get_object_class_descr(classid), objectid)));
+ elog(ERROR, "cache lookup failed for %s %u",
+ get_object_class_descr(classid), objectid);
}
ownerId = DatumGetObjectId(SysCacheGetAttrNotNull(cacheid,
diff --git a/src/backend/utils/adt/dbsize.c b/src/backend/utils/adt/dbsize.c
index e63e99c1416..e6f8900f107 100644
--- a/src/backend/utils/adt/dbsize.c
+++ b/src/backend/utils/adt/dbsize.c
@@ -170,6 +170,11 @@ pg_database_size_oid(PG_FUNCTION_ARGS)
Oid dbOid = PG_GETARG_OID(0);
int64 size;
+ if (!SearchSysCacheExists1(DATABASEOID, ObjectIdGetDatum(dbOid)))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("database with OID %u does not exist",
dbOid));
+
size = calculate_database_size(dbOid);
if (size == 0)
@@ -274,6 +279,11 @@ pg_tablespace_size_oid(PG_FUNCTION_ARGS)
Oid tblspcOid = PG_GETARG_OID(0);
int64 size;
+ if (!SearchSysCacheExists1(TABLESPACEOID, ObjectIdGetDatum(tblspcOid)))
+ ereport(ERROR,
+ errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("tablespace with OID %u does not exist",
tblspcOid));
+
size = calculate_tablespace_size(tblspcOid);
if (size < 0)
--
2.47.1
