From: | Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> |
---|---|
To: | Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> |
Cc: | Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM |
Date: | 2025-09-26 01:16:35 |
Message-ID: | 20250926101635.dea7cec62b02d1c614103f78@sraoss.co.jp |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Thu, 25 Sep 2025 14:31:18 -0700
Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > > I fixed it to use 'generator'.
>
> LGTM. I've pushed the 0001 patch.
Thank you!
> > > > ---
> > > > - /* Complete COPY <sth> FROM <sth> */
> > > > - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny))
> > > > + /* Complete COPY <sth> FROM [PROGRAM] <sth> */
> > > > + else if (Matches("COPY|\\copy", MatchAny, "FROM",
> > > > MatchAnyExcept("PROGRAM")) ||
> > > > + Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", MatchAny))
> > > >
> > > > I see this kind of conversion many places in the patch; convert one
> > > > condition with MatchAny into two conditions with
> > > > MatchAnyExcept("PROGRAM") and '"PROGRAM", MatchAny'. How about
> > > > simplifying it using MatchAnyN. For example,
> > > >
> > > > else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAny, MatchAnyN))
> > >
> > > We could simplify this by using MatchAnyN, but doing so would cause "WITH ("
> > > or "WHERE" to be suggested after "WITH (...)", even though that is not allowed
> > > by the syntax. This could be misleading for users, so I wonder whether it is
> > > worth adding a bit of complexity to prevent possible confusion.
> >
> > There was a mistake in the previous statement: "WHERE" appearing after "WITH (...)"
> > is actually correct. However, this also results in "WITH" being suggested after
> > "WHERE", which is not permitted by the syntax.
>
> True. How about other places? That is, where we check the completion
> after "WITH (". For example:
>
> - /* Complete COPY <sth> TO filename WITH ( */
> - else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, "WITH", "("))
> + /* Complete COPY <sth> TO [PROGRAM] filename WITH ( */
> + else if (Matches("COPY|\\copy", MatchAny, "TO",
> MatchAnyExcept("PROGRAM"), "WITH", "(") ||
> + Matches("COPY|\\copy", MatchAny, "TO", "PROGRAM",
> MatchAny, "WITH", "("))
>
> Does it make sense to replace these two lines with the following one line?
>
> else if (Matches("COPY|\\copy", MatchAny, "TO", MatchAny, MatchAnyN,
> "WITH", "("))
That works for other places where options are suggested after "WITH (" and
"WHERE" is suggested after "WITH (*)".
I've attached updated patches using MatchAnyN following your suggestion.
The patch 0002 was also changed to use Matches, since MathAnyN cannot be used
with HeadMatches. I don't think this is a problem, because the COPY command cannot
be nested and "COPY or "\copy" would always appear at the beginning.
Regards,
Yugo Nagata
--
Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp>
Attachment | Content-Type | Size |
---|---|---|
v7-0002-Improve-tab-completion-for-COPY-option-lists.patch | text/x-diff | 3.0 KB |
v7-0001-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch | text/x-diff | 7.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2025-09-26 01:20:19 | Re: [BUG] temporary file usage report with extended protocol and unnamed portals |
Previous Message | Sami Imseih | 2025-09-26 00:47:48 | Re: Add support for entry counting in pgstats |