[REVIEW] psql tab completion for DROP TRIGGER/RULE and ALTER TABLE ... DISABLE/ENABLE
Andreas Karlsson (andreas@proxel.se) wrote:
Hi,
When benchmarking an application I got annoyed at how basic the tab
completion for ALTER TABLE ... DISABLE/ENABLE TRIGGER and DROP TRIGGER
is. So here is a patch improving the tab completion around triggers. For
consistency I have also added the same completions to rules since their
DDL is almost identical.
Thanks for this patch; I'm playing around with rules at the moment and it was
very useful. A quick review:
- applies cleanly to HEAD
- does what it claims, i.e. adds tab completion support for this syntax:
ALTER TABLE table { ENABLE | DISABLE } [ ALWAYS | REPLICA ] { RULE | TRIGGER } rule_or_trigger
DROP TRIGGER trigger ON relation { CASCADE | RESTRICT }
DROP RULE rule ON relation { CASCADE | RESTRICT }
- code style is consistent with the project style
One issue - the table's internal triggers will also be listed. which can result in
something like this:
database=> ALTER TABLE object_version DISABLE TRIGGER <TAB>
"RI_ConstraintTrigger_a_1916401" "RI_ConstraintTrigger_a_1916422" "RI_ConstraintTrigger_c_1916358"
"RI_ConstraintTrigger_a_1916402" "RI_ConstraintTrigger_c_1916238" "RI_ConstraintTrigger_c_1916359"
"RI_ConstraintTrigger_a_1916406" "RI_ConstraintTrigger_c_1916239" "RI_ConstraintTrigger_c_1916398"
"RI_ConstraintTrigger_a_1916407" "RI_ConstraintTrigger_c_1916263" "RI_ConstraintTrigger_c_1916399"
"RI_ConstraintTrigger_a_1916411" "RI_ConstraintTrigger_c_1916264" "RI_ConstraintTrigger_c_1916478"
"RI_ConstraintTrigger_a_1916412" "RI_ConstraintTrigger_c_1916298" "RI_ConstraintTrigger_c_1916479"
"RI_ConstraintTrigger_a_1916416" "RI_ConstraintTrigger_c_1916299" "RI_ConstraintTrigger_c_1916513"
"RI_ConstraintTrigger_a_1916417" "RI_ConstraintTrigger_c_1916328" "RI_ConstraintTrigger_c_1916514"
"RI_ConstraintTrigger_a_1916421" "RI_ConstraintTrigger_c_1916329" ts_vector_update
This is a bit of an extreme case, but I don't think manually manipulating
internal triggers (which can only be done as a superuser) is a common enough
operation to justify their inclusion. I suggest adding
'AND tgisinternal is FALSE' to 'Query_for_trigger_of_table' to hide them.
Regards
Ian Barwick
--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/17/2014 01:36 PM, Ian Barwick wrote:
Thanks for this patch; I'm playing around with rules at the moment and
it was
very useful. A quick review:- applies cleanly to HEAD
- does what it claims, i.e. adds tab completion support for this syntax:
ALTER TABLE table { ENABLE | DISABLE } [ ALWAYS | REPLICA ] { RULE
| TRIGGER } rule_or_trigger
DROP TRIGGER trigger ON relation { CASCADE | RESTRICT }
DROP RULE rule ON relation { CASCADE | RESTRICT }- code style is consistent with the project style
Thanks for the review.
One issue - the table's internal triggers will also be listed. which can
result in
something like this:This is a bit of an extreme case, but I don't think manually manipulating
internal triggers (which can only be done as a superuser) is a common
enough
operation to justify their inclusion. I suggest adding
'AND tgisinternal is FALSE' to 'Query_for_trigger_of_table' to hide them.
Good suggestion. I have attached a patch which filters out the internal
triggers, both for ALTER TABLE and DROP TRIGGER. I am not entirely sure
about the DROP TRIGGER case but I think I prefer no auto completion of
RI triggers.
--
Andreas Karlsson
Attachments:
tab-complete-trigger-rule-ddl-2.patchtext/x-patch; name=tab-complete-trigger-rule-ddl-2.patchDownload
diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
new file mode 100644
index 3bb727f..be5c3c5
*** a/src/bin/psql/tab-complete.c
--- b/src/bin/psql/tab-complete.c
*************** static const SchemaQuery Query_for_list_
*** 626,631 ****
--- 626,639 ----
" WHERE pg_catalog.quote_ident(conname)='%s')"
/* the silly-looking length condition is just to eat up the current word */
+ #define Query_for_rule_of_table \
+ "SELECT pg_catalog.quote_ident(rulename) "\
+ " FROM pg_catalog.pg_class c1, pg_catalog.pg_rewrite "\
+ " WHERE c1.oid=ev_class and (%d = pg_catalog.length('%s'))"\
+ " 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_list_of_tables_for_rule \
"SELECT pg_catalog.quote_ident(relname) "\
" FROM pg_catalog.pg_class"\
*************** static const SchemaQuery Query_for_list_
*** 635,640 ****
--- 643,657 ----
" WHERE pg_catalog.quote_ident(rulename)='%s')"
/* the silly-looking length condition is just to eat up the current word */
+ #define Query_for_trigger_of_table \
+ "SELECT pg_catalog.quote_ident(tgname) "\
+ " FROM pg_catalog.pg_class c1, pg_catalog.pg_trigger "\
+ " WHERE c1.oid=tgrelid 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 tgisinternal"
+
+ /* the silly-looking length condition is just to eat up the current word */
#define Query_for_list_of_tables_for_trigger \
"SELECT pg_catalog.quote_ident(relname) "\
" FROM pg_catalog.pg_class"\
*************** static const pgsql_thing_t words_after_c
*** 774,780 ****
{"TEMP", NULL, NULL, THING_NO_DROP}, /* for CREATE TEMP TABLE ... */
{"TEMPLATE", Query_for_list_of_ts_templates, NULL, THING_NO_SHOW},
{"TEXT SEARCH", NULL, NULL},
! {"TRIGGER", "SELECT pg_catalog.quote_ident(tgname) FROM pg_catalog.pg_trigger WHERE substring(pg_catalog.quote_ident(tgname),1,%d)='%s'"},
{"TYPE", NULL, &Query_for_list_of_datatypes},
{"UNIQUE", NULL, NULL, THING_NO_DROP}, /* for CREATE UNIQUE INDEX ... */
{"UNLOGGED", NULL, NULL, THING_NO_DROP}, /* for CREATE UNLOGGED TABLE
--- 791,797 ----
{"TEMP", NULL, NULL, THING_NO_DROP}, /* for CREATE TEMP TABLE ... */
{"TEMPLATE", Query_for_list_of_ts_templates, NULL, THING_NO_SHOW},
{"TEXT SEARCH", NULL, NULL},
! {"TRIGGER", "SELECT pg_catalog.quote_ident(tgname) FROM pg_catalog.pg_trigger WHERE substring(pg_catalog.quote_ident(tgname),1,%d)='%s' AND NOT tgisinternal"},
{"TYPE", NULL, &Query_for_list_of_datatypes},
{"UNIQUE", NULL, NULL, THING_NO_DROP}, /* for CREATE UNIQUE INDEX ... */
{"UNLOGGED", NULL, NULL, THING_NO_DROP}, /* for CREATE UNLOGGED TABLE
*************** psql_completion(const char *text, int st
*** 1409,1414 ****
--- 1426,1463 ----
COMPLETE_WITH_LIST(list_ALTERENABLE2);
}
+ else if (pg_strcasecmp(prev5_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev2_wd, "ENABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "RULE") == 0)
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_rule_of_table);
+ }
+ else if (pg_strcasecmp(prev6_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev5_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev3_wd, "ENABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "RULE") == 0)
+ {
+ completion_info_charp = prev4_wd;
+ COMPLETE_WITH_QUERY(Query_for_rule_of_table);
+ }
+ else if (pg_strcasecmp(prev5_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev2_wd, "ENABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "TRIGGER") == 0)
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_trigger_of_table);
+ }
+ else if (pg_strcasecmp(prev6_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev5_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev3_wd, "ENABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "TRIGGER") == 0)
+ {
+ completion_info_charp = prev4_wd;
+ COMPLETE_WITH_QUERY(Query_for_trigger_of_table);
+ }
/* ALTER TABLE xxx INHERIT */
else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
*************** psql_completion(const char *text, int st
*** 1424,1429 ****
--- 1473,1479 ----
{
COMPLETE_WITH_SCHEMA_QUERY(Query_for_list_of_tables, "");
}
+ /* ALTER TABLE xxx DISABLE */
else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
pg_strcasecmp(prev3_wd, "TABLE") == 0 &&
pg_strcasecmp(prev_wd, "DISABLE") == 0)
*************** psql_completion(const char *text, int st
*** 1433,1438 ****
--- 1483,1504 ----
COMPLETE_WITH_LIST(list_ALTERDISABLE);
}
+ else if (pg_strcasecmp(prev5_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev2_wd, "DISABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "RULE") == 0)
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_rule_of_table);
+ }
+ else if (pg_strcasecmp(prev5_wd, "ALTER") == 0 &&
+ pg_strcasecmp(prev4_wd, "TABLE") == 0 &&
+ pg_strcasecmp(prev2_wd, "DISABLE") == 0 &&
+ pg_strcasecmp(prev_wd, "TRIGGER") == 0)
+ {
+ completion_info_charp = prev3_wd;
+ COMPLETE_WITH_QUERY(Query_for_trigger_of_table);
+ }
/* ALTER TABLE xxx ALTER */
else if (pg_strcasecmp(prev4_wd, "ALTER") == 0 &&
*************** psql_completion(const char *text, int st
*** 2587,2592 ****
--- 2653,2681 ----
COMPLETE_WITH_LIST(list_ALTERTEXTSEARCH);
}
+ /* DROP TRIGGER */
+ else if (pg_strcasecmp(prev3_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev2_wd, "TRIGGER") == 0)
+ {
+ COMPLETE_WITH_CONST("ON");
+ }
+ else if (pg_strcasecmp(prev4_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev3_wd, "TRIGGER") == 0 &&
+ pg_strcasecmp(prev_wd, "ON") == 0)
+ {
+ completion_info_charp = prev2_wd;
+ COMPLETE_WITH_QUERY(Query_for_list_of_tables_for_trigger);
+ }
+ else if (pg_strcasecmp(prev5_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev4_wd, "TRIGGER") == 0 &&
+ pg_strcasecmp(prev2_wd, "ON") == 0)
+ {
+ static const char *const list_DROPCR[] =
+ {"CASCADE", "RESTRICT", NULL};
+
+ COMPLETE_WITH_LIST(list_DROPCR);
+ }
+
/* DROP EVENT TRIGGER */
else if (pg_strcasecmp(prev2_wd, "DROP") == 0 &&
pg_strcasecmp(prev_wd, "EVENT") == 0)
*************** psql_completion(const char *text, int st
*** 2600,2605 ****
--- 2689,2717 ----
COMPLETE_WITH_QUERY(Query_for_list_of_event_triggers);
}
+ /* DROP RULE */
+ else if (pg_strcasecmp(prev3_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev2_wd, "RULE") == 0)
+ {
+ COMPLETE_WITH_CONST("ON");
+ }
+ else if (pg_strcasecmp(prev4_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev3_wd, "RULE") == 0 &&
+ pg_strcasecmp(prev_wd, "ON") == 0)
+ {
+ completion_info_charp = prev2_wd;
+ COMPLETE_WITH_QUERY(Query_for_list_of_tables_for_rule);
+ }
+ else if (pg_strcasecmp(prev5_wd, "DROP") == 0 &&
+ pg_strcasecmp(prev4_wd, "RULE") == 0 &&
+ pg_strcasecmp(prev2_wd, "ON") == 0)
+ {
+ static const char *const list_DROPCR[] =
+ {"CASCADE", "RESTRICT", NULL};
+
+ COMPLETE_WITH_LIST(list_DROPCR);
+ }
+
/* EXECUTE, but not EXECUTE embedded in other commands */
else if (pg_strcasecmp(prev_wd, "EXECUTE") == 0 &&
prev2_wd[0] == '\0')
On 14/06/18 7:51, Andreas Karlsson wrote:
On 06/17/2014 01:36 PM, Ian Barwick wrote:
One issue - the table's internal triggers will also be listed. which can
result in
something like this:This is a bit of an extreme case, but I don't think manually manipulating
internal triggers (which can only be done as a superuser) is a common
enough
operation to justify their inclusion. I suggest adding
'AND tgisinternal is FALSE' to 'Query_for_trigger_of_table' to hide them.Good suggestion. I have attached a patch which filters out the internal triggers,
both for ALTER TABLE and DROP TRIGGER. I am not entirely sure about the DROP TRIGGER
case but I think I prefer no auto completion of RI triggers.
Thanks, looks good. Another reason for not autocompleting RI triggers is that
the names are all auto-generated; on the offchance you are manually manipulating
them individually, you'd have to have a pretty good idea of which ones you're
working with anyway.
Personally I think this patch could go into 9.4, as it's not introducing any
new features and doesn't depend on any 9.5 syntax.
Regards
Ian Barwick
--
Ian Barwick http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/18/2014 02:34 AM, Ian Barwick wrote:
On 14/06/18 7:51, Andreas Karlsson wrote:
On 06/17/2014 01:36 PM, Ian Barwick wrote:
One issue - the table's internal triggers will also be listed. which can
result in
something like this:This is a bit of an extreme case, but I don't think manually manipulating
internal triggers (which can only be done as a superuser) is a common
enough
operation to justify their inclusion. I suggest adding
'AND tgisinternal is FALSE' to 'Query_for_trigger_of_table' to hide them.Good suggestion. I have attached a patch which filters out the internal triggers,
both for ALTER TABLE and DROP TRIGGER. I am not entirely sure about the DROP TRIGGER
case but I think I prefer no auto completion of RI triggers.Thanks, looks good. Another reason for not autocompleting RI triggers is that
the names are all auto-generated; on the offchance you are manually manipulating
them individually, you'd have to have a pretty good idea of which ones you're
working with anyway.
Thanks, could you update the status of the patch in the commitfest app
to "Ready for Committer"?
Personally I think this patch could go into 9.4, as it's not introducing any
new features and doesn't depend on any 9.5 syntax.
I think the feature freeze is strict to avoid having to think about what
is an exception and what is not.
--
Andreas Karlsson
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 06/18/2014 03:39 AM, Andreas Karlsson wrote:
On 06/18/2014 02:34 AM, Ian Barwick wrote:
On 14/06/18 7:51, Andreas Karlsson wrote:
On 06/17/2014 01:36 PM, Ian Barwick wrote:
One issue - the table's internal triggers will also be listed. which can
result in
something like this:This is a bit of an extreme case, but I don't think manually manipulating
internal triggers (which can only be done as a superuser) is a common
enough
operation to justify their inclusion. I suggest adding
'AND tgisinternal is FALSE' to 'Query_for_trigger_of_table' to hide them.Good suggestion. I have attached a patch which filters out the internal triggers,
both for ALTER TABLE and DROP TRIGGER. I am not entirely sure about the DROP TRIGGER
case but I think I prefer no auto completion of RI triggers.Thanks, looks good. Another reason for not autocompleting RI triggers is that
the names are all auto-generated; on the offchance you are manually manipulating
them individually, you'd have to have a pretty good idea of which ones you're
working with anyway.Thanks, could you update the status of the patch in the commitfest app
to "Ready for Committer"?
Thanks, committed.
- Heikki
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers