Re: Add has_large_object_privilege function

On 2024/07/02 16:34, Yugo NAGATA wrote:
> So, I would like to propose to add
> has_large_object_function for checking if a user has the privilege on a large
> object.

+1

BTW since we already have pg_largeobject, using has_largeobject_privilege might
offer better consistency. However, I'm okay with the current name for now.
Even after committing the patch, we can rename it if others prefer has_largeobject_privilege.

> I am not sure why these
> duplicated codes have been left for long time, and there might be some reasons.
> However, otherwise, I think this deduplication also could reduce possible
> maintenance cost in future.

I couldn't find the discussion that mentioned that reason either,
but I agree with simplifying the code.

As for 0001.patch, should we also remove the inclusion of "access/genam.h" and
"access/htup_details.h" since they're no longer needed?

> 0002 adds has_large_object_privilege function.There are three variations whose
> arguments are combinations of large object OID with user name, user OID, or
> implicit user (current_user).

As for 0002.patch, as the code in these three functions is mostly the same,
it might be more efficient to create a common internal function and have
the three functions call it for simplicity.

Here are other review comments for 0002.patch.

+ <row>
+ <entry role="func_table_entry"><para role="func_signature">
+ <indexterm>
+ <primary>has_large_object_privilege</primary>

In the documentation, access privilege inquiry functions are listed
alphabetically. So, this function's description should come right
after has_language_privilege.

+ * has_large_objec_privilege variants

Typo: s/objec/object

+ * The result is a boolean value: true if user has been granted
+ * the indicated privilege or false if not.

The comment should clarify that NULL is returned if the specified
large object doesn’t exist. For example,
--------------
The result is a boolean value: true if user has the indicated
privilege, false if not, or NULL if object doesn't exist.
--------------

+convert_large_object_priv_string(text *priv_text)

It would be better to use "priv_type_text" instead of "priv_text"
for consistency with similar functions.

+ static const priv_map parameter_priv_map[] = {
+ {"SELECT", ACL_SELECT},
+ {"UPDATE", ACL_UPDATE},

parameter_priv_map should be large_object_priv_map.

Additionally, the entries for "WITH GRANT OPTION" should be included here.

+-- not-existing user
+SELECT has_large_object_privilege(-99999, 1001, 'SELECT'); -- false
+ has_large_object_privilege
+----------------------------
+ t
+(1 row)

The comment states the result should be false, but the actual result is true.
One of them seems incorrect.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION