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

On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Fri, Oct 3, 2025 at 2:06 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Thu, Sep 25, 2025 at 6:16 PM Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> > >
> > > 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.
> >
> > 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?

Pushed the first two patches.

As for the remaining patch that adds tab completion for COPY option
lists, I think it would be a good idea to add tab completion for other
options too such as HEADER and FREEZE.

Regards,

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