The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, failed
Documentation:            tested, passed

Hi Christoph,

The patch have a potential, although I have to agree with Jim Jones, it 
obviously have a problem with "alter view <name> set<tab>" handling.

First of all user can notice, that SET and RESET alternatives are proposed in 
an absolutely equivalent way:
postgres=# alter view VVV <tab>
ALTER COLUMN  OWNER TO      RENAME        RESET (       SET (         SET SCHEMA

But completion of a parentheses differs fore these alternatives:

postgres=# alter view VVV reset<tab> -> completes with "<space>(" -> <tab>
CHECK_OPTION      SECURITY_BARRIER  SECURITY_INVOKER

postgres=# alter view VVV set<tab> -> completes with a single spase -> <tab>
Display all 188 possibilities? (y or n)
(and these 188 possibilities do not contain "(")

The probmen is obviously in the newly added second line of the following clause:
                COMPLETE_WITH("ALTER COLUMN", "OWNER TO", "RENAME",
                                          "SET SCHEMA", "SET (", "RESET (");

"set schema" and "set (" alternatives are competing, while completion of the 
common part "set<space>" leads to a string composition which does not have the 
check branch (Matches("ALTER", "VIEW", MatchAny, "SET")).

I think it may worth looking at "alter materialized view"  completion tree and 
making "alter view" the same way.

The new status of this patch is: Waiting on Author

Reply via email to