Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM

On Thu, Sep 25, 2025 at 6:16 PM Yugo Nagata <nagata@sraoss.co.jp> wrote:
>
> On Thu, 25 Sep 2025 14:31:18 -0700
> Masahiko Sawada <sawada.mshk@gmail.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.

Thank you for updating the patch!

After reviewing both the v6 and v7 patches, I realize that your
original approach in v6 is actually better than what I suggested.
While it requires more lines of code, it provides more precise
completions. Additionally, since most of these extra lines are removed
by the next patch (v6-0003), the code size isn't really an issue.
Would it be possible to withdraw my previous comments and proceed with
the v6 approach? I apologize for the back-and-forth on this.

I have two review comments about the complete_from_files() function:

+ * This function wraps _complete_from_files() so that both literal keywords
+ * and filenames can be included in the completion results.
+ *
+ * If completion_charpp is set to a null-terminated array of literal keywords,
+ * those keywords are added to the completion results alongside filenames,
+ * as long as they case-insensitively match the current input.

How about rephrasing the comments to the following?

/*
 * This function returns in order one of a fixed, NULL pointer terminated list
 * of string that matches file names or optionally specified list of keywords.
 *
 * If completion_charpp is set to a null-terminated array of literal keywords,
 * those keywords are added to the completion results alongside filenames if
 * they case-insensitively match the current input.
 */

---
+   /* Return a filename that matches */
+   if (!files_done && (result = _complete_from_files(text, state)))
+       return result;
+   else if (!completion_charpp)
+       return NULL;
+   else
+       files_done = true;

It works but it's odd a bit that we don't set files_done to true if
completion_charpp is not NULL. I think it becomes more readable if we
could set files_done to true if _complete_from_files() doesn't return
a string and proceed with the hard-wired keywords.

The attached patch that can be applied on top of v6-0002 patch,
implements my suggestions and includes pgindent fixes. Please review
these changes.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com