Re: Extend ALTER DEFAULT PRIVILEGES for large objects

On Wed, 9 Jul 2025 20:42:42 +0900
Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:

>
>
> On 2025/06/11 13:57, Yugo Nagata wrote:
> > On Wed, 11 Jun 2025 13:33:07 +0900
> > Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)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(at)sraoss(dot)co(dot)jp>