Re: Add has_large_object_privilege function - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: Add has_large_object_privilege function |
Date | |
Msg-id | 3a6f2184-c1b8-4877-8356-f2cf3b2aaf84@oss.nttdata.com Whole thread Raw |
In response to | Add has_large_object_privilege function (Yugo NAGATA <nagata@sraoss.co.jp>) |
List | pgsql-hackers |
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
pgsql-hackers by date: