Received: from malur.postgresql.org ([217.196.149.56])
	by arkaria.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1sneve-005A51-Fb
	for pgsql-hackers@arkaria.postgresql.org; Mon, 09 Sep 2024 13:59:43 +0000
Received: from localhost ([127.0.0.1] helo=malur.postgresql.org)
	by malur.postgresql.org with esmtp (Exim 4.94.2)
	(envelope-from <pgsql-hackers-owner+archive@lists.postgresql.org>)
	id 1sneve-00Cbwx-4h
	for pgsql-hackers@arkaria.postgresql.org; Mon, 09 Sep 2024 13:59:42 +0000
Received: from makus.postgresql.org ([2001:4800:3e1:1::229])
	by malur.postgresql.org with esmtps  (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <masao.fujii@oss.nttdata.com>)
	id 1snevd-00Cbwp-Qy
	for pgsql-hackers@lists.postgresql.org; Mon, 09 Sep 2024 13:59:41 +0000
Received: from oss.nttdata.com ([49.212.34.109])
	by makus.postgresql.org with esmtps  (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384
	(Exim 4.94.2)
	(envelope-from <masao.fujii@oss.nttdata.com>)
	id 1sneva-000JG9-8k
	for pgsql-hackers@lists.postgresql.org; Mon, 09 Sep 2024 13:59:40 +0000
Received: from [192.168.11.6] (p4455253-ipxg14001funabasi.chiba.ocn.ne.jp [153.195.228.253])
	by oss.nttdata.com (Postfix) with ESMTPSA id 97DAC6149A;
	Mon,  9 Sep 2024 22:59:34 +0900 (JST)
X-Virus-Status: Clean
X-Virus-Scanned: clamav-milter 0.103.11 at oss.nttdata.com
Message-ID: <3a6f2184-c1b8-4877-8356-f2cf3b2aaf84@oss.nttdata.com>
Date: Mon, 9 Sep 2024 22:59:34 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Add has_large_object_privilege function
Content-Language: en-US
To: Yugo NAGATA <nagata@sraoss.co.jp>, pgsql-hackers@lists.postgresql.org
References: <20240702163444.ab586f6075e502eb84f11b1a@sranhm.sraoss.co.jp>
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: <20240702163444.ab586f6075e502eb84f11b1a@sranhm.sraoss.co.jp>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
List-Id: <pgsql-hackers.lists.postgresql.org>
List-Help: <https://lists.postgresql.org/manage/>
List-Subscribe: <https://lists.postgresql.org/manage/>
List-Post: <mailto:pgsql-hackers@lists.postgresql.org>
List-Owner: <mailto:pgsql-hackers-owner@lists.postgresql.org>
List-Archive: <https://www.postgresql.org/list/pgsql-hackers>
Archived-At: <https://www.postgresql.org/message-id/3a6f2184-c1b8-4877-8356-f2cf3b2aaf84%40oss.nttdata.com>
Precedence: bulk



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