Horton

Thread

Commits

  1. Downgrade error in object_aclmask_ext() to internal

  2. Downgrade errors in object_ownercheck() to internal

  1. downgrade some aclchk.c errors to internal

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-20T11:47:31Z

    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.

  2. Re: downgrade some aclchk.c errors to internal

    Peter Eisentraut <peter@eisentraut.org> — 2024-12-20T14:03:17Z

    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.

  3. Re: downgrade some aclchk.c errors to internal

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-01-14T09:10:35Z

    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)

  4. Re: downgrade some aclchk.c errors to internal

    Peter Eisentraut <peter@eisentraut.org> — 2025-01-15T16:05:02Z

    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.