tab complete for COPY populated materialized view TO

Started by jian he9 months ago9 messages
#1jian he
jian.universality@gmail.com
1 attachment(s)

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1]https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578.
The attached patch is to add a tab complete for it.

[1]: https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Attachments:

v1-0001-tab-complete-for-COPY-populated-materialized-view.patchtext/x-patch; charset=US-ASCII; name=v1-0001-tab-complete-for-COPY-populated-materialized-view.patchDownload
From c016a14b42d4f23bf5ece7ab8374d6e93bc5646f Mon Sep 17 00:00:00 2001
From: jian he <jian.universality@gmail.com>
Date: Wed, 9 Apr 2025 16:16:54 +0800
Subject: [PATCH v1 1/1] tab complete for COPY populated materialized view TO

---
 src/bin/psql/tab-complete.in.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index c916b9299a8..2261c0253c5 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -683,6 +683,17 @@ static const SchemaQuery Query_for_list_of_tables = {
 	.result = "c.relname",
 };
 
+static const SchemaQuery Query_for_list_of_tables_for_copy = {
+	.catname = "pg_catalog.pg_class c",
+	.selcondition =
+	"c.relkind IN (" CppAsString2(RELKIND_RELATION) ", "
+	CppAsString2(RELKIND_MATVIEW) ", "
+	CppAsString2(RELKIND_PARTITIONED_TABLE) ") and c.relispopulated ",
+	.viscondition = "pg_catalog.pg_table_is_visible(c.oid)",
+	.namespace = "c.relnamespace",
+	.result = "c.relname",
+};
+
 static const SchemaQuery Query_for_list_of_partitioned_tables = {
 	.catname = "pg_catalog.pg_class c",
 	.selcondition = "c.relkind IN (" CppAsString2(RELKIND_PARTITIONED_TABLE) ")",
@@ -3249,7 +3260,7 @@ match_previous_words(int pattern_id,
 	 * backslash command).
 	 */
 	else if (Matches("COPY|\\copy"))
-		COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_tables, "(");
+		COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_tables_for_copy, "(");
 	/* Complete COPY ( with legal query commands */
 	else if (Matches("COPY|\\copy", "("))
 		COMPLETE_WITH("SELECT", "TABLE", "VALUES", "INSERT INTO", "UPDATE", "DELETE FROM", "MERGE INTO", "WITH");
-- 
2.34.1

#2Kirill Reshke
reshkekirill@gmail.com
In reply to: jian he (#1)
Re: tab complete for COPY populated materialized view TO

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com> wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0]/messages/by-id/CACJufxHjBPrsbNZAp-DCmwvOE_Gkogb+rhfqqe1=S5cOHR-V7Q@mail.gmail.com will be pushed.

[0]: /messages/by-id/CACJufxHjBPrsbNZAp-DCmwvOE_Gkogb+rhfqqe1=S5cOHR-V7Q@mail.gmail.com

--
Best regards,
Kirill Reshke

#3Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Kirill Reshke (#2)
Re: tab complete for COPY populated materialized view TO

On 2025/04/09 18:25, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com> wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0] will be pushed.

Is the partitioned table currently tab-completed for the COPY FROM case?

If we aim to support tab-completion for all valid targets of both COPY TO
and COPY FROM, shouldn't foreign tables also be included? And what about
views with INSTEAD OF INSERT triggers - though maybe that's overkill?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

#4Kirill Reshke
reshkekirill@gmail.com
In reply to: Fujii Masao (#3)
Re: tab complete for COPY populated materialized view TO

On Wed, 9 Apr 2025 at 14:45, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/09 18:25, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com> wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0] will be pushed.

Is the partitioned table currently tab-completed for the COPY FROM case?

If I'm not mistaken, yes. I double checked.

INSTEAD OF INSERT triggers - though maybe that's overkill?

That's wild to me, psql tab completions feature designed to support
postgresql not fully, but in frequent cases. So maybe we should keep
it stupud.

--
Best regards,
Kirill Reshke

#5Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Kirill Reshke (#4)
1 attachment(s)
Re: tab complete for COPY populated materialized view TO

