Subject: Re: Extend ALTER DEFAULT PRIVILEGES for large objects

From: Fujii Masao <[email-masked]>
Date: Wed, 2 Apr 2025 02:35:35 +0900

On 2025/01/23 19:22, Yugo NAGATA wrote:
> On Wed, 22 Jan 2025 13:30:17 +0100
> Laurenz Albe <[email-masked]> wrote:
> 
>> On Fri, 2024-09-13 at 16:18 +0900, Yugo Nagata wrote:
>>> I've attached a updated patch. The test is rewritten using has_largeobject_privilege()
>>> function instead of calling loread & lowrite, which makes the test a bit simpler.
>>> Thare are no other changes.
>>
>> When I tried to apply this patch, I found that it doesn't apply any
>> more since commit f391d9dc93 renamed tab-complete.c to tab-complete.in.c.
>>
>> Attached is a rebased patch.
> 
> Thank you for updating the patch!
> 
>> I agree that large objects are a feature that should fade out (alas,
>> the JDBC driver still uses it for BLOBs).  But this patch is not big
>> or complicated and is unlikely to create a big maintenance burden.
>>
>> So I am somewhat for committing it.  It works as advertised.
>> If you are fine with my rebased patch, I can mark it as "ready for
>> committer".  If it actually gets committed depends on whether there
>> is a committer who thinks it worth the effort or not.
> 
> I confirmed the patch and I am fine with it.

I've started reviewing this patch since it's marked as "ready for committer".

I know of several systems that use large objects, and I believe
this feature would be beneficial for them. Overall, I like the idea.

The latest patch looks good to me. I just have one minor comment:

>   only the privileges for schemas, tables (including views and foreign
>   tables), sequences, functions, and types (including domains) can be
>   altered.

In alter_default_privileges.sgml, this part should also mention large objects?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: Fujii Masao <[email-masked]>
Date: Fri, 4 Apr 2025 00:21:27 +0900

On 2025/04/03 23:04, Yugo NAGATA wrote:
> On Wed, 2 Apr 2025 02:35:35 +0900
> Fujii Masao <[email-masked]> wrote:
> 
>>
>>
> Thank you for your reviewing this patch!
> 
> Agreed. I attached a updated patch.

Thanks for updating the patch!

If there are no objections, I'll proceed with committing it using the following commit log.

----------------
Extend ALTER DEFAULT PRIVILEGES to define default privileges for large objects.

Previously, ALTER DEFAULT PRIVILEGES did not support large objects.
This meant that to grant privileges to users other than the owner,
permissions had to be manually assigned each time a large object
was created, which was inconvenient.

This commit extends ALTER DEFAULT PRIVILEGES to allow defining default
access privileges for large objects. With this change, specified privileges
will automatically apply to newly created large objects, making privilege
management more efficient.

As a side effect, this commit introduces the new keyword OBJECTS
since it's used in the syntax of ALTER DEFAULT PRIVILEGES.

Original patch by Haruka Takatsuka, with some fixes and tests by Yugo Nagata,
and rebased by Laurenz Albe.

Author: Takatsuka Haruka <[email-masked]>
Co-authored-by: Yugo Nagata <[email-masked]>
Co-authored-by: Laurenz Albe <[email-masked]>
Reviewed-by: Masao Fujii <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
----------------

Regards,

---

From: Nathan Bossart <[email-masked]>
Date: Fri, 4 Apr 2025 09:47:37 -0500

On Fri, Apr 04, 2025 at 07:18:11PM +0900, Fujii Masao wrote:
> I've pushed the patch. Thanks!

