Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 9 Jul 2025 20:42:42 +0900
Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

> 
> 
> On 2025/06/11 13:57, Yugo Nagata wrote:
> > On Wed, 11 Jun 2025 13:33:07 +0900
> > Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
> > 
> >>
> >>
> >> On 2025/06/11 11:49, Yugo Nagata wrote:
> >>> 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
> >>
> >> 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"?
> > 
> > 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.)
> 
> Thanks for updating the patch! At first I've pushed the 0001 patch.
> 
> As for the 0002 patch:
> 
> +		if (TailMatches("FOREIGN", "SERVER"))
> +			COMPLETE_WITH_QUERY(Query_for_list_of_servers);
> 
> 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?

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

-- 
Yugo Nagata <nagata@sraoss.co.jp>