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