Thread
Commits
-
psql: Improve tab completion for COPY option lists.
- c6a7d3bab493 19 (unreleased) landed
-
psql: Add tab completion for COPY ... PROGRAM.
- 6d2ff1de4d0b 19 (unreleased) landed
-
psql: Improve tab completion for COPY ... STDIN/STDOUT.
- 02fd47dbfade 19 (unreleased) landed
-
psql: Add COMPLETE_WITH_FILES and COMPLETE_WITH_GENERATOR macros.
- 76418a0b6762 19 (unreleased) landed
-
Define a new, more extensible syntax for COPY options.
- 923413ac6d31 9.0.0 cited
-
psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-06-05T01:08:35Z
Hi, Currently, tab completion for COPY only suggests filenames after TO or FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. I'd like to propose improving the tab completion behavior as described in the subject, so that these keywords are suggested appropriately, and filenames are offered as potential command names after the PROGRAM keyword. I've attached this proposal as a patch series with the following three parts: 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() This is a preparatory cleanup. Most completions in match_previous_words() already use COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls still remain. This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or COMPLETE_WITH_GENERATOR, improving consistency and readability. 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM after TO or FROM. After PROGRAM, filenames are suggested as possible command names. To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows combining literal keywords with filename suggestions in the completion list. 0003: Improve tab completion for COPY option lists Currently, only the first option in a parenthesized list is suggested during completion, and nothing is suggested after a comma. This patch enables suggestions after each comma, improving usability when specifying multiple options. Although not directly related to the main proposal, I believe this is a helpful enhancement to COPY tab completion and included it here for completeness. I’d appreciate your review and feedback on this series. Best regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-06-05T07:52:00Z
On Thu, 5 Jun 2025 10:08:35 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > Hi, > > Currently, tab completion for COPY only suggests filenames after TO or > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > I'd like to propose improving the tab completion behavior as described in > the subject, so that these keywords are suggested appropriately, and filenames > are offered as potential command names after the PROGRAM keyword. > > I've attached this proposal as a patch series with the following three parts: I'm sorry but the previous patches were accidentally broken and didn't work. I've attached fixed patches. > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > This is a preparatory cleanup. Most completions in match_previous_words() already use > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > still remain. > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > combining literal keywords with filename suggestions in the completion list. > > 0003: Improve tab completion for COPY option lists > > Currently, only the first option in a parenthesized list is suggested during completion, > and nothing is suggested after a comma. > > This patch enables suggestions after each comma, improving usability when specifying > multiple options. > > Although not directly related to the main proposal, I believe this is a helpful enhancement > to COPY tab completion and included it here for completeness. > > I’d appreciate your review and feedback on this series. > > > Best regards, > Yugo Nagata > > -- > Yugo Nagata <nagata@sraoss.co.jp> -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-06-16T15:08:32Z
On Thu, 5 Jun 2025 16:52:00 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Thu, 5 Jun 2025 10:08:35 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > Hi, > > > > Currently, tab completion for COPY only suggests filenames after TO or > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > I'd like to propose improving the tab completion behavior as described in > > the subject, so that these keywords are suggested appropriately, and filenames > > are offered as potential command names after the PROGRAM keyword. > > > > I've attached this proposal as a patch series with the following three parts: > > I'm sorry but the previous patches were accidentally broken and didn't work. > I've attached fixed patches. > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > still remain. > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > combining literal keywords with filename suggestions in the completion list. > > > > 0003: Improve tab completion for COPY option lists > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > and nothing is suggested after a comma. > > > > This patch enables suggestions after each comma, improving usability when specifying > > multiple options. > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > to COPY tab completion and included it here for completeness. > > > > I’d appreciate your review and feedback on this series. I've attached rebased patches. Best regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-07-17T01:57:36Z
On Tue, 17 Jun 2025 00:08:32 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Thu, 5 Jun 2025 16:52:00 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > Hi, > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > I'd like to propose improving the tab completion behavior as described in > > > the subject, so that these keywords are suggested appropriately, and filenames > > > are offered as potential command names after the PROGRAM keyword. > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > I've attached fixed patches. > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > still remain. > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > combining literal keywords with filename suggestions in the completion list. > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > and nothing is suggested after a comma. > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > multiple options. > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > to COPY tab completion and included it here for completeness. > > > > > > I’d appreciate your review and feedback on this series. I've attached rebased patches. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-07-18T07:49:28Z
On Thu, 17 Jul 2025 10:57:36 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Tue, 17 Jun 2025 00:08:32 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > Hi, > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > I've attached fixed patches. > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > still remain. > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > and nothing is suggested after a comma. > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > multiple options. > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > to COPY tab completion and included it here for completeness. > > > > > > > > I’d appreciate your review and feedback on this series. The previous patch was broken failed to complie since I missed following the required format of if-conditions in match_previous_words(). I've attached update patches. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Kirill Reshke <reshkekirill@gmail.com> — 2025-08-27T18:53:50Z
On Fri, 18 Jul 2025 at 12:49, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Thu, 17 Jul 2025 10:57:36 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Tue, 17 Jun 2025 00:08:32 +0900 > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > Hi, > > > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > I've attached fixed patches. > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > still remain. > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > and nothing is suggested after a comma. > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > multiple options. > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > I’d appreciate your review and feedback on this series. > > The previous patch was broken failed to complie since I missed following > the required format of if-conditions in match_previous_words(). > I've attached update patches. > > Regards, > Yugo Nagata > > -- > Yugo Nagata <nagata@sraoss.co.jp> Hi! This tab completion support is indeed very useful. Thanks for working on this. I spotted potential improvement here: current v5 does not support COPY completion for a pattern I do frequently use: specifying COPY options without WITH clause e.g. "copy yy from '/home/reshke/cpg/csv.csv' <tab>" Is not completed with BINARY, CSV, DELIMITER etc. -- Best regards, Kirill Reshke
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-08-28T07:22:04Z
Hi > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > > I've attached fixed patches. > > > > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > > still remain. > > > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > > and nothing is suggested after a comma. > > > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > > multiple options. > > > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > > > I’d appreciate your review and feedback on this series. > Hi! This tab completion support is indeed very useful. Thanks for > working on this. Thank you for your feedback! > I spotted potential improvement here: current v5 does not support COPY > completion for a pattern I do frequently use: > specifying COPY options without WITH clause e.g. "copy yy from > '/home/reshke/cpg/csv.csv' <tab>" Is not completed with BINARY, CSV, > DELIMITER etc. This is the old syntax and is supported for backward compatibility (commit 923413ac6d3), but I’m not sure it’s worth supporting in tab completion. Perhaps it would be better to discuss this point in a separate thread. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Kirill Reshke <reshkekirill@gmail.com> — 2025-08-28T08:36:00Z
On Thu, 28 Aug 2025 at 12:22, Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > Hi! This tab completion support is indeed very useful. Thanks for > > working on this. > > Thank you for your feedback! > > > I spotted potential improvement here: current v5 does not support COPY > > completion for a pattern I do frequently use: > > specifying COPY options without WITH clause e.g. "copy yy from > > '/home/reshke/cpg/csv.csv' <tab>" Is not completed with BINARY, CSV, > > DELIMITER etc. > > This is the old syntax and is supported for backward compatibility > (commit 923413ac6d3), but I’m not sure it’s worth supporting in tab > completion. Perhaps it would be better to discuss this point in a > separate thread. > I see your point. However I do not think that old syntax is a show-stopper here. current psql (on HEAD) has TAB support for archaic patterns like START <tab> completed with TRANSACTION and ABORT W<tab> completed with WORK. And I do think that the case I use in COPY (and propose to support thus) is more frequent than these two. I came across this thread while looking to support my usecase. This does not make you obligated to support this in this patch series. I can even send v5-0004 with this support in the thread if that's okay. -- Best regards, Kirill Reshke
-
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-09-17T19:53:10Z
On Fri, Jul 18, 2025 at 12:49 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Thu, 17 Jul 2025 10:57:36 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Tue, 17 Jun 2025 00:08:32 +0900 > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > Hi, > > > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > I've attached fixed patches. > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > still remain. > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > and nothing is suggested after a comma. > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > multiple options. > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > I’d appreciate your review and feedback on this series. > > The previous patch was broken failed to complie since I missed following > the required format of if-conditions in match_previous_words(). > I've attached update patches. > I agree with the basic direction of the patches. Here are some comments on the first two patches: v5-0001-Refactor-match_previous_words-to-remove-direct-us.patch: --- +#define COMPLETE_WITH_GENERATOR(function) \ + matches = rl_completion_matches(text, function) I think it would be clearer if we use 'generator' or 'genfunc' instead of 'function' as a macro argument. v5-0002-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch: --- + /* Complete COPY|\copy <sth> FROM|TO PROGRAM command */ + else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM")) + COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* COPY requires quoted filename */ Why does it complete the query with files names even after 'PROGRAM'? --- +static char * +_complete_from_files(const char *text, int state) { I think the comments of complete_from_files() should be moved to this new function. For instance, the comments starts with: * This function wraps rl_filename_completion_function() to strip quotes from * the input before searching for matches and to quote any matches for which * the consuming command will require it. But complete_from_files() function no longer calls rl_filename_completion_function(). --- - /* 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)) Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-09-18T03:05:06Z
On Wed, 17 Sep 2025 12:53:10 -0700 Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Fri, Jul 18, 2025 at 12:49 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > On Thu, 17 Jul 2025 10:57:36 +0900 > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > On Tue, 17 Jun 2025 00:08:32 +0900 > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > Hi, > > > > > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > > I've attached fixed patches. > > > > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > > still remain. > > > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > > and nothing is suggested after a comma. > > > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > > multiple options. > > > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > > > I’d appreciate your review and feedback on this series. > > > > The previous patch was broken failed to complie since I missed following > > the required format of if-conditions in match_previous_words(). > > I've attached update patches. > > > > I agree with the basic direction of the patches. Here are some > comments on the first two patches: Thank you for reviewing it. I've attached an updated patch. > > v5-0001-Refactor-match_previous_words-to-remove-direct-us.patch: > > --- > +#define COMPLETE_WITH_GENERATOR(function) \ > + matches = rl_completion_matches(text, function) > > I think it would be clearer if we use 'generator' or 'genfunc' instead > of 'function' as a macro argument. I fixed it to use 'generator'. > > v5-0002-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch: > > --- > + /* Complete COPY|\copy <sth> FROM|TO PROGRAM command */ > + else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM")) > + COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* > COPY requires quoted filename */ > > Why does it complete the query with files names even after 'PROGRAM'? Users can specify the command by giving a filename with an absolute or relative path, so I think it makes sense to allow filename completion after PROGRAM. > --- > +static char * > +_complete_from_files(const char *text, int state) > { > > I think the comments of complete_from_files() should be moved to this > new function. For instance, the comments starts with: > > * This function wraps rl_filename_completion_function() to strip quotes from > * the input before searching for matches and to quote any matches for which > * the consuming command will require it. > > But complete_from_files() function no longer calls > rl_filename_completion_function(). I moved the comments to the top of _complete_from_files() and added a new comment for complete_from_files() to describe that it is a wrapper of the former. > --- > - /* 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. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-09-18T03:27:49Z
On Thu, 18 Sep 2025 12:05:06 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Wed, 17 Sep 2025 12:53:10 -0700 > Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > On Fri, Jul 18, 2025 at 12:49 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > On Thu, 17 Jul 2025 10:57:36 +0900 > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > On Tue, 17 Jun 2025 00:08:32 +0900 > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > > > I've attached fixed patches. > > > > > > > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > > > still remain. > > > > > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > > > and nothing is suggested after a comma. > > > > > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > > > multiple options. > > > > > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > > > > > I’d appreciate your review and feedback on this series. > > > > > > The previous patch was broken failed to complie since I missed following > > > the required format of if-conditions in match_previous_words(). > > > I've attached update patches. > > > > > > > I agree with the basic direction of the patches. Here are some > > comments on the first two patches: > > Thank you for reviewing it. > I've attached an updated patch. > > > > > v5-0001-Refactor-match_previous_words-to-remove-direct-us.patch: > > > > --- > > +#define COMPLETE_WITH_GENERATOR(function) \ > > + matches = rl_completion_matches(text, function) > > > > I think it would be clearer if we use 'generator' or 'genfunc' instead > > of 'function' as a macro argument. > > I fixed it to use 'generator'. > > > > > v5-0002-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch: > > > > --- > > + /* Complete COPY|\copy <sth> FROM|TO PROGRAM command */ > > + else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM")) > > + COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* > > COPY requires quoted filename */ > > > > Why does it complete the query with files names even after 'PROGRAM'? > > Users can specify the command by giving a filename with an absolute or > relative path, so I think it makes sense to allow filename completion > after PROGRAM. > > > --- > > +static char * > > +_complete_from_files(const char *text, int state) > > { > > > > I think the comments of complete_from_files() should be moved to this > > new function. For instance, the comments starts with: > > > > * This function wraps rl_filename_completion_function() to strip quotes from > > * the input before searching for matches and to quote any matches for which > > * the consuming command will require it. > > > > But complete_from_files() function no longer calls > > rl_filename_completion_function(). > > I moved the comments to the top of _complete_from_files() and added a new > comment for complete_from_files() to describe that it is a wrapper of the > former. > > > --- > > - /* 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. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-09-25T21:31:18Z
On Wed, Sep 17, 2025 at 8:27 PM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Thu, 18 Sep 2025 12:05:06 +0900 > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > On Wed, 17 Sep 2025 12:53:10 -0700 > > Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > On Fri, Jul 18, 2025 at 12:49 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > On Thu, 17 Jul 2025 10:57:36 +0900 > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > On Tue, 17 Jun 2025 00:08:32 +0900 > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > On Thu, 5 Jun 2025 16:52:00 +0900 > > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > > On Thu, 5 Jun 2025 10:08:35 +0900 > > > > > > > Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > Currently, tab completion for COPY only suggests filenames after TO or > > > > > > > > FROM, even though STDIN, STDOUT, and PROGRAM are also valid syntax options. > > > > > > > > > > > > > > > > I'd like to propose improving the tab completion behavior as described in > > > > > > > > the subject, so that these keywords are suggested appropriately, and filenames > > > > > > > > are offered as potential command names after the PROGRAM keyword. > > > > > > > > > > > > > > > > I've attached this proposal as a patch series with the following three parts: > > > > > > > > > > > > > > I'm sorry but the previous patches were accidentally broken and didn't work. > > > > > > > I've attached fixed patches. > > > > > > > > > > > > > > > > > > > > > > > 0001: Refactor match_previous_words() to remove direct use of rl_completion_matches() > > > > > > > > > > > > > > > > This is a preparatory cleanup. Most completions in match_previous_words() already use > > > > > > > > COMPLETE_WITH* macros, which wrap rl_completion_matches(). However, some direct calls > > > > > > > > still remain. > > > > > > > > > > > > > > > > This patch replaces the remaining direct calls with COMPLETE_WITH_FILES or > > > > > > > > COMPLETE_WITH_GENERATOR, improving consistency and readability. > > > > > > > > > > > > > > > > 0002: Add tab completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM > > > > > > > > > > > > > > > > This is the main patch. It extends tab completion to suggest STDIN, STDOUT, and PROGRAM > > > > > > > > after TO or FROM. After PROGRAM, filenames are suggested as possible command names. > > > > > > > > > > > > > > > > To support this, a new macro COMPLETE_WITH_FILES_PLUS is introduced. This allows > > > > > > > > combining literal keywords with filename suggestions in the completion list. > > > > > > > > > > > > > > > > 0003: Improve tab completion for COPY option lists > > > > > > > > > > > > > > > > Currently, only the first option in a parenthesized list is suggested during completion, > > > > > > > > and nothing is suggested after a comma. > > > > > > > > > > > > > > > > This patch enables suggestions after each comma, improving usability when specifying > > > > > > > > multiple options. > > > > > > > > > > > > > > > > Although not directly related to the main proposal, I believe this is a helpful enhancement > > > > > > > > to COPY tab completion and included it here for completeness. > > > > > > > > > > > > > > > > I’d appreciate your review and feedback on this series. > > > > > > > > The previous patch was broken failed to complie since I missed following > > > > the required format of if-conditions in match_previous_words(). > > > > I've attached update patches. > > > > > > > > > > I agree with the basic direction of the patches. Here are some > > > comments on the first two patches: > > > > Thank you for reviewing it. > > I've attached an updated patch. Thank you for updating the patches! > > > > > > > > v5-0001-Refactor-match_previous_words-to-remove-direct-us.patch: > > > > > > --- > > > +#define COMPLETE_WITH_GENERATOR(function) \ > > > + matches = rl_completion_matches(text, function) > > > > > > I think it would be clearer if we use 'generator' or 'genfunc' instead > > > of 'function' as a macro argument. > > > > I fixed it to use 'generator'. LGTM. I've pushed the 0001 patch. > > > > > > > > v5-0002-Add-tab-completion-support-for-COPY-.-TO-FROM-STD.patch: > > > > > > --- > > > + /* Complete COPY|\copy <sth> FROM|TO PROGRAM command */ > > > + else if (Matches("COPY|\\copy", MatchAny, "FROM|TO", "PROGRAM")) > > > + COMPLETE_WITH_FILES("", HeadMatches("COPY")); /* > > > COPY requires quoted filename */ > > > > > > Why does it complete the query with files names even after 'PROGRAM'? > > > > Users can specify the command by giving a filename with an absolute or > > relative path, so I think it makes sense to allow filename completion > > after PROGRAM. Agreed. > > > > > --- > > > +static char * > > > +_complete_from_files(const char *text, int state) > > > { > > > > > > I think the comments of complete_from_files() should be moved to this > > > new function. For instance, the comments starts with: > > > > > > * This function wraps rl_filename_completion_function() to strip quotes from > > > * the input before searching for matches and to quote any matches for which > > > * the consuming command will require it. > > > > > > But complete_from_files() function no longer calls > > > rl_filename_completion_function(). > > > > I moved the comments to the top of _complete_from_files() and added a new > > comment for complete_from_files() to describe that it is a wrapper of the > > former. > > > > > --- > > > - /* 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", "(")) Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-09-26T01:16:35Z
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. 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@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-10-03T21:06:57Z
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 -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-10-07T00:03:30Z
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 -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-11-04T18:57:14Z
On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > 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? 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 -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-11-14T07:55:06Z
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 <sawada.mshk@gmail.com> wrote: > On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > 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? > > 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 <nagata@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-11-24T23:32:36Z
On Thu, Nov 13, 2025 at 11:55 PM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > 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 <sawada.mshk@gmail.com> wrote: > > > On Mon, Oct 6, 2025 at 5:03 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > > > 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? > > > > 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. > Thank you for updating the patch! I have one question about the following change: + /* 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", "(*)")) + { Given get_previous_words() treats a completed parenthesized option list as one word, why is the later if statement necessary? In the situation where the condition of the later if statement becomes false, the condition of the first 'else if' statement should have become false, no? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-12-22T15:37:05Z
On Mon, 24 Nov 2025 15:32:36 -0800 Masahiko Sawada <sawada.mshk@gmail.com> wrote: I'm sorry for the late response. > Thank you for updating the patch! I have one question about the > following change: > > + /* 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", "(*)")) > + { > > Given get_previous_words() treats a completed parenthesized option > list as one word, why is the later if statement necessary? In the > situation where the condition of the later if statement becomes false, > the condition of the first 'else if' statement should have become > false, no? First of all, the condition of the outer 'else if' was wrong. What I actually meant was: + /* 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", "(*")) That is, the last word should be "(*" instead of "(". I've attached an updated patch reflecting this fix. This patterm is mimic how option lists are handled in the other places. For example: 3160 else if (HeadMatches("ANALYZE", "(*") && 3161 !HeadMatches("ANALYZE", "(*)")) This prevents option completion after a completed paranthesized option list. For example, after "ANALYZE (VERVOSE ON) tbl (", no ANALYZE options are suggested, instead column names of the table "tbl" are suggested. Similarly, this change prevents completion of COPY options after "COPY tbl FROM stdin WITH ( FORMAT text ) WHERE (", for example. Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-12-22T17:22:53Z
On Tue, 23 Dec 2025 00:37:05 +0900 Yugo Nagata <nagata@sraoss.co.jp> wrote: > On Mon, 24 Nov 2025 15:32:36 -0800 > Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > I'm sorry for the late response. > > > Thank you for updating the patch! I have one question about the > > following change: > > > > + /* 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", "(*)")) > > + { > > > > Given get_previous_words() treats a completed parenthesized option > > list as one word, why is the later if statement necessary? In the > > situation where the condition of the later if statement becomes false, > > the condition of the first 'else if' statement should have become > > false, no? > > First of all, the condition of the outer 'else if' was wrong. > What I actually meant was: > > + /* 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", "(*")) > > That is, the last word should be "(*" instead of "(". > I've attached an updated patch reflecting this fix. > > This patterm is mimic how option lists are handled in the other places. > For example: > > 3160 else if (HeadMatches("ANALYZE", "(*") && > 3161 !HeadMatches("ANALYZE", "(*)")) > > This prevents option completion after a completed paranthesized option list. > For example, after "ANALYZE (VERVOSE ON) tbl (", no ANALYZE options are suggested, > instead column names of the table "tbl" are suggested. > > Similarly, this change prevents completion of COPY options after > "COPY tbl FROM stdin WITH ( FORMAT text ) WHERE (", for example. By the way, while investigating your comments, I noticed an issue with tab completion for VACUUM option values. I reported it in a separate thread [1], since it appears to be a bug introduced by another commit. [1] https://www.postgresql.org/message-id/20251223021509.19bba68ecbbc70c9f983c2b4%40sraoss.co.jp -- Yugo Nagata <nagata@sraoss.co.jp> -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Masahiko Sawada <sawada.mshk@gmail.com> — 2025-12-22T22:31:51Z
On Mon, Dec 22, 2025 at 7:37 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > On Mon, 24 Nov 2025 15:32:36 -0800 > Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > I'm sorry for the late response. > > > Thank you for updating the patch! I have one question about the > > following change: > > > > + /* 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", "(*)")) > > + { > > > > Given get_previous_words() treats a completed parenthesized option > > list as one word, why is the later if statement necessary? In the > > situation where the condition of the later if statement becomes false, > > the condition of the first 'else if' statement should have become > > false, no? > > First of all, the condition of the outer 'else if' was wrong. > What I actually meant was: > > + /* 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", "(*")) > > That is, the last word should be "(*" instead of "(". > I've attached an updated patch reflecting this fix. > > This patterm is mimic how option lists are handled in the other places. > For example: > > 3160 else if (HeadMatches("ANALYZE", "(*") && > 3161 !HeadMatches("ANALYZE", "(*)")) > > This prevents option completion after a completed paranthesized option list. > For example, after "ANALYZE (VERVOSE ON) tbl (", no ANALYZE options are suggested, > instead column names of the table "tbl" are suggested. > > Similarly, this change prevents completion of COPY options after > "COPY tbl FROM stdin WITH ( FORMAT text ) WHERE (", for example. Thank you for updating the patch and the explanation! I agree with these changes, so I've pushed the patch. Also, this commit fest item is now marked as 'Committed'. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com -
Re: psql: tab-completion support for COPY ... TO/FROM STDIN, STDOUT, and PROGRAM
Yugo Nagata <nagata@sraoss.co.jp> — 2025-12-23T01:02:17Z
On Mon, 22 Dec 2025 14:31:51 -0800 Masahiko Sawada <sawada.mshk@gmail.com> wrote: > On Mon, Dec 22, 2025 at 7:37 AM Yugo Nagata <nagata@sraoss.co.jp> wrote: > > > > On Mon, 24 Nov 2025 15:32:36 -0800 > > Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > I'm sorry for the late response. > > > > > Thank you for updating the patch! I have one question about the > > > following change: > > > > > > + /* 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", "(*)")) > > > + { > > > > > > Given get_previous_words() treats a completed parenthesized option > > > list as one word, why is the later if statement necessary? In the > > > situation where the condition of the later if statement becomes false, > > > the condition of the first 'else if' statement should have become > > > false, no? > > > > First of all, the condition of the outer 'else if' was wrong. > > What I actually meant was: > > > > + /* 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", "(*")) > > > > That is, the last word should be "(*" instead of "(". > > I've attached an updated patch reflecting this fix. > > > > This patterm is mimic how option lists are handled in the other places. > > For example: > > > > 3160 else if (HeadMatches("ANALYZE", "(*") && > > 3161 !HeadMatches("ANALYZE", "(*)")) > > > > This prevents option completion after a completed paranthesized option list. > > For example, after "ANALYZE (VERVOSE ON) tbl (", no ANALYZE options are suggested, > > instead column names of the table "tbl" are suggested. > > > > Similarly, this change prevents completion of COPY options after > > "COPY tbl FROM stdin WITH ( FORMAT text ) WHERE (", for example. > > Thank you for updating the patch and the explanation! > > I agree with these changes, so I've pushed the patch. Also, this > commit fest item is now marked as 'Committed'. Thank you! Regards, Yugo Nagata -- Yugo Nagata <nagata@sraoss.co.jp>