On 2025/04/09 19:24, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 14:45, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/09 18:25, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com> wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0] will be pushed.

Is the partitioned table currently tab-completed for the COPY FROM case?

If I'm not mistaken, yes. I double checked.

INSTEAD OF INSERT triggers - though maybe that's overkill?

That's wild to me, psql tab completions feature designed to support
postgresql not fully, but in frequent cases. So maybe we should keep
it stupud.

I agree that it's reasonable to exclude such rarely used objects from
tab-completion. How about including just tables, partitioned tables,
foreign tables, and materialized views?
I've attached a patch for that.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Attachments:

v2-0001-psql-Improve-tab-completion-for-COPY-command.patchtext/plain; charset=UTF-8; name=v2-0001-psql-Improve-tab-completion-for-COPY-command.patchDownload
From 7f14bf122427a30f62e98ad65d4856455578cfc3 Mon Sep 17 00:00:00 2001
From: Fujii Masao <fujii@postgresql.org>
Date: Wed, 9 Apr 2025 18:21:18 +0900
Subject: [PATCH v2] psql: Improve tab completion for COPY command.

Previously, tab completion for COPY only suggested plain tables
and partitioned tables, even though materialized views are also
valid for COPY TO (since commit 534874fac0b), and foreign tables
are valid for COPY FROM.

This commit enhances tab completion for COPY to also include
materialized views and foreign tables.

Views with INSTEAD OF INSERT triggers are supported with
COPY FROM but rarely used, so plain views are intentionally
excluded from completion.
---
 src/bin/psql/tab-complete.in.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/bin/psql/tab-complete.in.c b/src/bin/psql/tab-complete.in.c
index c916b9299a8..9846b086ef2 100644
--- a/src/bin/psql/tab-complete.in.c
+++ b/src/bin/psql/tab-complete.in.c
@@ -889,6 +889,14 @@ static const SchemaQuery Query_for_list_of_analyzables = {
 	.result = "c.relname",
 };
 
+/*
+ * Relations supporting COPY TO/FROM are currently almost the same as
+ * those supporting ANALYZE. Although views with INSTEAD OF INSERT triggers
+ * can be used with COPY FROM, they are rarely used for this purpose,
+ * so plain views are intentionally excluded from this tab completion.
+ */
+#define Query_for_list_of_tables_for_copy Query_for_list_of_analyzables
+
 /* Relations supporting index creation */
 static const SchemaQuery Query_for_list_of_indexables = {
 	.catname = "pg_catalog.pg_class c",
@@ -3249,7 +3257,7 @@ match_previous_words(int pattern_id,
 	 * backslash command).
 	 */
 	else if (Matches("COPY|\\copy"))
-		COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_tables, "(");
+		COMPLETE_WITH_SCHEMA_QUERY_PLUS(Query_for_list_of_tables_for_copy, "(");
 	/* Complete COPY ( with legal query commands */
 	else if (Matches("COPY|\\copy", "("))
 		COMPLETE_WITH("SELECT", "TABLE", "VALUES", "INSERT INTO", "UPDATE", "DELETE FROM", "MERGE INTO", "WITH");
-- 
2.49.0

#6Kirill Reshke
reshkekirill@gmail.com
In reply to: Fujii Masao (#5)
Re: tab complete for COPY populated materialized view TO

On Thu, 10 Apr 2025 at 20:07, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/09 19:24, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 14:45, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2025/04/09 18:25, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com> wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0] will be pushed.

Is the partitioned table currently tab-completed for the COPY FROM case?

If I'm not mistaken, yes. I double checked.

INSTEAD OF INSERT triggers - though maybe that's overkill?

That's wild to me, psql tab completions feature designed to support
postgresql not fully, but in frequent cases. So maybe we should keep
it stupud.

I agree that it's reasonable to exclude such rarely used objects from
tab-completion. How about including just tables, partitioned tables,
foreign tables, and materialized views?
I've attached a patch for that.

Regards,

Patch is ok. However...

If we aim to support tab-completion for all valid targets of both COPY TO

