Hi Sawada-san, I still cannot confirm that my previous email has been archived [1], so let me resend it. I apologize again for the repeated emails.
[1] https://www.postgresql.org/list/pgsql-hackers/since/202511130000/ On Tue, 4 Nov 2025 10:57:14 -0800 Masahiko Sawada <[email protected]> wrote: > On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <[email protected]> wrote: > > > > On Fri, Oct 3, 2025 at 2:06 PM Masahiko Sawada <[email protected]> > > wrote: > > > > > > On Thu, Sep 25, 2025 at 6:16 PM Yugo Nagata <[email protected]> wrote: > > > > > > > > On Thu, 25 Sep 2025 14:31:18 -0700 > > > > Masahiko Sawada <[email protected]> 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. I'm sorry for the late reply. Thank you for reviewing and committing the patches! I've attached an updated patch that includes completions for the HEADER and FREEZE option values. Regards, Yugo Nagata -- Yugo Nagata <[email protected]>
>From 5d444f70891a4ea5d0bb7b2d1af07750d2a38413 Mon Sep 17 00:00:00 2001 From: Masahiko Sawada <[email protected]> Date: Mon, 6 Oct 2025 15:03:31 -0700 Subject: [PATCH v9] psql: Improve tab completion for COPY option lists. Previously, only the first option in a parenthesized option list was suggested by tab completion. This commit enhances tab completion for both COPY TO and COPY FROM commands to suggest options after each comma. Also add completion for HEADER and FREEZE option value candidates. Author: Yugo Nagata <[email protected]> Reviewed-by: Masahiko Sawada <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/bin/psql/tab-complete.in.c | 68 ++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 24 deletions(-) diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c index 51806597037..c3b0cfc3391 100644 --- a/src/bin/psql/tab-complete.in.c +++ b/src/bin/psql/tab-complete.in.c @@ -3364,30 +3364,50 @@ match_previous_words(int pattern_id, Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", MatchAny)) COMPLETE_WITH("WITH (", "WHERE"); - /* Complete COPY <sth> FROM [PROGRAM] filename WITH ( */ - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAnyExcept("PROGRAM"), "WITH", "(") || - Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", MatchAny, "WITH", "(")) - COMPLETE_WITH(Copy_from_options); - - /* 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", "(")) - COMPLETE_WITH(Copy_to_options); - - /* Complete COPY <sth> FROM|TO [PROGRAM] <sth> WITH (FORMAT */ - else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", MatchAnyExcept("PROGRAM"), "WITH", "(", "FORMAT") || - Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM", MatchAny, "WITH", "(", "FORMAT")) - COMPLETE_WITH("binary", "csv", "text"); - - /* Complete COPY <sth> FROM [PROGRAM] filename WITH (ON_ERROR */ - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAnyExcept("PROGRAM"), "WITH", "(", "ON_ERROR") || - Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", MatchAny, "WITH", "(", "ON_ERROR")) - COMPLETE_WITH("stop", "ignore"); - - /* Complete COPY <sth> FROM [PROGRAM] filename WITH (LOG_VERBOSITY */ - else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAnyExcept("PROGRAM"), "WITH", "(", "LOG_VERBOSITY") || - Matches("COPY|\\copy", MatchAny, "FROM", "PROGRAM", MatchAny, "WITH", "(", "LOG_VERBOSITY")) - COMPLETE_WITH("silent", "default", "verbose"); + /* Complete COPY <sth> FROM|TO [PROGRAM] filename WITH ( */ + else if (HeadMatches("COPY|\\copy", MatchAny, "FROM|TO", MatchAnyExcept("PROGRAM"), "WITH", "(") || + HeadMatches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM", MatchAny, "WITH", "(")) + { + if (!HeadMatches("COPY|\\copy", MatchAny, "FROM|TO", MatchAnyExcept("PROGRAM"), "WITH", "(*)") && + !HeadMatches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM", MatchAny, "WITH", "(*)")) + { + /* + * This fires if we're in an unfinished parenthesized option list. + * get_previous_words treats a completed parenthesized option list + * as one word, so the above tests are correct. + */ + + if (ends_with(prev_wd, '(') || ends_with(prev_wd, ',')) + { + if (HeadMatches("COPY|\\copy", MatchAny, "FROM")) + COMPLETE_WITH(Copy_from_options); + else + COMPLETE_WITH(Copy_to_options); + } + + /* Complete COPY <sth> FROM|TO filename WITH (FORMAT */ + else if (TailMatches("FORMAT")) + COMPLETE_WITH("binary", "csv", "text"); + + /* Complete COPY <sth> FROM|TO filename WITH (FREEZE */ + else if (TailMatches("FREEZE")) + COMPLETE_WITH("true", "false"); + + /* Complete COPY <sth> FROM|TO filename WITH (HEADER */ + else if (TailMatches("HEADER")) + COMPLETE_WITH("true", "false", "MATCH"); + + /* Complete COPY <sth> FROM filename WITH (ON_ERROR */ + else if (TailMatches("ON_ERROR")) + COMPLETE_WITH("stop", "ignore"); + + /* Complete COPY <sth> FROM filename WITH (LOG_VERBOSITY */ + else if (TailMatches("LOG_VERBOSITY")) + COMPLETE_WITH("silent", "default", "verbose"); + } + + /* A completed parenthesized option list should be caught below */ + } /* Complete COPY <sth> FROM [PROGRAM] <sth> WITH (<options>) */ else if (Matches("COPY|\\copy", MatchAny, "FROM", MatchAnyExcept("PROGRAM"), "WITH", MatchAny) || -- 2.43.0
