Hi Baji,

I tested v2 on top of fded90eec3e. Both patches apply cleanly (the CHECK
one has to go on top of the ALTER CONSTRAINT one) and build
without warnings.

I tested psql through a pty and compared master against patched for about
50 inputs:
- ALTER TABLE t ALTER CONSTRAINT fk <tab> now offers DEFERRABLE,
ENFORCED, INHERIT, INITIALLY DEFERRED/IMMEDIATE, NO INHERIT,
NOT DEFERRABLE, NOT ENFORCED; and NOT/NO/INITIALLY each complete to the
matching second word, so Chao's point is addressed.

- ALTER TABLE t ADD [CONSTRAINT c] CHECK (...) <tab> offers NO
INHERIT, NOT ENFORCED, NOT VALID, and NOT/NO follow up correctly.
This also works with nested parentheses in the expression and with
CHECK(a>0) written without a space.

- Lower-case input, a schema-qualified table and a quoted constraint name
all behave the same.

- The ALTER TABLE rules (ALTER [COLUMN] x ..., ALTER
CONSTRAINT <tab> listing names, DROP/RENAME/VALIDATE CONSTRAINT,
ADD, ADD CONSTRAINT x, ADD [CONSTRAINT x] NOT NULL, PRIMARY
KEY/UNIQUE, ADD c int) give identical output with and without the patches.

I also ran every suggested form against the server, all are accepted. NOT
VALID is correctly left out of the ALTER CONSTRAINT list, since
the grammar rejects it there. src/bin/psql/t/010_tab_completion.pl passes.

Two small, optional suggestions:
  1. After "ALTER CONSTRAINT fk DEFERRABLE" (or NOT DEFERRABLE)
nothing further is offered, although "DEFERRABLE INITIALLY DEFERRED" is
probably the most common way to write it. A rule
offering INITIALLY DEFERRED / INITIALLY IMMEDIATE there would
be nice, but I'm fine leaving it out to keep the patch small.

2. A couple of cases in 010_tab_completion.pl, e.g. for "alter table tab1
alter constraint c not<tab>" and
"... check (a > 0) no<tab>", would keep the new rules from silently
regressing.

Neither is a blocker, v2 looks good to me as is.

Regards,
Surya Poondla

Reply via email to