Thomas Munro wrote:
> Thanks, good point. Here's a version that uses NULL via a macro ANY.
> Aside from a few corrections it also now distinguishes between
> TAIL_MATCHESn (common) and MATCHESn (rarely used for now), for example:
This looks pretty neat -- 100x neater than what we have, at any rate. I
would use your new MATCHESn() macros a bit more -- for instance the
completion for "ALTER but not ALTER after ALTER TABLE" could be
rephrased as simply MATCHES1("ALTER"), i.e. have it match at start of
command only. Maybe that's just a matter of going over the new code
after the initial run, so that we can have a first patch that's mostly
mechanical and a second pass in which more semantically relevant changes
are applied. Seems easier to review ...
I would use "ANY" as a keyword here. Sounds way too generic to me.
Maybe "CompleteAny" or something like that.
Stylistically, I find there's too much uppercasing here. Maybe rename the
macros like this instead:
> + else if (TailMatches4("ALL", "IN", "TABLESPACE", ANY))
> + CompleteWithList2("SET TABLESPACE", "OWNED BY");
Not totally sure about this part TBH.
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
