Bug fix for tab completion of ALTER TABLE ... VALIDATE CONSTRAINT ...
Folks,
I noticed that $subject completes with already valid constraints,
please find attached a patch that fixes it. I noticed that there are
other places constraints can be validated, but didn't check whether
similar bugs exist there yet.
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v1-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patchtext/x-diff; charset=us-asciiDownload
From e191928111a7500c3a3bc84558797fe86451c24b Mon Sep 17 00:00:00 2001
From: David Fetter <david@fetter.org>
Date: Mon, 26 Apr 2021 17:17:15 -0700
Subject: [PATCH v1] tab-completion of ALTER TABLE...VALIDATE CONSTRAINT
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="------------2.30.2"
This is a multi-part message in MIME format.
--------------2.30.2
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit
...now chooses only among constraints that aren't already valid.
diff --git src/bin/psql/tab-complete.c src/bin/psql/tab-complete.c
index 7c4933333b..2252b7d3ac 100644
--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -785,6 +785,15 @@ static const SchemaQuery Query_for_list_of_collations = {
" and pg_catalog.quote_ident(c1.relname)='%s'"\
" and pg_catalog.pg_table_is_visible(c1.oid)"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_nonvalid_constraint_of_table \
+"SELECT pg_catalog.quote_ident(conname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_constraint con "\
+" WHERE c1.oid=conrelid and and not convalidated"\
+" and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c1.oid)"
+
#define Query_for_all_table_constraints \
"SELECT pg_catalog.quote_ident(conname) "\
" FROM pg_catalog.pg_constraint c "\
@@ -2118,11 +2127,16 @@ psql_completion(const char *text, int start, int end)
* If we have ALTER TABLE <sth> ALTER|DROP|RENAME|VALIDATE CONSTRAINT,
* provide list of constraints
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME|VALIDATE", "CONSTRAINT"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME", "CONSTRAINT"))
{
completion_info_charp = prev3_wd;
COMPLETE_WITH_QUERY(Query_for_constraint_of_table);
}
+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table);
+ }
/* ALTER TABLE ALTER [COLUMN] <foo> */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny) ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny))
--------------2.30.2--
Hi David,
I noticed that $subject completes with already valid constraints,
please find attached a patch that fixes it. I noticed that there are
other places constraints can be validated, but didn't check whether
similar bugs exist there yet.
There was a typo in the patch ("... and and not convalidated"). I've fixed
it. Otherwise the patch passes all the tests and works as expected:
eax=# create table foo (x int);
CREATE TABLE
eax=# alter table foo add constraint bar check (x < 3) not valid;
ALTER TABLE
eax=# alter table foo add constraint baz check (x <> 5) not valid;
ALTER TABLE
eax=# alter table foo validate constraint ba
bar baz
eax=# alter table foo validate constraint bar;
ALTER TABLE
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patchapplication/octet-stream; name=v2-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patchDownload
diff --git src/bin/psql/tab-complete.c src/bin/psql/tab-complete.c
index 7c4933333b..b1bca24714 100644
--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -785,6 +785,15 @@ static const SchemaQuery Query_for_list_of_collations = {
" and pg_catalog.quote_ident(c1.relname)='%s'"\
" and pg_catalog.pg_table_is_visible(c1.oid)"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_nonvalid_constraint_of_table \
+"SELECT pg_catalog.quote_ident(conname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_constraint con "\
+" WHERE c1.oid=conrelid and not convalidated"\
+" and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c1.oid)"
+
#define Query_for_all_table_constraints \
"SELECT pg_catalog.quote_ident(conname) "\
" FROM pg_catalog.pg_constraint c "\
@@ -2118,11 +2127,16 @@ psql_completion(const char *text, int start, int end)
* If we have ALTER TABLE <sth> ALTER|DROP|RENAME|VALIDATE CONSTRAINT,
* provide list of constraints
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME|VALIDATE", "CONSTRAINT"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME", "CONSTRAINT"))
{
completion_info_charp = prev3_wd;
COMPLETE_WITH_QUERY(Query_for_constraint_of_table);
}
+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table);
+ }
/* ALTER TABLE ALTER [COLUMN] <foo> */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny) ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny))
Hi hackers,
Otherwise the patch passes all the tests and works as expected
I've noticed there is no tab completion for ALTER TABLE xxx ADD. Here
is an alternative version of the patch that fixes this as well. Not
sure if this should be in the same commit though.
--
Best regards,
Aleksander Alekseev
Attachments:
v3-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patchapplication/octet-stream; name=v3-0001-tab-completion-of-ALTER-TABLE.VALIDATE-CONSTRAINT.patchDownload
diff --git src/bin/psql/tab-complete.c src/bin/psql/tab-complete.c
index 7c4933333b..b2b8431bc2 100644
--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -785,6 +785,15 @@ static const SchemaQuery Query_for_list_of_collations = {
" and pg_catalog.quote_ident(c1.relname)='%s'"\
" and pg_catalog.pg_table_is_visible(c1.oid)"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_nonvalid_constraint_of_table \
+"SELECT pg_catalog.quote_ident(conname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_constraint con "\
+" WHERE c1.oid=conrelid and not convalidated"\
+" and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c1.oid)"
+
#define Query_for_all_table_constraints \
"SELECT pg_catalog.quote_ident(conname) "\
" FROM pg_catalog.pg_constraint c "\
@@ -2107,8 +2116,8 @@ psql_completion(const char *text, int start, int end)
else if (Matches("ALTER", "TABLE", MatchAny, "RENAME", "COLUMN|CONSTRAINT", MatchAnyExcept("TO")))
COMPLETE_WITH("TO");
- /* If we have ALTER TABLE <sth> DROP, provide COLUMN or CONSTRAINT */
- else if (Matches("ALTER", "TABLE", MatchAny, "DROP"))
+ /* If we have ALTER TABLE <sth> ADD|DROP, provide COLUMN or CONSTRAINT */
+ else if (Matches("ALTER", "TABLE", MatchAny, "ADD|DROP"))
COMPLETE_WITH("COLUMN", "CONSTRAINT");
/* If we have ALTER TABLE <sth> DROP COLUMN, provide list of columns */
else if (Matches("ALTER", "TABLE", MatchAny, "DROP", "COLUMN"))
@@ -2118,11 +2127,16 @@ psql_completion(const char *text, int start, int end)
* If we have ALTER TABLE <sth> ALTER|DROP|RENAME|VALIDATE CONSTRAINT,
* provide list of constraints
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME|VALIDATE", "CONSTRAINT"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME", "CONSTRAINT"))
{
completion_info_charp = prev3_wd;
COMPLETE_WITH_QUERY(Query_for_constraint_of_table);
}
+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table);
+ }
/* ALTER TABLE ALTER [COLUMN] <foo> */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny) ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny))
On Tue, Apr 27, 2021 at 12:33:25PM +0300, Aleksander Alekseev wrote:
Hi David,
I noticed that $subject completes with already valid constraints,
please find attached a patch that fixes it. I noticed that there are
other places constraints can be validated, but didn't check whether
similar bugs exist there yet.There was a typo in the patch ("... and and not convalidated"). I've fixed
it. Otherwise the patch passes all the tests and works as expected:eax=# create table foo (x int);
CREATE TABLE
eax=# alter table foo add constraint bar check (x < 3) not valid;
ALTER TABLE
eax=# alter table foo add constraint baz check (x <> 5) not valid;
ALTER TABLE
eax=# alter table foo validate constraint ba
bar baz
eax=# alter table foo validate constraint bar;
ALTER TABLE
Sorry about that typo, and thanks for poking at this!
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
On Tue, Apr 27, 2021 at 12:58:52PM +0300, Aleksander Alekseev wrote:
I've noticed there is no tab completion for ALTER TABLE xxx ADD. Here
is an alternative version of the patch that fixes this as well. Not
sure if this should be in the same commit though.
- /* If we have ALTER TABLE <sth> DROP, provide COLUMN or CONSTRAINT */
- else if (Matches("ALTER", "TABLE", MatchAny, "DROP"))
+ /* If we have ALTER TABLE <sth> ADD|DROP, provide COLUMN or CONSTRAINT */
+ else if (Matches("ALTER", "TABLE", MatchAny, "ADD|DROP"))
Seems to me that the behavior to not complete with COLUMN or
CONSTRAINT for ADD is intentional, as it is possible to specify a
constraint or column name without the object type first. This
introduces a inconsistent behavior with what we do for columns with
ADD, for one. So a more consistent approach would be to list columns,
constraints, COLUMN and CONSTRAINT in the list of options available
after ADD.+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table);
+ }
Specifying valid constraints is an authorized grammar, so it does not
seem that bad to keep things as they are, either. I would leave that
alone.
--
Michael
On 19 May 2021, at 09:53, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Apr 27, 2021 at 12:58:52PM +0300, Aleksander Alekseev wrote:
I've noticed there is no tab completion for ALTER TABLE xxx ADD. Here
is an alternative version of the patch that fixes this as well. Not
sure if this should be in the same commit though.- /* If we have ALTER TABLE <sth> DROP, provide COLUMN or CONSTRAINT */ - else if (Matches("ALTER", "TABLE", MatchAny, "DROP")) + /* If we have ALTER TABLE <sth> ADD|DROP, provide COLUMN or CONSTRAINT */ + else if (Matches("ALTER", "TABLE", MatchAny, "ADD|DROP")) Seems to me that the behavior to not complete with COLUMN or CONSTRAINT for ADD is intentional, as it is possible to specify a constraint or column name without the object type first. This introduces a inconsistent behavior with what we do for columns with ADD, for one. So a more consistent approach would be to list columns, constraints, COLUMN and CONSTRAINT in the list of options available after ADD.+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT")) + { + completion_info_charp = prev3_wd; + COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table); + } Specifying valid constraints is an authorized grammar, so it does not seem that bad to keep things as they are, either. I would leave that alone.
This has stalled being marked Waiting on Author since May, and reading the
above it sounds like marking it Returned with Feedback is the logical next step
(patch also no longer applies).
--
Daniel Gustafsson https://vmware.com/
On Fri, Sep 03, 2021 at 08:27:55PM +0200, Daniel Gustafsson wrote:
On 19 May 2021, at 09:53, Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Apr 27, 2021 at 12:58:52PM +0300, Aleksander Alekseev wrote:
I've noticed there is no tab completion for ALTER TABLE xxx ADD. Here
is an alternative version of the patch that fixes this as well. Not
sure if this should be in the same commit though.- /* If we have ALTER TABLE <sth> DROP, provide COLUMN or CONSTRAINT */ - else if (Matches("ALTER", "TABLE", MatchAny, "DROP")) + /* If we have ALTER TABLE <sth> ADD|DROP, provide COLUMN or CONSTRAINT */ + else if (Matches("ALTER", "TABLE", MatchAny, "ADD|DROP")) Seems to me that the behavior to not complete with COLUMN or CONSTRAINT for ADD is intentional, as it is possible to specify a constraint or column name without the object type first. This introduces a inconsistent behavior with what we do for columns with ADD, for one. So a more consistent approach would be to list columns, constraints, COLUMN and CONSTRAINT in the list of options available after ADD.+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT")) + { + completion_info_charp = prev3_wd; + COMPLETE_WITH_QUERY(Query_for_nonvalid_constraint_of_table); + } Specifying valid constraints is an authorized grammar, so it does not seem that bad to keep things as they are, either. I would leave that alone.This has stalled being marked Waiting on Author since May, and reading the
above it sounds like marking it Returned with Feedback is the logical next step
(patch also no longer applies).
Please find attached the next revision :)
Best,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778
Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
Attachments:
v4-0001-psql-Fix-tab-completion-for-ALTER-TABLE.patchtext/x-diff; charset=us-asciiDownload
From 716e9dde96ef9973fc6b0fc9ca9934df153af9da Mon Sep 17 00:00:00 2001
From: David Fetter <david.fetter@aiven.io>
Date: Tue, 14 Sep 2021 15:28:34 -0700
Subject: [PATCH v4] psql: Fix tab completion for ALTER TABLE...
... VALIDATE CONSTRAINT.
Completions now include only constraints which have not yet been
validated.
---
src/bin/psql/tab-complete.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git src/bin/psql/tab-complete.c src/bin/psql/tab-complete.c
index 5cd5838668..b55166aa64 100644
--- src/bin/psql/tab-complete.c
+++ src/bin/psql/tab-complete.c
@@ -788,6 +788,15 @@ Query_for_index_of_table \
" and pg_catalog.quote_ident(c1.relname)='%s'"\
" and pg_catalog.pg_table_is_visible(c1.oid)"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_constraint_of_table_not_validated \
+"SELECT pg_catalog.quote_ident(conname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_constraint con "\
+" WHERE c1.oid=conrelid and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c1.oid)" \
+" and NOT con.convalidated"
+
#define Query_for_all_table_constraints \
"SELECT pg_catalog.quote_ident(conname) "\
" FROM pg_catalog.pg_constraint c "\
@@ -2145,14 +2154,22 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_ATTR(prev3_wd, "");
/*
- * If we have ALTER TABLE <sth> ALTER|DROP|RENAME|VALIDATE CONSTRAINT,
+ * If we have ALTER TABLE <sth> ALTER|DROP|RENAME CONSTRAINT,
* provide list of constraints
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME|VALIDATE", "CONSTRAINT"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME", "CONSTRAINT"))
{
completion_info_charp = prev3_wd;
COMPLETE_WITH_QUERY(Query_for_constraint_of_table);
}
+ /*
+ * ALTER TABLE <sth> VALIDATE CONSTRAINT <non-validated constraint>
+ */
+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_constraint_of_table_not_validated);
+ }
/* ALTER TABLE ALTER [COLUMN] <foo> */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny) ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny))
--
2.31.1
Hi David,
Please find attached the next revision :)
The patch didn't apply and couldn't pass cfbot [1]http://cfbot.cputube.org/patch_34_3113.log. The (hopefully)
corrected patch is attached. Other than that it looks OK to me but let's
see what cfbot will tell.
[1]: http://cfbot.cputube.org/patch_34_3113.log
--
Best regards,
Aleksander Alekseev
Attachments:
v5-0001-psql-Fix-tab-completion-for-ALTER-TABLE.patchapplication/octet-stream; name=v5-0001-psql-Fix-tab-completion-for-ALTER-TABLE.patchDownload
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 5cd5838668..b55166aa64 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -788,6 +788,15 @@ Query_for_index_of_table \
" and pg_catalog.quote_ident(c1.relname)='%s'"\
" and pg_catalog.pg_table_is_visible(c1.oid)"
+/* the silly-looking length condition is just to eat up the current word */
+#define Query_for_constraint_of_table_not_validated \
+"SELECT pg_catalog.quote_ident(conname) "\
+" FROM pg_catalog.pg_class c1, pg_catalog.pg_constraint con "\
+" WHERE c1.oid=conrelid and (%d = pg_catalog.length('%s'))"\
+" and pg_catalog.quote_ident(c1.relname)='%s'"\
+" and pg_catalog.pg_table_is_visible(c1.oid)" \
+" and NOT con.convalidated"
+
#define Query_for_all_table_constraints \
"SELECT pg_catalog.quote_ident(conname) "\
" FROM pg_catalog.pg_constraint c "\
@@ -2145,14 +2154,22 @@ psql_completion(const char *text, int start, int end)
COMPLETE_WITH_ATTR(prev3_wd, "");
/*
- * If we have ALTER TABLE <sth> ALTER|DROP|RENAME|VALIDATE CONSTRAINT,
+ * If we have ALTER TABLE <sth> ALTER|DROP|RENAME CONSTRAINT,
* provide list of constraints
*/
- else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME|VALIDATE", "CONSTRAINT"))
+ else if (Matches("ALTER", "TABLE", MatchAny, "ALTER|DROP|RENAME", "CONSTRAINT"))
{
completion_info_charp = prev3_wd;
COMPLETE_WITH_QUERY(Query_for_constraint_of_table);
}
+ /*
+ * ALTER TABLE <sth> VALIDATE CONSTRAINT <non-validated constraint>
+ */
+ else if (Matches("ALTER", "TABLE", MatchAny, "VALIDATE", "CONSTRAINT"))
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_constraint_of_table_not_validated);
+ }
/* ALTER TABLE ALTER [COLUMN] <foo> */
else if (Matches("ALTER", "TABLE", MatchAny, "ALTER", "COLUMN", MatchAny) ||
Matches("ALTER", "TABLE", MatchAny, "ALTER", MatchAny))
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: tested, passed
Documentation: tested, passed
The cfbot seems to be happy with the updated patch.
The new status of this patch is: Ready for Committer