and COPY FROM, shouldn't foreign tables also be included?

Ah.. Sorry I missed this part of your message initially. No, foreign
tables are not supported:

```
reshke=# CREATE FOREIGN TABLE ft (i int) server zz OPTIONS ( filename
'zz.csv', format 'csv' );
reshke=# copy ft to stdout;
ERROR: cannot copy from foreign table "ft"
HINT: Try the COPY (SELECT ...) TO variant.
```

So we will tab complete for cases that yet to be supported (foreign
tables and partitioned tables);

What's funny is that copying foreign tables using MV works fine

```
reshke=# create materialized view mv as table ft;
SELECT 1
reshke=# copy mv to stdout;
228
```

--
Best regards,
Kirill Reshke

#7David G. Johnston
david.g.johnston@gmail.com
In reply to: Kirill Reshke (#6)
Re: tab complete for COPY populated materialized view TO

On Thursday, April 10, 2025, Kirill Reshke <reshkekirill@gmail.com> wrote:

On Thu, 10 Apr 2025 at 20:07, Fujii Masao <masao.fujii@oss.nttdata.com>
wrote:

On 2025/04/09 19:24, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 14:45, Fujii Masao <masao.fujii@oss.nttdata.com>

wrote:

On 2025/04/09 18:25, Kirill Reshke wrote:

On Wed, 9 Apr 2025 at 13:23, jian he <jian.universality@gmail.com>

wrote:

hi.

we allow the "COPY table TO" command to copy rows from materialized
views in [1].
The attached patch is to add a tab complete for it.

[1] https://git.postgresql.org/cgit/postgresql.git/commit/?id=

534874fac0b34535c9a5ab9257d6574f78423578

Hi!
Patch works good for me, but I noticed that psql COPY <tab> suggests
partitioned relation both with and without this patch. Maybe that's
not a big problem, if [0] will be pushed.

Is the partitioned table currently tab-completed for the COPY FROM

case?

If I'm not mistaken, yes. I double checked.

INSTEAD OF INSERT triggers - though maybe that's overkill?

That's wild to me, psql tab completions feature designed to support
postgresql not fully, but in frequent cases. So maybe we should keep
it stupud.

I agree that it's reasonable to exclude such rarely used objects from
tab-completion. How about including just tables, partitioned tables,
foreign tables, and materialized views?
I've attached a patch for that.

Regards,

Patch is ok. However...

I concur with the premise of the patch. Tab-complete is going to happen
before we know whether to/from is specified so the syntax limits our smarts
here.

If we aim to support tab-completion for all valid targets of both COPY TO

and COPY FROM, shouldn't foreign tables also be included?

Ah.. Sorry I missed this part of your message initially. No, foreign
tables are not supported:

They are supported for the From variant; valid completions need only
satisfy one of to/from, not both.

What's funny is that copying foreign tables using MV works fine

```
reshke=# create materialized view mv as table ft;
SELECT 1
reshke=# copy mv to stdout;
228
```

I don’t get why this is “funny” or otherwise surprising.

David J.

#8Kirill Reshke
reshkekirill@gmail.com
In reply to: David G. Johnston (#7)
Re: tab complete for COPY populated materialized view TO

On Fri, 11 Apr 2025 at 00:33, David G. Johnston
<david.g.johnston@gmail.com> wrote:

They are supported for the From variant; valid completions need only satisfy one of to/from, not both.

Thank you. If so, then WFM, and I don't have any more objections.

--
Best regards,
Kirill Reshke

#9Fujii Masao
masao.fujii@oss.nttdata.com
In reply to: Kirill Reshke (#8)
Re: tab complete for COPY populated materialized view TO

On 2025/04/11 4:37, Kirill Reshke wrote:

On Fri, 11 Apr 2025 at 00:33, David G. Johnston
<david.g.johnston@gmail.com> wrote:

They are supported for the From variant; valid completions need only satisfy one of to/from, not both.

Thank you. If so, then WFM, and I don't have any more objections.

I've pushed the patch. Thanks!

Regards,

--
Fujii Masao
NTT DATA Japan Corporation