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

On Fri, Oct 3, 2025 at 2:06 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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.

I believe we can split the first patch into two patches: one adds
support for STDIN/STDOUT with COMPLETE_WIHT_FILES_PLUS() and another
one adds support for COPY syntaxes using the PROGRAM clause. I've
attached the reorganized patch set and made cosmetic changes to the
0003 patch (i.e., improving COPY option list). What do you think?

Regards,

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