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
