downgrade some aclchk.c errors to internal

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.

On 2024-Dec-20, Peter Eisentraut wrote:

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.

LGTM.

Subject: [PATCH v2 1/2] Downgrade errors in object_ownercheck() to internal

[...]

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

I'd add a comment on why we have that, otherwise it seems a bit random:

+   /*
+    * Not needed for correctness, but avoid non-user-facing error later
+    * message if the [database,whatever] doesn't exist.
+    */

(AFAIU it can still happen that the database disappears just after this
check and before the ACL check, so it's possible, just unlikely, to get
the later error. That seems OK.)

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"World domination is proceeding according to plan" (Andrew Morton)

On 14.01.25 10:10, Alvaro Herrera wrote:

On 2024-Dec-20, Peter Eisentraut wrote:

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.

LGTM.

Subject: [PATCH v2 1/2] Downgrade errors in object_ownercheck() to internal

[...]

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

I'd add a comment on why we have that, otherwise it seems a bit random:

+   /*
+    * Not needed for correctness, but avoid non-user-facing error later
+    * message if the [database,whatever] doesn't exist.
+    */

Committed with that change. Thanks.