Just a heads up, I fixed a pgindent issue in this commit (see commits
e1a8b1ad58 and 742317a80f).  I'd ordinarily just report it, but since we're
nearing feature freeze, I just fixed it because my workflow (and presumably
others') involves running pgindent on the entire tree periodically.

-- 
nathan

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 11:49:37 +0900

On Tue, 8 Apr 2025 12:28:57 +0900
Yugo NAGATA <[email-masked]> wrote:

> On Fri, 4 Apr 2025 19:18:11 +0900
> > On 2025/04/04 0:21, Fujii Masao wrote:
> Thank you!
> > While testing the feature, I noticed that psql doesn't complete
> > "ALTER DEFAULT PRIVILEGES GRANT/REVOKE ... ON LARGE OBJECTS" or
> > "GRANT/REVOKE ... ON LARGE OBJECT ..." with TO/FROM. The attached
> > patch adds tab-completion support for both cases.
> 
> This patch looks good to me. This works as expected.
> 
> While looking into this patch, I found that the tab completion suggests
> TO/FROM even after "LARGE OBJECT", but it is not correct because
> there should be largeobject id at that place. This is same for the
> "FOREIGN SERVER", server names should be suggested ratar than TO/FROM
> in this case. 
> 
> The additional patch 0002 fixed to prevents to suggest TO or FROM right
> after LARGE OBJECT or FOREIGN SERVER. Also, it allows to suggest list of
> foreign server names after FOREIGN SERVER.
> 

While looking at the thread [1], I've remembered this thread.
The patches in this thread are partially v18-related, but include
enhancement or fixes for existing feature, so should they be postponed
to v19, or should be separated properly to v18 part and other?

[1] https://www.postgresql.org/message-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a%40oss.nttdata.com

Best regards,
Yugo Nagata

-- 
Yugo Nagata <[email-masked]>

---

From: Fujii Masao <[email-masked]>
Date: Wed, 11 Jun 2025 13:33:07 +0900

> [1] https://www.postgresql.org/message-id/70372bdd-4399-4d5b-ab4f-6d4487a4911a%40oss.nttdata.com

I see these patches more as enhancements to psql tab-completion,
rather than fixes for clear oversights in the original commit.

For example, if tab-completion for ALTER DEFAULT PRIVILEGES had
completely missed LARGE OBJECTS, that would be an obvious oversight.
But these patches go beyond that kind of issue.

That said, if others think it's appropriate to include them in v18
for consistency or completeness, I'm fine with that.

Regarding the 0002 patch:

-	else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("TO");
-	else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-		COMPLETE_WITH("FROM");
+	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
+	{
+		if (TailMatches("FOREIGN", "SERVER"))
+			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
+		else if (!TailMatches("LARGE", "OBJECT"))
+		{
+			if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+				COMPLETE_WITH("TO");
+			else
+				COMPLETE_WITH("FROM");
+		}
+	}

Wouldn't this change break the case where "GRANT ... ON TABLE ... <TAB>"
is supposed to complete with "TO"?

Regards,

-- 
Fujii Masao
NTT DATA Japan Corporation

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 13:57:37 +0900

On Wed, 11 Jun 2025 13:33:07 +0900

> is supposed to complete with "TO"?

Sorry, I made a stupid mistake.

> +	else if (Matches("GRANT/REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))

This should be "GRANT|REVOKE".

I've attached update patches. (There is no change on 0001.)

Best regards,
Yugo Nagata

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 11 Jun 2025 14:03:01 +0900

> But these patches go beyond that kind of issue.

Thank you for your clarification. I agreed.

Best regards,
Yugo Nagata

---

From: Fujii Masao <[email-masked]>
Date: Wed, 9 Jul 2025 20:42:42 +0900

> I've attached update patches. (There is no change on 0001.)

Thanks for updating the patch! At first I've pushed the 0001 patch.

As for the 0002 patch:

This part seems not needed, since we already have the following tab-completion code:

/* FOREIGN SERVER */
	else if (TailMatches("FOREIGN", "SERVER"))
		COMPLETE_WITH_QUERY(Query_for_list_of_servers);

Thought?

Regards,

---

From: Yugo Nagata <[email-masked]>
Date: Thu, 10 Jul 2025 10:30:41 +0900

On Wed, 9 Jul 2025 20:42:42 +0900

> 		COMPLETE_WITH_QUERY(Query_for_list_of_servers);

You're right. I must have overlooked something. I think I saw "TO" being
suggested after "FOREIGN SERVER" when no foreign servers were defined.

The attached patch still prevents "TO/FROM" from being suggested after
"FOREIGN SERVER" in such cases. But perhaps this corner case doesn't really
need to be handled?

Regards,
Yugo Nagata

---

From: Fujii Masao <[email-masked]>
Date: Thu, 10 Jul 2025 13:23:47 +0900

On 2025/07/10 10:30, Yugo Nagata wrote:
> "FOREIGN SERVER" in such cases.

Based on your patch, I'm thinking of simplifying the code like this:

-       else if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("TO");
-       else if (Matches("REVOKE", MatchAnyN, "ON", MatchAny, MatchAny))
-               COMPLETE_WITH("FROM");
+       else if (Matches("GRANT|REVOKE", MatchAnyN, "ON", MatchAny, MatchAny) &&
+                        !TailMatches("FOREIGN", "SERVER") && !TailMatches("LARGE", "OBJECT"))
+       {
+               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))
+                       COMPLETE_WITH("TO");
+               else
+                       COMPLETE_WITH("FROM");
+       }

> But perhaps this corner case doesn't really
> need to be handled?

Probably I failed to get your point here. Could you clarify what you meant?

Regards,

---

From: Yugo Nagata <[email-masked]>
Date: Thu, 10 Jul 2025 14:11:29 +0900

On Thu, 10 Jul 2025 13:23:47 +0900

> +               if (Matches("GRANT", MatchAnyN, "ON", MatchAny, MatchAny))

Thank you for your review!
 I agree with your suggestion and have updated the patch accordingly.

> Probably I failed to get your point here. Could you clarify what you meant?

I'm sorry for not explaining it clearly.

Currently, TO or FROM could be suggested immediately after FOREIGN SERVER, but
only when no foreign servers are defined. When foreign servers do exist,
their names are correctly suggested instead, as expected.

The patch fixed the behavior so that TO or FROM are not suggested after FOREIGN SERVER,
even when no foreign servers are defined. However, I've started to wonder if it's worth
fixing such a corner case. What do you think?

Regards,
Yugo Nagata

---

From: Fujii Masao <[email-masked]>
Date: Tue, 15 Jul 2025 19:07:16 +0900

> fixing such a corner case. What do you think?

I think it's worth doing. This issue can lead to unexpected behavior
and is something users might run into. If the fix were overly complex
for a minor issue, it might not be justified. But that's not the case here.

Regards,

---

From: Yugo Nagata <[email-masked]>
Date: Wed, 16 Jul 2025 16:22:31 +0900

On Tue, 15 Jul 2025 19:07:16 +0900

Thank you!

> for a minor issue, it might not be justified. But that's not the case here.

Thank you for your explanation.
It makes sense to me.

Regards,
Yugo Nagata