Thread: Add has_large_object_privilege function
Hi, Currently, there are many has_*_privilege functions for table, column, function, type, role, database, schema, language, server, foreign data wrapper, parameter, and so on. However, large object is not supported yet. I can find a way to check the privilege on a large object in the regression test, in which whether a function call such as lo_open(lowrite(..)) raises an error or not is checked. However, I think it is not good that we need to try to write to a large object to check we can write it, and also the transaction will be aborted due to a permission error when the user doesn't have the privilege. So, I would like to propose to add has_large_object_function for checking if a user has the privilege on a large object. I attached two files of patches. 0001 makes a bit refactoring on large object codes. To check if a large object exists, myLargeObjectExists() function has to be used rather than public LargeObjectExists(), because we need to use different snapshots between read and write cases to make the behavior compatible to lo_open. However, myLargeObjectExists() was static function, so I made it public and renamed it to LargeObjectExistsWIthSnapshot(). Also, since these two functions are almost same except to whether snapshot can be specified, I rewrote LargeObjectExists to call LargeObjectExistsWIthSnapshot internally. 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. 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). It returns NULL if not-existing large object id is specified, and false if non-existing user id is specified, and raises an error if non-existing user name is specified. These behavior is similar with has_table_privilege. The regression test is also included. Regards, Yugo Nagata -- Yugo NAGATA <nagata@sraoss.co.jp>
Attachment
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
On 2024/09/10 17:45, Yugo NAGATA wrote: > On Mon, 9 Sep 2024 22:59:34 +0900 > Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > >> >> >> 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 > > Thank you for your looking into this! > I've attached a updated patch. Thanks for updating the patches! LGTM, except for a couple of minor things: +okui chiba * has_largeobject_privilege_id "okui chiba" seems to be a garbage text accidentally added. + * role by Oid, large object by id, and privileges as AclMode. "Oid" seems better than "id" in "large object by id". Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On Thu, 12 Sep 2024 19:51:33 +0900 Yugo NAGATA <nagata@sraoss.co.jp> wrote: > On Thu, 12 Sep 2024 19:07:22 +0900 > Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > > > > On 2024/09/10 17:45, Yugo NAGATA wrote: > > > On Mon, 9 Sep 2024 22:59:34 +0900 > > > Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > > >> > > >> > > >> 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 > > > > > > Thank you for your looking into this! > > > I've attached a updated patch. > > > > Thanks for updating the patches! LGTM, except for a couple of minor things: > > > > +okui chiba * has_largeobject_privilege_id > > > > "okui chiba" seems to be a garbage text accidentally added. > > > > + * role by Oid, large object by id, and privileges as AclMode. > > > > "Oid" seems better than "id" in "large object by id". > > Thank you for pointing out it. > I've fixed them and attached the updated patch, v3. I confirmed the patches are committed in the master branch. Thank you! I've updated the commitfest status to "committed". Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
On 2024/09/26 17:16, Michael Paquier wrote:
> On Fri, Sep 13, 2024 at 03:56:11PM +0900, Yugo Nagata wrote:
>> I confirmed the patches are committed in the master branch.
>> Thank you!
>>
>> I've updated the commitfest status to "committed".
>
> This patch has been committed as of 4eada203a5a8, and introduced this
> block in pg_proc.dat:
>
> { oid => '4551', descr => 'user privilege on large objct by username, large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'name oid text',
> prosrc => 'has_largeobject_privilege_name_id' },
> { oid => '4552', descr => 'current privilege on large objct by large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'oid text',
> prosrc => 'has_largeobject_privilege_id' },
> { oid => '4553', descr => 'user privilege on large objct by user oid, large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'oid oid text',
> prosrc => 'has_largeobject_privilege_id_id' },
>
> This has a couple of mistakes:
> - New functions introduced during a development cycle should use OIDs in
> the range 8000-9999. See 98eab30b93d5, consisting of running
> ./unused_oids to get a random range.
> - The function descriptions are inconsistent and have the three same
> typos:
> -- s/large objct/large object/.
> -- s/current privilege/current user privilege/ for the second entry.
I agree these issues need to be fixed. Thanks for the patch!
> And while that's not mandatory for committers, I would apply a
> reformat-dat-files while on it, to reduce some diffs I'm seeing.
Yes, that sounds better.
> This results in the attached that I'd like to apply to fix all that. It
> needs a catalog version bump, of course.
So, are you planning to commit the patch? If not, I'm happy to handle it!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Thu, 26 Sep 2024 17:16:07 +0900
Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Sep 13, 2024 at 03:56:11PM +0900, Yugo Nagata wrote:
> > I confirmed the patches are committed in the master branch.
> > Thank you!
> >
> > I've updated the commitfest status to "committed".
>
> This patch has been committed as of 4eada203a5a8, and introduced this
> block in pg_proc.dat:
>
> { oid => '4551', descr => 'user privilege on large objct by username, large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'name oid text',
> prosrc => 'has_largeobject_privilege_name_id' },
> { oid => '4552', descr => 'current privilege on large objct by large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'oid text',
> prosrc => 'has_largeobject_privilege_id' },
> { oid => '4553', descr => 'user privilege on large objct by user oid, large object oid',
> proname => 'has_largeobject_privilege', procost => '10', provolatile => 's',
> prorettype => 'bool', proargtypes => 'oid oid text',
> prosrc => 'has_largeobject_privilege_id_id' },
>
> This has a couple of mistakes:
> - New functions introduced during a development cycle should use OIDs in
> the range 8000-9999. See 98eab30b93d5, consisting of running
> ./unused_oids to get a random range.
> - The function descriptions are inconsistent and have the three same
> typos:
> -- s/large objct/large object/.
> -- s/current privilege/current user privilege/ for the second entry.
Thank you for pointing out them.
I used unused_oids to look for available oids, but I missed to
follow the best practice suggested by it. I'll be careful.
Regards,
Yugo Nagata
>
> And while that's not mandatory for committers, I would apply a
> reformat-dat-files while on it, to reduce some diffs I'm seeing.
>
> This results in the attached that I'd like to apply to fix all that. It
> needs a catalog version bump, of course.
> --
> Michael
--
Yugo NAGATA <nagata@sraoss.co.jp>
Michael Paquier <michael@paquier.xyz> writes: > - New functions introduced during a development cycle should use OIDs in > the range 8000-9999. See 98eab30b93d5, consisting of running > ./unused_oids to get a random range. There's been seen several fixups of this kind recently. Should we add a note about this to the comment at the top of all of the pg_*.dat files that have explicit oid assignments? People might be more likely to notice that than the the section over in bki.sgml. - ilmari
On Fri, 27 Sep 2024 11:44:25 +0100 Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> wrote: > Michael Paquier <michael@paquier.xyz> writes: > > > - New functions introduced during a development cycle should use OIDs in > > the range 8000-9999. See 98eab30b93d5, consisting of running > > ./unused_oids to get a random range. > > There's been seen several fixups of this kind recently. Should we add a > note about this to the comment at the top of all of the pg_*.dat files > that have explicit oid assignments? People might be more likely to > notice that than the the section over in bki.sgml. How about adding more to unused_oids output to explain the reason why patches should use consecutive OIDs in the range 8000-9999 and low OIDs should not be used in patches(that is, this minimizes the risk of OID collisions with other patches) instead of just saying it is the best practise. I think patch authors looking for OIDs they can use will run unused_oids, so more likely notice this. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>